From baed905c0b1a8381bd97167a691096e206f1a168 Mon Sep 17 00:00:00 2001
From: jakkdl <h6+github@pm.me>
Date: Wed, 17 Jan 2024 15:44:38 +0100
Subject: [PATCH] Move scheduling of fixture finalization so it isn't
 rescheduled if the fixture value is cached.

---
 src/_pytest/fixtures.py                       | 29 ++++++++++------
 .../test_scope_fixture_teardown_order.py      | 33 +++++++++++++++++++
 2 files changed, 52 insertions(+), 10 deletions(-)
 create mode 100644 testing/python/test_scope_fixture_teardown_order.py

diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py
index c294ec586..5edb7c4a3 100644
--- a/src/_pytest/fixtures.py
+++ b/src/_pytest/fixtures.py
@@ -538,6 +538,8 @@ class FixtureRequest(abc.ABC):
         :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.  '
@@ -574,9 +576,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
-        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)
@@ -641,11 +642,8 @@ class FixtureRequest(abc.ABC):
 
         # 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"
@@ -1055,7 +1053,9 @@ class FixtureDef(Generic[FixtureValue]):
             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:
@@ -1063,7 +1063,7 @@ class FixtureDef(Generic[FixtureValue]):
             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:
@@ -1073,6 +1073,7 @@ class FixtureDef(Generic[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]
@@ -1083,7 +1084,15 @@ 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)
+
+        # note: unused
         return result
 
     def cache_key(self, request: SubRequest) -> object:
diff --git a/testing/python/test_scope_fixture_teardown_order.py b/testing/python/test_scope_fixture_teardown_order.py
new file mode 100644
index 000000000..fdff19af2
--- /dev/null
+++ b/testing/python/test_scope_fixture_teardown_order.py
@@ -0,0 +1,33 @@
+import pytest
+
+last_executed = ""
+
+
+@pytest.fixture(scope="module")
+def fixture_1():
+    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():
+    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, fixture_2):
+    global last_executed
+    assert last_executed == "noautouse_setup"
+    last_executed = "run_test"
+
+
+def test_2(fixture_1):
+    pass