-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Save teardown exceptions for further teardowns #13002
base: main
Are you sure you want to change the base?
Changes from 7 commits
1a892d0
11b5de4
1535fa2
9abac4d
ae683a9
a8e0aba
d5a146f
067d8fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Teardown fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions`. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be a list and not an exception group? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I didn't realize they were immutable.. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -539,19 +539,20 @@ | |
if list(self.stack.keys()) == needed_collectors[: len(self.stack)]: | ||
break | ||
node, (finalizers, _) = self.stack.popitem() | ||
these_exceptions = [] | ||
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
+549
to
+555
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing as there are no "teardown fixtures", perhaps: