Save teardown exceptions for further teardowns by Myp3a · Pull Request #13002 · pytest-dev/pytest (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR adds an ability to access the exceptions that happened during teardown in other teardowns.
Motivation: some plugins (playwright-pytest) must rely on current teardown status to properly execute their hooks. Currently the teardown information isn't available, so it must be extracted with different workarounds.
Doesn't change functionality, only exposes the local variable on Node object.
Allows proper implementation of microsoft/playwright-pytest#207, not relying on traceback hacks but getting the information directly.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW such changes must be always accompanied by respective tests.
| @@ -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] = [] |
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?
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
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..
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
test: tests for teardown exceptions patch
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just a small change in the test and needs a changelog (see contributing docs)
| def pytest_exception_interact(node, call, report): |
|---|
| sys.stderr.write("{}".format(node.teardown_exceptions)) |
| import pytest |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor - importing pytest twice in this conftest block
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine, just a couple small things.
Comment on lines +550 to +556
| 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]) |
| ) |
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.
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
| these_exceptions = [] |
|---|
| node.teardown_exceptions = [] |
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__?
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
Comment on lines 1595 to 1612
| def pytest_exception_interact(node, call, report): |
|---|
| sys.stderr.write("{}".format(node.teardown_exceptions)) |
| import pytest |
| @pytest.fixture |
| def mylist(): |
| yield |
| raise AssertionError(111) |
| """, |
| test_file=""" |
| def test_func(mylist): |
| assert True |
| """, |
| ) |
| result = pytester.runpytest() |
| assert result.ret == ExitCode.TESTS_FAILED |
| assert "AssertionError(111)" in result.stderr.str() |
| result.stdout.fnmatch_lines(["*1 error*"]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks a bit unreliable to me, assert "AssertionError(111)" in result.stderr.str() could plausibly pass for other reasons than pytest_exception_interact printing it. Printing some marker characters surrounding it should suffice though, e.g. assert "node.teardown_exceptions: `AssertionError(111)`" in result.stderr.str().
It's also better to use result.assert_outcomes(errors=1) than fnmatch'ing lines
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, should add a comment referring to #9909 for why there's both a pass and a fail to the test.
I don't think I have anything to add to the discussion in #9909 though
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to comment, the PR doesn't touch the pytest architecture.
oh actually, probably nice to add to the comment that it's only saved for the sake of external post-teardown inspection; and not required internally. If there's any docs for the Node type (that's not just autogenerated) they might also need updating
Guys, are we waiting for something?) Looks like we did all stuff from reviewers
Guys, are we waiting for something?) Looks like we did all stuff from reviewers
it's usually good habit to wait a couple days to see if anybody else wants to review, though this is minor enough that I feel comfortable merging it soon
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, I wonder though if keeping those exceptions around might cause more memory to be used than expected?
Perhaps not given those are only really related to exceptions during teardown...
An alternative for this, in case this is a problem, would be to implement a new hook, and call that hook passing the node and exception (or exceptions?), instead of storing them in the Node.
I'm not sure if this is needed, just food for thought in case storing the exceptions is something we should be more careful about.
| @@ -0,0 +1 @@ |
|---|
| Teardown fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions`. |
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:
| Teardown fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions`. |
|---|
| Fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions` during their own teardowns. |
I wonder though if keeping those exceptions around might cause more memory to be used than expected?
Keeping the exceptions alive keeps the tracebacks alive which keeps all the locals in all the frames alive
Keeping the exceptions alive keeps the tracebacks alive which keeps all the locals in all the frames alive
Indeed, that's why I'm bringing this up.
Initially I was thinking this might not be a big problem, but actually this might really be catastrophic in case you have a fixture which is used a lot (say autouse), which then would fail for every test onward and store a lot of data in memory, possibly causing the machine to run out of memory and crashing, instead of being able to report the problem...
To be safe it is probably best to introduce a new hook, which would circumvent the problem and also allow plugins to handle the exceptions as they appear (possibly handling them immediately or storing them for later, depending on their need).
To be safe it is probably best to introduce a new hook...
nicoddemus agree, but, what if we rewrite self.teardown_exceptions to use it as iterator? But, if no chance, do you prefer we refactor to handle hook based on this PR or create another?
what if we rewrite self.teardown_exceptions to use it as iterator?
What do you mean? Doesn't that require storing the exceptions anyway, even if we deliver those later as an iterator?
But, if no chance, do you prefer we refactor to handle hook based on this PR or create another?
Might be better to create a new one, and close this one but keeping it on the repository for historical reasons.
What do you mean? Doesn't that require storing the exceptions anyway, even if we deliver those later as an iterator?
What about a hook? Doesn't it meanning we need to store same thing but in another place? Confused a little bit.
What about a hook? Doesn't it meanning we need to store same thing but in another place? Confused a little bit.
No, using a hook, instead of storing the exception like this:
try: fin() except TEST_OUTCOME as e: node.teardown_exceptions.append(e)
We just call the hook at that point, passing the exception:
try: fin() except TEST_OUTCOME as e: self.config.ihook.pytest_handle_teardown_exception(item=item, e=e)
So pytest itself will no longer long term store the tracebacks, and whoever implements that hook can decide what to do (they might want to store the tracebacks anyway, but then it is the plugin's decision).
Of course we will need to restore the these_exceptions variable in order to raise the ExceptionGroup, however this will be local and not so much of a problem (just as before).
Using a hook like this to forward information to plugins is central to pytest's design, and used a lot: for example test reports are not stored anywhere, they are passed to hooks, which are then processed by plugins, like the terminal and junitxml.
I apologize for the delay, wasn't feeling well for a couple of days. However, now I'm ready to support this further.
I wonder though if keeping those exceptions around might cause more memory to be used than expected?
@nicoddemus @graingert
Aren't we doing the same thing now? Line 541 of runner.py makes a list of exceptions, which are passed to the ExceptionGroup later. So, we're keeping all the exceptions and tracebacks for the entire teardown.
This one also wouldn't keep Node objects, as the exceptions list is only used while tearing down this exact node, and afterwards only the exceptions are kept to raise them later. After raising, execution stops - so all objects are discarded and shouldn't hog the memory.
I'm not sure about the hook. If that's an important design feature, I can agree on making it.
However, I see a clear advantage of using a exception list: next fixtures can access all ongoing exceptions. If we're using a hook, it receives the information about only one specific exception, with no relation to previous exceptions. This can help with tracing what led to the test failure.