pytest
pytest copied to clipboard
pytest-dev
Save teardown exceptions for further teardowns
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 https://github.com/microsoft/playwright-pytest/pull/207, not relying on traceback hacks but getting the information directly.
Looks like we need reviewers: @nicoddemus @daara-s @graingert @Zac-HD @bluetech @jakkdl could you please review this improvement? thanks)
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
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.
@webknjaz @daara-s @jakkdl @graingert @nicoddemus @mxschmitt please share you thoughts?