Skip to content
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ Kian Eliasi
Kian-Meng Ang
Kodi B. Arfer
Kojo Idrissa
Konstantin Shkel
Kostis Anagnostopoulos
Kristoffer Nordström
Kyle Altendorf
Expand Down
3 changes: 3 additions & 0 deletions src/_pytest/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ def __init__(
# Deprecated alias. Was never public. Can be removed in a few releases.
self._store = self.stash

#: A list of exceptions that happened during teardown
self.teardown_exceptions: list[BaseException] = []
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a list and not an exception group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use a list then pass the list to the exception group, you can't append things to a group

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't realize they were immutable..

Copy link
Copy Markdown

@storenth storenth Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys, where to put tests? Can u suggest appropriate place? Looks like: testing/test_collection.py and teardown?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged, thank you. Now we have a test to check if the fixture fails and exception is added to the list


@classmethod
def from_parent(cls, parent: Node, **kw) -> Self:
"""Public constructor for Nodes.
Expand Down
14 changes: 8 additions & 6 deletions src/_pytest/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -539,19 +539,21 @@ def teardown_exact(self, nextitem: Item | None) -> None:
if list(self.stack.keys()) == needed_collectors[: len(self.stack)]:
break
node, (finalizers, _) = self.stack.popitem()
these_exceptions = []
node.teardown_exceptions = []
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to reinitialize the list? Shouldn't that already be done in Node.__init__?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there's no other code interacting with this list, you are correct - this is not really needed

while finalizers:
fin = finalizers.pop()
try:
fin()
except TEST_OUTCOME as e:
these_exceptions.append(e)
node.teardown_exceptions.append(e)

if len(these_exceptions) == 1:
exceptions.extend(these_exceptions)
elif these_exceptions:
if len(node.teardown_exceptions) == 1:
exceptions.extend(node.teardown_exceptions)
elif node.teardown_exceptions:
msg = f"errors while tearing down {node!r}"
exceptions.append(BaseExceptionGroup(msg, these_exceptions[::-1]))
exceptions.append(
BaseExceptionGroup(msg, node.teardown_exceptions[::-1])
)
Comment on lines +571 to +577
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perhaps something for another PR (or too late/not worth changing), but from trio's experience with strict_exception_groups it's generally a bad design philosophy to sometimes return an exception group. This leads users to write code where they assume there's only ever one or no exceptions, and everything breaks when >1 exceptions happen to occur.
Maybe it's less of an issue here, given that we're not raising the exceptions, but I could see this complicating parsing logic nevertheless.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're raising them a few lines later, after all teardowns. However, this is a design change, and probably should be done in another PR


if len(exceptions) == 1:
raise exceptions[0]
Expand Down