update comments, and remove test relying on internal functionality

changelog/12135.bugfix.rst

@@ -1 +1 @@
-Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances.
+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,19 +1035,25 @@ class FixtureDef(Generic[FixtureValue]):
             raise BaseExceptionGroup(msg, exceptions[::-1])
 
     def execute(self, request: SubRequest) -> FixtureValue:
-        # Ensure arguments (parent fixtures) are loaded.
-        # If their cache has been invalidated it will finish itself and subfixtures,
-        # which will set our self.cached_result = None.
-        # If/when parent fixture parametrization is included in our cache key this
-        # can be moved after checking our cache key and not require saving in a list.
-        parent_fixtures_to_add_finalizer = []
+        """Return the value of this fixture, executing it if not cached."""
+        # Ensure dependent fixtures that this fixture requests 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):
-                # save fixture as one to add our finalizer to, if we're not cached
-                # resolves #12135
-                parent_fixtures_to_add_finalizer.append(fixturedef)
+                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]
@@ -1065,9 +1071,11 @@ 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)
-        # add finalizer to parent fixtures
-        for parent_fixture in parent_fixtures_to_add_finalizer:
+        for parent_fixture in requested_fixtures_that_should_finalize_us:
             parent_fixture.addfinalizer(finalizer)
 
         ihook = request.node.ihook

testing/python/fixtures.py

@@ -4753,39 +4753,6 @@ def test_scoped_fixture_teardown_order(pytester: Pytester) -> None:
     assert result.ret == 0
 
 
-def test_subfixture_not_queue_multiple_finalizers(pytester: Pytester) -> None:
-    """Make sure subfixtures don't needlessly requeue their finalizer multiple times in
-    parent fixture.
-
-    Regression test for #12135
-    """
-    pytester.makepyfile(
-        """
-        import pytest
-
-        def _assert_len(request):
-            assert len(request._get_active_fixturedef('fixture_1')._finalizers) == 1
-
-
-        @pytest.fixture(scope="module")
-        def fixture_1():
-            ...
-
-        @pytest.fixture(scope="module")
-        def fixture_2(request, fixture_1):
-            yield
-
-        def test_1(request, fixture_2):
-            _assert_len(request)
-
-        def test_2(request, fixture_2):
-            _assert_len(request)
-        """
-    )
-    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