Don't add fixture finalizer if the value is cached

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

@@ -534,6 +534,8 @@ def getfixturevalue(self, argname: str) -> Any:
         :raises pytest.FixtureLookupError:
             If the given fixture could not be found.
         """
+        # Note: This is called during setup for evaluating fixtures defined via
+        # function arguments as well.
         fixturedef = self._get_active_fixturedef(argname)
         assert fixturedef.cached_result is not None, (
             f'The fixture value for "{argname}" is not available.  '
@@ -570,9 +572,8 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None:
         """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
-        results and compute a new fixture value, which will be stored into
-        the FixtureDef object itself.
+        If the FixtureDef has cached the result it will do nothing, otherwise it will
+        setup and run the fixture, cache the value, and schedule a finalizer for it.
         """
         # prepare a subrequest object before calling fixture function
         # (latter managed by fixturedef)
@@ -633,11 +634,8 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None:
 
         # Check if a higher-level scoped fixture accesses a lower level one.
         subrequest._check_scope(argname, self._scope, scope)
-        try:
-            # Call the fixture function.
-            fixturedef.execute(request=subrequest)
-        finally:
-            self._schedule_finalizers(fixturedef, subrequest)
+        # Make sure the fixture value is cached, running it if it isn't
+        fixturedef.execute(request=subrequest)
 
     def _schedule_finalizers(
         self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest"
@@ -1043,15 +1041,17 @@ def finish(self, request: SubRequest) -> None:
             self.cached_result = None
             self._finalizers.clear()
 
+    # Note: the return value is entirely unused, no tests depend on it
     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 argname != "request":
                 # PseudoFixtureDef is only for "request".
                 assert isinstance(fixturedef, FixtureDef)
-                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:
@@ -1061,6 +1061,7 @@ def execute(self, request: SubRequest) -> FixtureValue:
             if my_cache_key is cache_key:
                 if self.cached_result[2] is not None:
                     exc = self.cached_result[2]
+                    # this would previously trigger adding a finalizer. Should it?
                     raise exc
                 else:
                     result = self.cached_result[0]
@@ -1071,7 +1072,15 @@ def execute(self, request: SubRequest) -> 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)
+
+        # note: unused
         return result
 
     def cache_key(self, request: SubRequest) -> object:

testing/python/test_scope_fixture_caching.py

@@ -0,0 +1,65 @@
+from __future__ import annotations
+
+from typing import Generator
+
+import pytest
+
+
+# These tests will fail if run out of order, or selectively ... so they should probably be defined in a different way
+
+executed: list[str] = []
+
+
+#########
+# Make sure setup and finalization is only run once when using fixture multiple times
+# I'm pretty sure there's other tests that checks this already though, though idr fully where
+#########
+@pytest.fixture(scope="class")
+def fixture_1() -> Generator[None, None, None]:
+    executed.append("fix setup")
+    yield
+    executed.append("fix teardown")
+
+
+class TestFixtureCaching:
+    def test_1(self, fixture_1: None) -> None:
+        assert executed == ["fix setup"]
+
+    def test_2(self, fixture_1: None) -> None:
+        assert executed == ["fix setup"]
+
+
+def test_expected_setup_and_teardown() -> None:
+    assert executed == ["fix setup", "fix teardown"]
+
+
+######
+# Make sure setup & finalization is only run once, with a cached exception
+######
+executed_crash: list[str] = []
+
+
+@pytest.fixture(scope="class")
+def fixture_crash(request: pytest.FixtureRequest) -> None:
+    executed_crash.append("fix_crash setup")
+
+    def my_finalizer() -> None:
+        executed_crash.append("fix_crash teardown")
+
+    request.addfinalizer(my_finalizer)
+
+    raise Exception("foo")
+
+
+class TestFixtureCachingException:
+    @pytest.mark.xfail
+    def test_crash_1(self, fixture_crash: None) -> None:
+        assert False
+
+    @pytest.mark.xfail
+    def test_crash_2(self, fixture_crash: None) -> None:
+        assert False
+
+
+def test_crash_expected_setup_and_teardown() -> None:
+    assert executed_crash == ["fix_crash setup", "fix_crash teardown"]

testing/python/test_scope_fixture_teardown_order.py

@@ -0,0 +1,36 @@
+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 = "autouse_setup"
+    yield
+    assert last_executed == "noautouse_teardown"
+    last_executed = "autouse_teardown"
+
+
+@pytest.fixture(scope="module")
+def fixture_2() -> Generator[None, None, None]:
+    global last_executed
+    assert last_executed == "autouse_setup"
+    last_executed = "noautouse_setup"
+    yield
+    assert last_executed == "run_test"
+    last_executed = "noautouse_teardown"
+
+
+def test_autouse_fixture_teardown_order(fixture_1: None, fixture_2: None) -> None:
+    global last_executed
+    assert last_executed == "noautouse_setup"
+    last_executed = "run_test"
+
+
+def test_2(fixture_1: None) -> None:
+    pass