diff --git a/changelog/12135.bugfix.rst b/changelog/12135.bugfix.rst new file mode 100644 index 000000000..734733b10 --- /dev/null +++ b/changelog/12135.bugfix.rst @@ -0,0 +1 @@ +Fix fixtures adding their finalizer multiple times to fixtures they request, causing unreliable and non-intuitive teardown ordering in some instances. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index a8fed047e..265ed601d 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1035,14 +1035,25 @@ class FixtureDef(Generic[FixtureValue]): raise BaseExceptionGroup(msg, exceptions[::-1]) def execute(self, request: SubRequest) -> FixtureValue: - finalizer = functools.partial(self.finish, request=request) - # Get required arguments and register our own finish() - # with their finalization. + """Return the value of this fixture, executing it if not cached.""" + # Ensure that the dependent fixtures requested by this fixture are loaded. + # This needs to be done before checking if we have a cached value, since + # if a dependent fixture has their cache invalidated, e.g. due to + # parametrization, they finalize themselves and fixtures depending on it + # (which will likely include this fixture) setting `self.cached_result = None`. + # See #4871 + requested_fixtures_that_should_finalize_us = [] for argname in self.argnames: fixturedef = request._get_active_fixturedef(argname) + # Saves requested fixtures in a list so we later can add our finalizer + # to them, ensuring that if a requested fixture gets torn down we get torn + # down first. This is generally handled by SetupState, but still currently + # needed when this fixture is not parametrized but depends on a parametrized + # fixture. if not isinstance(fixturedef, PseudoFixtureDef): - fixturedef.addfinalizer(finalizer) + requested_fixtures_that_should_finalize_us.append(fixturedef) + # Check for (and return) cached value/exception. my_cache_key = self.cache_key(request) if self.cached_result is not None: cache_key = self.cached_result[1] @@ -1060,6 +1071,13 @@ class FixtureDef(Generic[FixtureValue]): self.finish(request) assert self.cached_result is None + # Add finalizer to requested fixtures we saved previously. + # We make sure to do this after checking for cached value to avoid + # adding our finalizer multiple times. (#12135) + finalizer = functools.partial(self.finish, request=request) + for parent_fixture in requested_fixtures_that_should_finalize_us: + parent_fixture.addfinalizer(finalizer) + ihook = request.node.ihook try: # Setup the fixture, run the code in it, and cache the value diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 1e22270e5..12ca6e926 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4751,3 +4751,55 @@ def test_scoped_fixture_teardown_order(pytester: Pytester) -> None: ) result = pytester.runpytest() assert result.ret == 0 + + +def test_subfixture_teardown_order(pytester: Pytester) -> None: + """ + Make sure fixtures don't re-register their finalization in parent fixtures multiple + times, causing ordering failure in their teardowns. + + Regression test for #12135 + """ + pytester.makepyfile( + """ + import pytest + + execution_order = [] + + @pytest.fixture(scope="class") + def fixture_1(): + ... + + @pytest.fixture(scope="class") + def fixture_2(fixture_1): + execution_order.append("setup 2") + yield + execution_order.append("teardown 2") + + @pytest.fixture(scope="class") + def fixture_3(fixture_1): + execution_order.append("setup 3") + yield + execution_order.append("teardown 3") + + class TestFoo: + def test_initialize_fixtures(self, fixture_2, fixture_3): + ... + + # This would previously reschedule fixture_2's finalizer in the parent fixture, + # causing it to be torn down before fixture 3. + def test_reschedule_fixture_2(self, fixture_2): + ... + + # Force finalization directly on fixture_1 + # Otherwise the cleanup would sequence 3&2 before 1 as normal. + @pytest.mark.parametrize("fixture_1", [None], indirect=["fixture_1"]) + def test_finalize_fixture_1(self, fixture_1): + ... + + def test_result(): + assert execution_order == ["setup 2", "setup 3", "teardown 3", "teardown 2"] + """ + ) + result = pytester.runpytest() + assert result.ret == 0