Skip to content

Commit

Permalink
fixtures: use exception group when multiple finalizers raise in fixtu…
Browse files Browse the repository at this point in the history
…re teardown

Previously, if more than one fixture finalizer raised, only the first
was reported, and the other errors were lost.

Use an exception group to report them all. This is similar to the change
we made in node teardowns (in `SetupState`).
  • Loading branch information
bluetech committed Mar 2, 2024
1 parent 6ed0051 commit 434282e
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 24 deletions.
2 changes: 2 additions & 0 deletions changelog/12047.improvement.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
When multiple finalizers of a fixture raise an exception, now all exceptions are reported as an exception group.
Previously, only the first exception was reported.
45 changes: 24 additions & 21 deletions src/_pytest/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import inspect
import os
from pathlib import Path
import sys
from typing import AbstractSet
from typing import Any
from typing import Callable
Expand Down Expand Up @@ -67,6 +68,10 @@
from _pytest.scope import Scope


if sys.version_info[:2] < (3, 11):
from exceptiongroup import BaseExceptionGroup


if TYPE_CHECKING:
from typing import Deque

Expand Down Expand Up @@ -1017,27 +1022,25 @@ def addfinalizer(self, finalizer: Callable[[], object]) -> None:
self._finalizers.append(finalizer)

def finish(self, request: SubRequest) -> None:
exc = None
try:
while self._finalizers:
try:
func = self._finalizers.pop()
func()
except BaseException as e:
# XXX Only first exception will be seen by user,
# ideally all should be reported.
if exc is None:
exc = e
if exc:
raise exc
finally:
ihook = request.node.ihook
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()
exceptions: List[BaseException] = []
while self._finalizers:
fin = self._finalizers.pop()
try:
fin()
except BaseException as e:
exceptions.append(e)

Check warning on line 1031 in src/_pytest/fixtures.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/fixtures.py#L1030-L1031

Added lines #L1030 - L1031 were not covered by tests
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]

Check warning on line 1040 in src/_pytest/fixtures.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/fixtures.py#L1040

Added line #L1040 was not covered by tests
elif len(exceptions) > 1:
msg = f'errors while tearing down fixture "{self.argname}" of {node}'
raise BaseExceptionGroup(msg, exceptions[::-1])

Check warning on line 1043 in src/_pytest/fixtures.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/fixtures.py#L1042-L1043

Added lines #L1042 - L1043 were not covered by tests

def execute(self, request: SubRequest) -> FixtureValue:
# Get required arguments and register our own finish()
Expand Down
15 changes: 12 additions & 3 deletions testing/python/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -932,8 +932,9 @@ def test_request_subrequest_addfinalizer_exceptions(
self, pytester: Pytester
) -> None:
"""
Ensure exceptions raised during teardown by a finalizer are suppressed
until all finalizers are called, re-raising the first exception (#2440)
Ensure exceptions raised during teardown by finalizers are suppressed
until all finalizers are called, then re-reaised together in an
exception group (#2440)
"""
pytester.makepyfile(
"""
Expand All @@ -960,8 +961,16 @@ def test_second():
"""
)
result = pytester.runpytest()
result.assert_outcomes(passed=2, errors=1)
result.stdout.fnmatch_lines(
["*Exception: Error in excepts fixture", "* 2 passed, 1 error in *"]
[
' | *ExceptionGroup: errors while tearing down fixture "subrequest" of <Function test_first> (2 sub-exceptions)', # noqa: E501
" +-+---------------- 1 ----------------",
" | Exception: Error in something fixture",
" +---------------- 2 ----------------",
" | Exception: Error in excepts fixture",
" +------------------------------------",
],
)

def test_request_getmodulepath(self, pytester: Pytester) -> None:
Expand Down

0 comments on commit 434282e

Please sign in to comment.