fixtures: a couple of bug fixes for pytest_fixture_post_finalizer#14275
Open
bluetech wants to merge 3 commits intopytest-dev:mainfrom
Open
fixtures: a couple of bug fixes for pytest_fixture_post_finalizer#14275bluetech wants to merge 3 commits intopytest-dev:mainfrom
pytest_fixture_post_finalizer#14275bluetech wants to merge 3 commits intopytest-dev:mainfrom
Conversation
…r called more than once
…le times Fixes pytest-dev#5848. Co-authored-by: Ran Benita <ran@unusedvar.com>
Previously, if `pytest_fixture_post_finalizer` raised an error, the fixture was not reset properly, because the `FixtureDef.finish()` was disrupted. Fix this by making `pytest_fixture_post_finalizer` itself part of the teardown as a finalizer, which already has proper error handling. If it raises, it's handled (and shown to the user) the same as a teardown failure. Fix pytest-dev#14114.
psf-chronographer
bot
added
the
bot:chronographer:provided
Contributor
|
LGTM, @RonnyPfannschmidt please review |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are changes extracted from @Anton3's PR #14104. That PR is big, so I decided to split this part so we can have a focused discussion and decision on these particular changes, and then #14104's can be a bit smaller.
The first test only adds a failing (xfail) test for #5848, that is that
pytest_fixture_post_finalizeris sometimes called twice for the same teardown. I think this is definitely undesirable. We could state thatpytest_fixture_post_finalizerneeds to be idempotent and be prepared to be called multiple times, but I don't see any reason for that. It's also somewhat wasteful.The second commit is not from @Anton3's PR, but just fixes the above problem in the most direct way, namely to do an early exit if already finished (
cached_resultisNone). There is a possible gotcha here is somehowaddfinalizeris called afterfinish, but I'm pretty sure this can't happen. Would have added an assert but it's not easy I think. I think this is good change anyway to avoid uselessly running thefinishcode when it will have no effect (well, except thepytest_fixture_post_finalizercall).The third commit is @Anton3's different clever fix for the problem and also for another issue #14114 that things aren't handled gracefully if
pytest_fixture_post_finalizerraises. I'm not a 100% on this:pytest_fixture_post_finalizeris not very realistic, generally if hooks raise it's not something we really try to handle, it's basically a broken plugin. I think this is also what @RonnyPfannschmidt mean in the issue.However, I think handling it is still nice, and the semantics can make sense from a certain perspective. So it has my approval..