Don't add fixture finalizer if the value is cached (#11833)

Fixes #1489

changelog/1489.bugfix.rst

@@ -0,0 +1 @@
+Fix some instances where teardown of higher-scoped fixtures was not happening in the reverse order they were initialized in.

src/_pytest/fixtures.py

@@ -543,6 +543,11 @@ class FixtureRequest(abc.ABC):
         :raises pytest.FixtureLookupError:
             If the given fixture could not be found.
         """
+        # Note that in addition to the use case described in the docstring,
+        # getfixturevalue() is also called by pytest itself during item and fixture
+        # setup to evaluate the fixtures that are requested statically
+        # (using function parameters, autouse, etc).
+
         fixturedef = self._get_active_fixturedef(argname)
         assert fixturedef.cached_result is not None, (
             f'The fixture value for "{argname}" is not available.  '
@@ -587,9 +592,8 @@ class FixtureRequest(abc.ABC):
         """Create a SubRequest based on "self" and call the execute method
         of the given FixtureDef object.
 
-        This will force the FixtureDef object to throw away any previous
+        If the FixtureDef has cached the result it will do nothing, otherwise it will
-        results and compute a new fixture value, which will be stored into
+        setup and run the fixture, cache the value, and schedule a finalizer for it.
-        the FixtureDef object itself.
         """
         # prepare a subrequest object before calling fixture function
         # (latter managed by fixturedef)
@@ -646,18 +650,9 @@ class FixtureRequest(abc.ABC):
         subrequest = SubRequest(
             self, scope, param, param_index, fixturedef, _ispytest=True
         )
-        try:
-            # Call the fixture function.
-            fixturedef.execute(request=subrequest)
-        finally:
-            self._schedule_finalizers(fixturedef, subrequest)
 
-    def _schedule_finalizers(
+        # Make sure the fixture value is cached, running it if it isn't
-        self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest"
+        fixturedef.execute(request=subrequest)
-    ) -> None:
-        # If fixture function failed it might have registered finalizers.
-        finalizer = functools.partial(fixturedef.finish, request=subrequest)
-        subrequest.node.addfinalizer(finalizer)
 
 
 @final
@@ -788,21 +783,6 @@ class SubRequest(FixtureRequest):
     def addfinalizer(self, finalizer: Callable[[], object]) -> None:
         self._fixturedef.addfinalizer(finalizer)
 
-    def _schedule_finalizers(
-        self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest"
-    ) -> None:
-        # If the executing fixturedef was not explicitly requested in the argument list (via
-        # getfixturevalue inside the fixture call) then ensure this fixture def will be finished
-        # first.
-        if (
-            fixturedef.argname not in self._fixture_defs
-            and fixturedef.argname not in self._pyfuncitem.fixturenames
-        ):
-            fixturedef.addfinalizer(
-                functools.partial(self._fixturedef.finish, request=self)
-            )
-        super()._schedule_finalizers(fixturedef, subrequest)
-
 
 @final
 class FixtureLookupError(LookupError):
@@ -1055,12 +1035,13 @@ 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.
         for argname in self.argnames:
             fixturedef = request._get_active_fixturedef(argname)
             if not isinstance(fixturedef, PseudoFixtureDef):
-                fixturedef.addfinalizer(functools.partial(self.finish, request=request))
+                fixturedef.addfinalizer(finalizer)
 
         my_cache_key = self.cache_key(request)
         if self.cached_result is not None:
@@ -1080,7 +1061,14 @@ class FixtureDef(Generic[FixtureValue]):
             assert self.cached_result is None
 
         ihook = request.node.ihook
-        result = ihook.pytest_fixture_setup(fixturedef=self, request=request)
+        try:
+            # Setup the fixture, run the code in it, and cache the value
+            # in self.cached_result
+            result = ihook.pytest_fixture_setup(fixturedef=self, request=request)
+        finally:
+            # schedule our finalizer, even if the setup failed
+            request.node.addfinalizer(finalizer)
+
         return result
 
     def cache_key(self, request: SubRequest) -> object:

testing/python/fixtures.py

@@ -4696,3 +4696,58 @@ def test_scoped_fixture_caching_exception(pytester: Pytester) -> None:
     )
     result = pytester.runpytest()
     assert result.ret == 0
+
+
+def test_scoped_fixture_teardown_order(pytester: Pytester) -> None:
+    """
+    Make sure teardowns happen in reverse order of setup with scoped fixtures, when
+    a later test only depends on a subset of scoped fixtures.
+
+    Regression test for https://github.com/pytest-dev/pytest/issues/1489
+    """
+    pytester.makepyfile(
+        """
+        from typing import Generator
+
+        import pytest
+
+
+        last_executed = ""
+
+
+        @pytest.fixture(scope="module")
+        def fixture_1() -> Generator[None, None, None]:
+            global last_executed
+            assert last_executed == ""
+            last_executed = "fixture_1_setup"
+            yield
+            assert last_executed == "fixture_2_teardown"
+            last_executed = "fixture_1_teardown"
+
+
+        @pytest.fixture(scope="module")
+        def fixture_2() -> Generator[None, None, None]:
+            global last_executed
+            assert last_executed == "fixture_1_setup"
+            last_executed = "fixture_2_setup"
+            yield
+            assert last_executed == "run_test"
+            last_executed = "fixture_2_teardown"
+
+
+        def test_fixture_teardown_order(fixture_1: None, fixture_2: None) -> None:
+            global last_executed
+            assert last_executed == "fixture_2_setup"
+            last_executed = "run_test"
+
+
+        def test_2(fixture_1: None) -> None:
+            # This would previously queue an additional teardown of fixture_1,
+            # despite fixture_1's value being cached, which caused fixture_1 to be
+            # torn down before fixture_2 - violating the rule that teardowns should
+            # happen in reverse order of setup.
+            pass
+        """
+    )
+    result = pytester.runpytest()
+    assert result.ret == 0