Fix teardown error reporting when --maxfail=1#11721
Fix teardown error reporting when --maxfail=1#11721bluetech merged 7 commits intopytest-dev:mainfrom bbrown1867:bbrown/11706/fix-teardown-reporting
--maxfail=1#11721Conversation
| cc: @bluetech |
RonnyPfannschmidt left a comment
RonnyPfannschmidt left a comment There was a problem hiding this comment.
this looks good for dealing with a number of edge cases
should we backport it?
| Thanks for the review @RonnyPfannschmidt! Yeah I think backporting to 7.4.x would be useful to pick these changes up sooner. |
RonnyPfannschmidt added the needs backport | @RonnyPfannschmidt - is anything needed on my end to get this merged? Or are we waiting for more maintainers to review? |
| I'm down as with a headache, so I'd like other's to merge |
bluetech left a comment
bluetech left a comment There was a problem hiding this comment.
Thanks for send a PR @bbrown1867.
I left a couple of comments on the diff, but here are the high level comments:
-
Similar to
session.shouldfail, there issession.shouldstop, which we should handle as well. You should be able to test it by usingpytest.exitinstead ofpytest.fail. -
You've put the
nextitemcheck inrunner.py'spytest_runtest_teardownhook implementation, but I think it should be done inruntestprotocol, such that other hook impls in other plugins getnextitem=Noneas well. -
I am still somewhat worried about this being quite subtle and other plugins might mess with with
shouldfail/shouldstopwhich will then cause very confusing situations. Currently I can't come up with a better fix, so I think we should do it. But I'd like to mitigate the risk a bit: Let's turnshouldstopandshouldfailinto properties, with setters which prevent true -> false state transitions.
As for backporting, I think this is too risky for that. And considering that it is not a regression but has been this way forever AFAICT, I think we shouldn't backport.
bluetech removed the needs backport 
bbrown1867 left a comment
bbrown1867 left a comment There was a problem hiding this comment.
Thanks for the feedback @bluetech! I think I've implemented all of your feedback.
| def shouldstop(self, value: Union[bool, str]) -> None: | ||
| """Prevent plugins from setting this to False once it has been set.""" | ||
| if value is False and self._shouldstop: | ||
| return |
There was a problem hiding this comment.
@bluetech should an exception be raised here? Or is simply ignoring the operation okay?
There was a problem hiding this comment.
I think silently ignoring is no good, it will be confusing why there is no effect.
The two options are issuing a warning, or raising an exception. Since we're already stopping raising an exception seems kinda pointless, so I think we should go with a warning. I think it can be something like this:
# The runner checks shouldfail and assumes that if it is set we are definitely stopping, # so prevent unsetting it. warnings.warn(PytestWarning("session.shouldfail cannot be unset after it has been set; ignoring."), stacklevel=2)Unfortunately it does mean you'll need to write extra tests to cover these warnings, let me know if you need help with that.
There was a problem hiding this comment.
Okay sounds good, I've added the warnings and and a unit test for the shouldfail scenario called test_shouldfail_is_sticky. I'm using pytest_sessionfinish to trigger the bad behavior. However this same approach did not work for shouldstop - it seems like shouldstop was still False in pytest_sessionfinish even if I changed pytest.fail to pytest.exit. Any ideas on how to test the shouldstop scenario?
There was a problem hiding this comment.
The only thing which sets shouldstop in pytest core is --stepwise, which actually has the same problem as --maxfail. Since I've already checked out the branch, I've pushed the new test along with some tweaks. This is now good to go, thanks!
…eck for shouldstop
…-> False transition
…shouldfail, add unit test to test_session.py
…1721)" Fix pytest-dev#12021. Reopens pytest-dev#11706. This reverts commit 12b9bd5. This change caused a bad regression in pytest-xdist: pytest-dev/pytest-xdist#1024 pytest-xdist necessarily has special handling of `--maxfail` and session fixture teardown get executed multiple times with the change. Since I'm not sure how to adapt pytest-xdist myself, revert for now. I kept the sticky `shouldstop`/`shouldfail` changes as they are good ideas regardless I think.
Revert "Fix teardown error reporting when `--maxfail=1` (#11721)"
[8.0.x] Revert "Fix teardown error reporting when `--maxfail=1` (#11721)"
…1721)" Fix pytest-dev#12021. Reopens pytest-dev#11706. This reverts commit 12b9bd5. This change caused a bad regression in pytest-xdist: pytest-dev/pytest-xdist#1024 pytest-xdist necessarily has special handling of `--maxfail` and session fixture teardown get executed multiple times with the change. Since I'm not sure how to adapt pytest-xdist myself, revert for now. I kept the sticky `shouldstop`/`shouldfail` changes as they are good ideas regardless I think.
…2279) Closes #11706. Originally fixed in #11721, but then reverted in #12022 due to a regression in pytest-xdist. The regression was fixed on the pytest-xdist side in pytest-dev/pytest-xdist#1026.
3 participants
Closes #11706.