Skip to content
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

Fix teardown error reporting when --maxfail=1 #11721

Merged
merged 7 commits into from
Jan 3, 2024
Merged

Fix teardown error reporting when --maxfail=1 #11721

merged 7 commits into from
Jan 3, 2024

Conversation

bbrown1867
Copy link
Contributor

Closes #11706.

@bbrown1867 bbrown1867 marked this pull request as ready for review December 18, 2023 18:20
@bbrown1867
Copy link
Contributor Author

cc: @bluetech

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

this looks good for dealing with a number of edge cases

should we backport it?

@bbrown1867
Copy link
Contributor Author

Thanks for the review @RonnyPfannschmidt! Yeah I think backporting to 7.4.x would be useful to pick these changes up sooner.

@RonnyPfannschmidt RonnyPfannschmidt added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Dec 21, 2023
@bbrown1867
Copy link
Contributor Author

@RonnyPfannschmidt - is anything needed on my end to get this merged? Or are we waiting for more maintainers to review?

@RonnyPfannschmidt
Copy link
Member

I'm down as with a headache, so I'd like other's to merge

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

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 is session.shouldstop, which we should handle as well. You should be able to test it by using pytest.exit instead of pytest.fail.

  • You've put the nextitem check in runner.py's pytest_runtest_teardown hook implementation, but I think it should be done in runtestprotocol, such that other hook impls in other plugins get nextitem=None as well.

  • I am still somewhat worried about this being quite subtle and other plugins might mess with with shouldfail/shouldstop which 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 turn shouldstop and shouldfail into 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.

testing/test_runner.py Outdated Show resolved Hide resolved
testing/test_runner.py Outdated Show resolved Hide resolved
@bluetech bluetech removed the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Dec 31, 2023
Copy link
Contributor Author

@bbrown1867 bbrown1867 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bluetech should an exception be raised here? Or is simply ignoring the operation okay?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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!

@bluetech bluetech merged commit 12b9bd5 into pytest-dev:main Jan 3, 2024
23 of 24 checks passed
@bbrown1867 bbrown1867 deleted the bbrown/11706/fix-teardown-reporting branch January 3, 2024 18:29
bluetech added a commit to bluetech/pytest that referenced this pull request Feb 23, 2024
…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.
bluetech added a commit that referenced this pull request Feb 23, 2024
Revert "Fix teardown error reporting when `--maxfail=1` (#11721)"
bluetech added a commit that referenced this pull request Feb 23, 2024
[8.0.x] Revert "Fix teardown error reporting when `--maxfail=1` (#11721)"
flying-sheep pushed a commit to flying-sheep/pytest that referenced this pull request Apr 9, 2024
…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.
bluetech pushed a commit that referenced this pull request May 2, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pytest aborts when fixture errors during teardown and --maxfail=1
3 participants