Don't reregister subfixture finalizer in requested fixture if value is cached (pytest-dev#12136)

Author: jakkdl

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.

src/_pytest/fixtures.py

@@ -1035,14 +1035,25 @@ def finish(self, request: SubRequest) -> None:
             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 @@ def execute(self, request: SubRequest) -> 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

testing/python/fixtures.py

@@ -4751,3 +4751,55 @@ def test_2(fixture_1: None) -> 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