Pass teardown exception to pytest_fixture_post_finalizer explicitly

AUTHORS

@@ -279,6 +279,7 @@ Kim Soo
 Kodi B. Arfer
 Kojo Idrissa
 Kostis Anagnostopoulos
+Krishan Subudhi
 Kristoffer Nordström
 Kyle Altendorf
 Lawrence Mitchell

changelog/12306.feature.rst

@@ -0,0 +1,3 @@
+:hook:`pytest_fixture_post_finalizer` now receives a ``teardown_exception`` argument with the exception raised while tearing down the fixture: the exception itself, a :class:`BaseExceptionGroup` if several finalizers of the fixture failed, or ``None`` if teardown succeeded. Use it instead of :func:`sys.exc_info`, which has not reliably reflected teardown errors inside this hook since pytest 8.1.
+
+The hook now also always receives the request that set the fixture up, and exceptions raised by implementations of the hook propagate instead of being reported as fixture teardown errors.

changelog/14800.bugfix.rst

@@ -0,0 +1 @@
+Fixed an internal ``AssertionError`` when a :hook:`pytest_fixture_setup` implementation raises before the fixture result is cached: the remaining parameters of the same fixture now run normally, and :hook:`pytest_fixture_post_finalizer` is still called for the failed setup. This was a regression in pytest 9.1.0.

src/_pytest/fixtures.py

@@ -1186,6 +1186,9 @@ def __init__(
         # 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 the fixture is currently set up with, if any; passed to
+        # pytest_fixture_post_finalizer on teardown.
+        self._setup_request: SubRequest | None = None
 
         # only used to emit a deprecationwarning, can be removed in pytest9
         self._autouse = _autouse
@@ -1204,10 +1207,14 @@ def addfinalizer(self, finalizer: Callable[[], object]) -> None:
         self._finalizers.append(finalizer)
 
     def finish(self, request: SubRequest) -> None:
-        if self.cached_result is None:
-            # Already finished. It is assumed that finalizers cannot be added in
-            # this state.
+        if self.cached_result is None and self._setup_request is None:
+            # Already finished (or never set up). It is assumed that finalizers
+            # cannot be added in this state.
             return
+        # The hook must see the request that set the fixture up, not the one
+        # that may be tearing it down to switch to a new param (#12306).
+        request = self._setup_request or request
+        self._setup_request = None
 
         exceptions: list[BaseException] = []
         while self._finalizers:
@@ -1217,16 +1224,26 @@ def finish(self, request: SubRequest) -> None:
             except BaseException as e:
                 exceptions.append(e)
         node = request.node
-        # 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()
+        teardown_exception: BaseException | None = None
         if len(exceptions) == 1:
-            raise exceptions[0]
+            teardown_exception = exceptions[0]
         elif len(exceptions) > 1:
             msg = f'errors while tearing down fixture "{self.argname}" of {node}'
-            raise BaseExceptionGroup(msg, exceptions[::-1])
+            teardown_exception = BaseExceptionGroup(msg, exceptions[::-1])
+        try:
+            # An exception raised by the hook itself is a plugin bug and
+            # propagates as-is.
+            node.ihook.pytest_fixture_post_finalizer(
+                fixturedef=self, request=request, teardown_exception=teardown_exception
+            )
+        finally:
+            # 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 teardown_exception is not None:
+            raise teardown_exception
 
     def execute(self, request: SubRequest) -> FixtureValue:
         """Return the value of this fixture, executing it if not cached."""
@@ -1264,10 +1281,11 @@ def execute(self, request: SubRequest) -> FixtureValue:
                     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
+        # We may have a previous but differently parametrized fixture instance,
+        # or a setup that failed before caching a result; tear it down before
+        # creating a new one (no-op if there is nothing to tear down).
+        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
@@ -1276,16 +1294,8 @@ def execute(self, request: SubRequest) -> FixtureValue:
         for parent_fixture in requested_fixtures_that_should_finalize_us:
             parent_fixture.addfinalizer(finalizer)
 
-        # Register the pytest_fixture_post_finalizer as the first finalizer,
-        # which is executed last.
-        assert not self._finalizers
-        self.addfinalizer(
-            lambda: request.node.ihook.pytest_fixture_post_finalizer(
-                fixturedef=self, request=request
-            )
-        )
-
         ihook = request.node.ihook
+        self._setup_request = request
         try:
             # Setup the fixture, run the code in it, and cache the value
             # in self.cached_result.

src/_pytest/hookspec.py

@@ -864,16 +864,33 @@ def pytest_fixture_setup(
 
 
 def pytest_fixture_post_finalizer(
-    fixturedef: FixtureDef[Any], request: SubRequest
+    fixturedef: FixtureDef[Any],
+    request: SubRequest,
+    teardown_exception: BaseException | None,
 ) -> None:
     """Called after fixture teardown, but before the cache is cleared, so
-    the fixture result ``fixturedef.cached_result`` is still available (not
-    ``None``).
+    the fixture result ``fixturedef.cached_result`` is still available.
+
+    Also called when the fixture setup failed before a result was cached
+    (for example, a :hook:`pytest_fixture_setup` implementation raised), in
+    which case ``fixturedef.cached_result`` is ``None``.
+
+    An exception raised by an implementation of this hook is a bug in that
+    implementation: it is not swallowed or merged into
+    ``teardown_exception``, but propagates immediately.
 
     :param fixturedef:
         The fixture definition object.
     :param request:
-        The fixture request object.
+        The fixture request object that set up the fixture.
+    :param teardown_exception:
+        The exception raised while tearing down the fixture, or ``None`` if
+        teardown succeeded. If several finalizers of the fixture failed, this
+        is a :class:`BaseExceptionGroup` holding all of their exceptions.
+        Use this argument rather than :func:`sys.exc_info` to inspect
+        teardown failures.
+
+        .. versionadded:: 9.2
 
     Use in conftest plugins
     =======================

testing/python/fixtures.py

@@ -4585,7 +4585,7 @@ def test_second(foo, bar, baz):
 
 
 def test_fixture_post_finalizer_hook_exception(pytester: Pytester) -> None:
-    """Test that exceptions in pytest_fixture_post_finalizer hook are caught.
+    """Test that exceptions in pytest_fixture_post_finalizer hook propagate.
 
     Also verifies that the fixture cache is properly reset even when the
     post_finalizer hook raises an exception, so the fixture can be rebuilt
@@ -4638,6 +4638,128 @@ def test_second(my_fixture):
     )
 
 
+def test_fixture_post_finalizer_teardown_exception(pytester: Pytester) -> None:
+    """The teardown error is passed to the hook explicitly (#12306)."""
+    pytester.makeconftest(
+        """
+        import pytest
+
+        def pytest_fixture_post_finalizer(fixturedef, request, teardown_exception):
+            if fixturedef.argname in ("failing", "failing_twice", "passing"):
+                print(f"\\n{fixturedef.argname}: {teardown_exception!r}")
+
+        @pytest.fixture
+        def failing():
+            yield
+            raise RuntimeError("teardown failed")
+
+        @pytest.fixture
+        def failing_twice(request):
+            request.addfinalizer(lambda: 1 / 0)
+            yield
+            raise RuntimeError("teardown failed")
+
+        @pytest.fixture
+        def passing():
+            yield
+        """
+    )
+    pytester.makepyfile(
+        """
+        def test_failing(failing):
+            pass
+
+        def test_failing_twice(failing_twice):
+            pass
+
+        def test_passing(passing):
+            pass
+        """
+    )
+    result = pytester.runpytest("-s")
+    result.assert_outcomes(passed=3, errors=2)
+    result.stdout.fnmatch_lines(
+        [
+            "failing: RuntimeError('teardown failed')",
+            "*failing_twice: ExceptionGroup('errors while tearing down fixture "
+            "\"failing_twice\"*ZeroDivisionError('division by zero'), "
+            "RuntimeError('teardown failed')*",
+            "*passing: None",
+        ]
+    )
+
+
+def test_fixture_post_finalizer_gets_setup_request_on_param_switch(
+    pytester: Pytester,
+) -> None:
+    """When a parametrized fixture is torn down because the next test needs a
+    different param, the hook gets the request that set it up (#12306)."""
+    pytester.makeconftest(
+        """
+        def pytest_fixture_post_finalizer(fixturedef, request):
+            if fixturedef.argname == "fix":
+                print(f"\\npost_finalizer: {fixturedef.cached_result[0]} {request.param}")
+        """
+    )
+    pytester.makepyfile(
+        """
+        import pytest
+
+        @pytest.fixture(scope="module", params=[1, 2])
+        def fix(request):
+            return request.param
+
+        def test_it(fix):
+            pass
+        """
+    )
+    result = pytester.runpytest("-s")
+    result.assert_outcomes(passed=2)
+    result.stdout.fnmatch_lines(["post_finalizer: 1 1", "*post_finalizer: 2 2"])
+
+
+def test_fixture_post_finalizer_after_setup_hook_failure(pytester: Pytester) -> None:
+    """The hook runs once for a setup that failed before a result was cached,
+    and the following params of the fixture still run (#14800)."""
+    pytester.makeconftest(
+        """
+        import pytest
+
+        calls = []
+
+        @pytest.hookimpl(tryfirst=True)
+        def pytest_fixture_setup(fixturedef, request):
+            if getattr(request, "param", None) == "fail":
+                raise RuntimeError("setup hook failed")
+
+        def pytest_fixture_post_finalizer(fixturedef, request):
+            if fixturedef.argname == "val":
+                calls.append((request.node.name, fixturedef.cached_result))
+
+        def pytest_terminal_summary(terminalreporter):
+            terminalreporter.write_line(f"post_finalizer calls: {calls}")
+        """
+    )
+    pytester.makepyfile(
+        """
+        import pytest
+
+        @pytest.mark.parametrize("val", ["fail", "ok"])
+        def test_thing(val):
+            assert val == "ok"
+        """
+    )
+    result = pytester.runpytest()
+    result.assert_outcomes(passed=1, errors=1)
+    result.stdout.no_fnmatch_line("*AssertionError*")
+    result.stdout.fnmatch_lines(
+        [
+            "post_finalizer calls: [('test_thing[fail]', None), "
+            "('test_thing[ok]', ('ok', 'ok', None))]"
+        ]
+    )
+
+
 class TestParamValueKey:
     """Unit tests for the equivalence key used by `reorder_items` (#8914)."""