Account for changes in fixture dependencies properly

changelog/14103.bugfix.rst

@@ -0,0 +1 @@
+Fixtures are now rebuilt when param changes for a fixture they depend on, if the dependency is via ``request.getfixturevalue()``.

changelog/14114.bugfix.rst

@@ -0,0 +1 @@
+An exception from ``pytest_fixture_post_finalizer`` no longer prevents fixtures from being torn down, causing additional errors in the following tests.

changelog/2043.bugfix.rst

@@ -0,0 +1 @@
+If a fixture depends on a fixture that becomes hidden in a test (compared to the previous test), the hidden fixture definition is no longer executed.

changelog/4871.improvement.rst

@@ -0,0 +1,3 @@
+Garbage finalizers for fixture teardown are no longer accumulated in nodes and fixtures.
+
+:func:`Node.addfinalizer <_pytest.nodes.Node.addfinalizer>` and ``request.addfinalizer()`` now return a handle that allows to remove the finalizer.

changelog/5848.improvement.rst

@@ -0,0 +1 @@
+``pytest_fixture_post_finalizer`` is no longer called extra times for the same fixture teardown.

changelog/9287.bugfix.rst

@@ -0,0 +1,47 @@
+Teardown of parametrized fixtures now happens in the teardown stage of the test before the parameter changes.
+
+Previously teardown would happen in the setup stage of the test where the parameter changes.
+
+If a test forces teardown of a parametrized fixture, e.g. using ``request.getfixturevalue()``, it instead fails. An example of such test:
+
+.. code-block:: pytest
+
+    # conftest.py
+    import pytest
+
+    @pytest.hookimpl(wrapper=True, tryfirst=True)
+    def pytest_collection_modifyitems(items):
+        # Disable built-in test reordering.
+        original_items = items[:]
+        yield
+        items[:] = original_items
+
+    # test_invalid.py
+    import pytest
+
+    @pytest.fixture(scope="session")
+    def foo(request):
+        return getattr(request, "param", "default")
+
+    @pytest.mark.parametrize("foo", [1], indirect=True)
+    def test_a(foo):
+        assert foo == 1
+
+    def test_b(request):
+        request.getfixturevalue("foo")
+
+    @pytest.mark.parametrize("foo", [1], indirect=True)
+    def test_c(foo):
+        assert foo == 1
+
+This produces the following error:
+
+.. code-block:: console
+
+    Parameter for the requested fixture changed unexpectedly in test:
+        test_invalid.py::test_b
+    Requested fixture 'foo' defined in:
+        test_invalid.py:4
+
+    Previous parameter value: 1
+    New parameter value: None

src/_pytest/fixtures.py

@@ -69,10 +69,6 @@
 from _pytest.warning_types import PytestWarning
 
 
-if sys.version_info < (3, 11):
-    from exceptiongroup import BaseExceptionGroup
-
-
 if TYPE_CHECKING:
     from _pytest.python import CallSpec2
     from _pytest.python import Function
@@ -389,6 +385,9 @@ def __init__(
         # - In the future we might consider using a generic for the param type, but
         #   for now just using Any.
         self.param: Any
+        # FixtureDefs requested through this specific `request` object.
+        # Allows tracking dependencies on fixtures.
+        self._own_fixture_defs: Final[dict[str, FixtureDef[object]]] = {}
 
     @property
     def _fixturemanager(self) -> FixtureManager:
@@ -483,9 +482,12 @@ def session(self) -> Session:
         return self._pyfuncitem.session
 
     @abc.abstractmethod
-    def addfinalizer(self, finalizer: Callable[[], object]) -> None:
+    def addfinalizer(self, finalizer: Callable[[], object]) -> Callable[[], None]:
         """Add finalizer/teardown function to be called without arguments after
-        the last test within the requesting test context finished execution."""
+        the last test within the requesting test context finished execution.
+
+        :returns: A handle that can be used to remove the finalizer.
+        """
         raise NotImplementedError()
 
     def applymarker(self, marker: str | MarkDecorator) -> None:
@@ -555,6 +557,7 @@ def _get_active_fixturedef(self, argname: str) -> FixtureDef[object]:
         fixturedef = self._fixture_defs.get(argname)
         if fixturedef is not None:
             self._check_scope(fixturedef, fixturedef._scope)
+            self._own_fixture_defs[argname] = fixturedef
             return fixturedef
 
         # Find the appropriate fixturedef.
@@ -592,10 +595,7 @@ def _get_active_fixturedef(self, argname: str) -> FixtureDef[object]:
         fixturedef = fixturedefs[index]
 
         # Prepare a SubRequest object for calling the fixture.
-        try:
-            callspec = self._pyfuncitem.callspec
-        except AttributeError:
-            callspec = None
+        callspec: CallSpec2 | None = getattr(self._pyfuncitem, "callspec", None)
         if callspec is not None and argname in callspec.params:
             param = callspec.params[argname]
             param_index = callspec.indices[argname]
@@ -616,6 +616,7 @@ def _get_active_fixturedef(self, argname: str) -> FixtureDef[object]:
             self, scope, param, param_index, fixturedef, _ispytest=True
         )
 
+        self._own_fixture_defs[argname] = fixturedef
         # Make sure the fixture value is cached, running it if it isn't
         fixturedef.execute(request=subrequest)
 
@@ -687,7 +688,7 @@ def _check_scope(
         pass
 
     @property
-    def node(self):
+    def node(self) -> nodes.Node:
         return self._pyfuncitem
 
     def __repr__(self) -> str:
@@ -699,8 +700,8 @@ def _fillfixtures(self) -> None:
             if argname not in item.funcargs:
                 item.funcargs[argname] = self.getfixturevalue(argname)
 
-    def addfinalizer(self, finalizer: Callable[[], object]) -> None:
-        self.node.addfinalizer(finalizer)
+    def addfinalizer(self, finalizer: Callable[[], object]) -> Callable[[], None]:
+        return self.node.addfinalizer(finalizer)
 
 
 @final
@@ -786,8 +787,8 @@ def _format_fixturedef_line(self, fixturedef: FixtureDef[object]) -> str:
         sig = signature(factory)
         return f"{path}:{lineno + 1}:  def {factory.__name__}{sig}"
 
-    def addfinalizer(self, finalizer: Callable[[], object]) -> None:
-        self._fixturedef.addfinalizer(finalizer)
+    def addfinalizer(self, finalizer: Callable[[], object]) -> Callable[[], None]:
+        return self._fixturedef.addfinalizer(finalizer)
 
 
 @final
@@ -950,6 +951,16 @@ def _eval_scope_callable(
     return result
 
 
+def _get_cached_value(
+    cached_result: _FixtureCachedResult[FixtureValue],
+) -> FixtureValue:
+    if cached_result[2] is not None:
+        exc, exc_tb = cached_result[2]
+        raise exc.with_traceback(exc_tb)
+    else:
+        return cached_result[0]
+
+
 class FixtureDef(Generic[FixtureValue]):
     """A container for a fixture definition.
 
@@ -1014,7 +1025,10 @@ def __init__(
         # If the fixture was executed, the current value of the fixture.
         # Can change if the fixture is executed with different parameters.
         self.cached_result: _FixtureCachedResult[FixtureValue] | None = None
-        self._finalizers: Final[list[Callable[[], object]]] = []
+        # The request object with which the fixture was set up.
+        self._cached_request: SubRequest | None = None
+        # Handles to remove our finalizer from various scopes.
+        self._self_finalizer_handles: Final[list[Callable[[], None]]] = []
 
         # only used to emit a deprecationwarning, can be removed in pytest9
         self._autouse = _autouse
@@ -1024,77 +1038,37 @@ def scope(self) -> _ScopeName:
         """Scope string, one of "function", "class", "module", "package", "session"."""
         return self._scope.value
 
-    def addfinalizer(self, finalizer: Callable[[], object]) -> None:
-        self._finalizers.append(finalizer)
+    def addfinalizer(self, finalizer: Callable[[], object]) -> Callable[[], None]:
+        assert self._cached_request is not None
+        setupstate = self._cached_request.session._setupstate
+        return setupstate.fixture_addfinalizer(finalizer, self)
 
     def finish(self, request: SubRequest) -> None:
-        exceptions: list[BaseException] = []
-        while self._finalizers:
-            fin = self._finalizers.pop()
-            try:
-                fin()
-            except BaseException as e:
-                exceptions.append(e)
-        node = request.node
-        node.ihook.pytest_fixture_post_finalizer(fixturedef=self, request=request)
-        # Even if finalization fails, we invalidate the cached fixture
-        # value and remove all finalizers because they may be bound methods
-        # which will keep instances alive.
-        self.cached_result = None
-        self._finalizers.clear()
-        if len(exceptions) == 1:
-            raise exceptions[0]
-        elif len(exceptions) > 1:
-            msg = f'errors while tearing down fixture "{self.argname}" of {node}'
-            raise BaseExceptionGroup(msg, exceptions[::-1])
+        try:
+            request.session._setupstate.fixture_teardown(self, request.node)
+        finally:
+            self.cached_result = None
+            self._cached_request = None
+            # Avoid accumulating garbage finalizers in nodes and fixturedefs (#4871).
+            for handle in self._self_finalizer_handles:
+                handle()
+            self._self_finalizer_handles.clear()
 
     def execute(self, request: SubRequest) -> FixtureValue:
         """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.
-            requested_fixtures_that_should_finalize_us.append(fixturedef)
-
-        # Check for (and return) cached value/exception.
         if self.cached_result is not None:
-            request_cache_key = self.cache_key(request)
-            cache_key = self.cached_result[1]
-            try:
-                # Attempt to make a normal == check: this might fail for objects
-                # which do not implement the standard comparison (like numpy arrays -- #6497).
-                cache_hit = bool(request_cache_key == cache_key)
-            except (ValueError, RuntimeError):
-                # If the comparison raises, use 'is' as fallback.
-                cache_hit = request_cache_key is cache_key
-
-            if cache_hit:
-                if self.cached_result[2] is not None:
-                    exc, exc_tb = self.cached_result[2]
-                    raise exc.with_traceback(exc_tb)
-                else:
-                    return self.cached_result[0]
-            # We have a previous but differently parametrized fixture instance
-            # so we need to tear it down before creating a new one.
-            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)
+            self._check_cache_hit(request, self.cached_result[1])
+            return _get_cached_value(self.cached_result)
+
+        # Execute fixtures from argnames here to make sure that analytics
+        # in pytest_fixture_setup only handle the body of the current fixture.
+        for argname in self.argnames:
+            request._get_active_fixturedef(argname)
+
+        self._cached_request = request
+        setupstate = request.session._setupstate
+        setupstate.fixture_setup(self)
+        setupstate.fixture_addfinalizer(self._run_post_finalizer, self)
 
         ihook = request.node.ihook
         try:
@@ -1105,10 +1079,84 @@ def execute(self, request: SubRequest) -> FixtureValue:
             )
         finally:
             # Schedule our finalizer, even if the setup failed.
-            request.node.addfinalizer(finalizer)
+            fin = functools.partial(self.finish, request)
+            self._self_finalizer_handles.append(request.node.addfinalizer(fin))
+            for fixturedef in request._own_fixture_defs.values():
+                self._self_finalizer_handles.append(fixturedef.addfinalizer(fin))
 
         return result
 
+    def _run_post_finalizer(self) -> None:
+        request = self._cached_request
+        assert request is not None
+        ihook = request.node.ihook
+        ihook.pytest_fixture_post_finalizer(fixturedef=self, request=request)
+
+    def _is_cache_hit(self, old_cache_key: object, new_cache_key: object) -> bool:
+        try:
+            # Attempt to make a normal == check: this might fail for objects
+            # which do not implement the standard comparison (like numpy arrays -- #6497).
+            cache_hit = bool(new_cache_key == old_cache_key)
+        except (ValueError, RuntimeError):
+            # If the comparison raises, use 'is' as fallback.
+            cache_hit = new_cache_key is old_cache_key
+        return cache_hit
+
+    def _finish_if_param_changed(self, nextitem: nodes.Item) -> None:
+        assert self._cached_request is not None
+        assert self.cached_result is not None
+        old_cache_key = self.cached_result[1]
+
+        callspec: CallSpec2 | None = getattr(nextitem, "callspec", None)
+        if callspec is not None:
+            new_cache_key = callspec.params.get(self.argname, None)
+        else:
+            new_cache_key = None
+
+        if old_cache_key is None and new_cache_key is None:
+            # Shortcut for the most common case.
+            return
+
+        if new_cache_key is None:
+            fixtureinfo: FuncFixtureInfo | None
+            fixtureinfo = getattr(nextitem, "_fixtureinfo", None)
+            if fixtureinfo is None:
+                return
+            fixturedefs = fixtureinfo.name2fixturedefs.get(self.argname, ())
+            if self not in fixturedefs:
+                # Carry the fixture cache over a test that does not request
+                # the (previously) parametrized fixture statically.
+                # This implementation decision has the consequence that requesting
+                # the fixture dynamically is disallowed, see _check_cache_hit.
+                return
+
+        if not self._is_cache_hit(old_cache_key, new_cache_key):
+            self.finish(self._cached_request)
+
+    def _check_cache_hit(self, request: SubRequest, old_cache_key: object) -> None:
+        new_cache_key = self.cache_key(request)
+        if self._is_cache_hit(old_cache_key, new_cache_key):
+            return
+
+        # Finishing the fixture in setup phase in unacceptable (see PR #14104).
+        item = request._pyfuncitem
+        location = getlocation(self.func, item.config.rootpath)
+        msg = (
+            "Parameter for the requested fixture changed unexpectedly in test:\n"
+            f"    {item.nodeid}\n\n"
+            f"Requested fixture '{self.argname}' defined in:\n"
+            f"{location}\n\n"
+            f"Previous parameter value: {old_cache_key!r}\n"
+            f"New parameter value: {new_cache_key!r}\n\n"
+            f"This could happen because the current test requested the previously "
+            "parametrized fixture dynamically via 'getfixturevalue' and did not "
+            "provide a parameter for the fixture.\n"
+            "Either provide a parameter for the fixture, or make fixture "
+            f"'{self.argname}' statically reachable from the current test, "
+            "e.g. by adding it as an argument to the test function."
+        )
+        fail(msg, pytrace=False)
+
     def cache_key(self, request: SubRequest) -> object:
         return getattr(request, "param", None)
 
@@ -1134,8 +1182,8 @@ def __init__(self, request: FixtureRequest) -> None:
         )
         self.cached_result = (request, [0], None)
 
-    def addfinalizer(self, finalizer: Callable[[], object]) -> None:
-        pass
+    def addfinalizer(self, finalizer: Callable[[], object]) -> Callable[[], None]:
+        assert False
 
 
 def resolve_fixture_function(