Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save teardown exceptions for further teardowns #13002

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,10 @@ Kevin Hierro Carrasco
Kevin J. Foley
Kian Eliasi
Kian-Meng Ang
Kirill Zhdanov
Kodi B. Arfer
Kojo Idrissa
Konstantin Shkel
Kostis Anagnostopoulos
Kristoffer Nordström
Kyle Altendorf
Expand Down
1 change: 1 addition & 0 deletions changelog/12163.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Teardown fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions`.
Copy link
Member

@nicoddemus nicoddemus Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit confusing as there are no "teardown fixtures", perhaps:

Suggested change
Teardown fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions`.
Fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions` during their own teardowns.

4 changes: 4 additions & 0 deletions src/_pytest/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,10 @@ def __init__(
# Deprecated alias. Was never public. Can be removed in a few releases.
self._store = self.stash

#: A list of exceptions that happened during teardown. Intended for
#: post-teardown inspection, not required internally.
self.teardown_exceptions: list[BaseException] = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a list and not an exception group?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use a list then pass the list to the exception group, you can't append things to a group

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't realize they were immutable..

Copy link

@storenth storenth Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys, where to put tests? Can u suggest appropriate place? Looks like: testing/test_collection.py and teardown?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged, thank you. Now we have a test to check if the fixture fails and exception is added to the list


@classmethod
def from_parent(cls, parent: Node, **kw) -> Self:
"""Public constructor for Nodes.
Expand Down
13 changes: 7 additions & 6 deletions src/_pytest/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -539,19 +539,20 @@
if list(self.stack.keys()) == needed_collectors[: len(self.stack)]:
break
node, (finalizers, _) = self.stack.popitem()
these_exceptions = []
while finalizers:
fin = finalizers.pop()
try:
fin()
except TEST_OUTCOME as e:
these_exceptions.append(e)
node.teardown_exceptions.append(e)

Check warning on line 547 in src/_pytest/runner.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/runner.py#L547

Added line #L547 was not covered by tests

if len(these_exceptions) == 1:
exceptions.extend(these_exceptions)
elif these_exceptions:
if len(node.teardown_exceptions) == 1:
exceptions.extend(node.teardown_exceptions)

Check warning on line 550 in src/_pytest/runner.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/runner.py#L550

Added line #L550 was not covered by tests
elif node.teardown_exceptions:
msg = f"errors while tearing down {node!r}"
exceptions.append(BaseExceptionGroup(msg, these_exceptions[::-1]))
exceptions.append(

Check warning on line 553 in src/_pytest/runner.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/runner.py#L553

Added line #L553 was not covered by tests
BaseExceptionGroup(msg, node.teardown_exceptions[::-1])
)
Comment on lines +549 to +555
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perhaps something for another PR (or too late/not worth changing), but from trio's experience with strict_exception_groups it's generally a bad design philosophy to sometimes return an exception group. This leads users to write code where they assume there's only ever one or no exceptions, and everything breaks when >1 exceptions happen to occur.
Maybe it's less of an issue here, given that we're not raising the exceptions, but I could see this complicating parsing logic nevertheless.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're raising them a few lines later, after all teardowns. However, this is a design change, and probably should be done in another PR


if len(exceptions) == 1:
raise exceptions[0]
Expand Down
27 changes: 27 additions & 0 deletions testing/acceptance_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1584,3 +1584,30 @@ def test_no_terminal_plugin(pytester: Pytester) -> None:
pytester.makepyfile("def test(): assert 1 == 2")
result = pytester.runpytest("-pno:terminal", "-s")
assert result.ret == ExitCode.TESTS_FAILED


def test_get_exception_on_teardown_failure(pytester: Pytester) -> None:
"""Smoke test to be sure teardown exceptions handled properly via node property"""
pytester.makepyfile(
conftest="""
import sys
import pytest
def pytest_exception_interact(node, call, report):
sys.stderr.write("teardown_exceptions: `{}`".format(node.teardown_exceptions))

@pytest.fixture
def mylist():
yield
raise AssertionError(111)
""",
test_file="""
def test_func(mylist):
assert True
""",
)
result = pytester.runpytest()
assert result.ret == ExitCode.TESTS_FAILED
assert "teardown_exceptions: `[AssertionError(111)]`" in result.stderr.str()
# Related to the #9909 - first the test passes, then the teardown fails, what
# results in a double-reporting.
result.assert_outcomes(passed=1, errors=1)
Loading