Save teardown exceptions for further teardowns #13002

Myp3a · 2024-11-27T12:15:26Z

This PR adds an ability to access the exceptions that happened during teardown in other teardowns.

Motivation: some plugins (playwright-pytest) must rely on current teardown status to properly execute their hooks. Currently the teardown information isn't available, so it must be extracted with different workarounds.

Doesn't change functionality, only exposes the local variable on Node object.

Allows proper implementation of microsoft/playwright-pytest#207, not relying on traceback hacks but getting the information directly.

storenth

LGTM. Possible resolved: #9909 and #12163

webknjaz

FWIW such changes must be always accompanied by respective tests.

webknjaz · 2024-11-27T15:16:31Z

src/_pytest/nodes.py

@@ -214,6 +214,9 @@ 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
+        self.teardown_exceptions: list[BaseException] = []


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

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

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

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

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

…perty

test: tests for teardown exceptions patch

for more information, see https://pre-commit.ci

storenth · 2024-12-05T11:45:28Z

Looks like we need reviewers: @nicoddemus @daara-s @graingert @Zac-HD @bluetech @jakkdl could you please review this improvement? thanks)

daara-s

LGTM - just a small change in the test and needs a changelog (see contributing docs)

daara-s · 2024-12-05T12:09:52Z

testing/acceptance_test.py

+        def pytest_exception_interact(node, call, report):
+            sys.stderr.write("{}".format(node.teardown_exceptions))
+
+        import pytest


minor - importing pytest twice in this conftest block

jakkdl

seems fine, just a couple small things.

jakkdl · 2024-12-05T12:08:00Z

src/_pytest/runner.py

+            if len(node.teardown_exceptions) == 1:
+                exceptions.extend(node.teardown_exceptions)
+            elif node.teardown_exceptions:
                msg = f"errors while tearing down {node!r}"
-                exceptions.append(BaseExceptionGroup(msg, these_exceptions[::-1]))
+                exceptions.append(
+                    BaseExceptionGroup(msg, node.teardown_exceptions[::-1])
+                )


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.

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

jakkdl · 2024-12-05T12:12:05Z

src/_pytest/runner.py

-            these_exceptions = []
+            node.teardown_exceptions = []


does this need to reinitialize the list? Shouldn't that already be done in Node.__init__?

As there's no other code interacting with this list, you are correct - this is not really needed

jakkdl · 2024-12-05T12:14:57Z

testing/acceptance_test.py

+        def pytest_exception_interact(node, call, report):
+            sys.stderr.write("{}".format(node.teardown_exceptions))
+
+        import pytest
+        @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 "AssertionError(111)" in result.stderr.str()
+    result.stdout.fnmatch_lines(["*1 error*"])


this looks a bit unreliable to me, assert "AssertionError(111)" in result.stderr.str() could plausibly pass for other reasons than pytest_exception_interact printing it. Printing some marker characters surrounding it should suffice though, e.g. assert "node.teardown_exceptions: `AssertionError(111)`" in result.stderr.str().

It's also better to use result.assert_outcomes(errors=1) than fnmatch'ing lines

I've put this stuff because there is unresolved one #9909
btw @jakkdl please, share you mind on that: a8e0aba#diff-c2ee4e81db455299781df03980cd131ee4680fa5b6e5e484f3355dbeef8452c6R1611

ah, should add a comment referring to #9909 for why there's both a pass and a fail to the test.
I don't think I have anything to add to the discussion in #9909 though

Nothing to comment, the PR doesn't touch the pytest architecture.

jakkdl · 2024-12-05T14:47:50Z

oh actually, probably nice to add to the comment that it's only saved for the sake of external post-teardown inspection; and not required internally. If there's any docs for the Node type (that's not just autogenerated) they might also need updating

storenth · 2024-12-08T16:32:25Z

Guys, are we waiting for something?) Looks like we did all stuff from reviewers

jakkdl · 2024-12-09T11:21:34Z

Guys, are we waiting for something?) Looks like we did all stuff from reviewers

it's usually good habit to wait a couple days to see if anybody else wants to review, though this is minor enough that I feel comfortable merging it soon

nicoddemus

Overall looks good, I wonder though if keeping those exceptions around might cause more memory to be used than expected?

Perhaps not given those are only really related to exceptions during teardown...

An alternative for this, in case this is a problem, would be to implement a new hook, and call that hook passing the node and exception (or exceptions?), instead of storing them in the Node.

I'm not sure if this is needed, just food for thought in case storing the exceptions is something we should be more careful about.

nicoddemus · 2024-12-09T17:47:38Z

changelog/12163.feature.rst

@@ -0,0 +1 @@
+Teardown fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions`.


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.

graingert · 2024-12-09T17:52:00Z

I wonder though if keeping those exceptions around might cause more memory to be used than expected?

Keeping the exceptions alive keeps the tracebacks alive which keeps all the locals in all the frames alive

nicoddemus · 2024-12-09T17:59:02Z

Keeping the exceptions alive keeps the tracebacks alive which keeps all the locals in all the frames alive

Indeed, that's why I'm bringing this up.

Initially I was thinking this might not be a big problem, but actually this might really be catastrophic in case you have a fixture which is used a lot (say autouse), which then would fail for every test onward and store a lot of data in memory, possibly causing the machine to run out of memory and crashing, instead of being able to report the problem...

To be safe it is probably best to introduce a new hook, which would circumvent the problem and also allow plugins to handle the exceptions as they appear (possibly handling them immediately or storing them for later, depending on their need).

storenth · 2024-12-15T12:18:47Z

To be safe it is probably best to introduce a new hook...

nicoddemus agree, but, what if we rewrite self.teardown_exceptions to use it as iterator? But, if no chance, do you prefer we refactor to handle hook based on this PR or create another?

nicoddemus · 2024-12-15T12:52:05Z

what if we rewrite self.teardown_exceptions to use it as iterator?

What do you mean? Doesn't that require storing the exceptions anyway, even if we deliver those later as an iterator?

But, if no chance, do you prefer we refactor to handle hook based on this PR or create another?

Might be better to create a new one, and close this one but keeping it on the repository for historical reasons.

feat: save teardown exceptions for further teardowns

1a892d0

storenth approved these changes Nov 27, 2024

View reviewed changes

webknjaz reviewed Nov 27, 2024

View reviewed changes

storenth and others added 3 commits November 28, 2024 14:54

test: teardown exception handled using node teardown_exceptions pro…

11b5de4

…perty

Merge pull request #1 from storenth/patch-1

1535fa2

test: tests for teardown exceptions patch

[pre-commit.ci] auto fixes from pre-commit.com hooks

9abac4d

for more information, see https://pre-commit.ci

daara-s suggested changes Dec 5, 2024

View reviewed changes

jakkdl requested changes Dec 5, 2024

View reviewed changes

Myp3a added 3 commits December 5, 2024 16:07

fix: remove double initialization

ae683a9

test: assert fixes

a8e0aba

chore: changelog entry

d5a146f

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Dec 5, 2024

jakkdl approved these changes Dec 5, 2024

View reviewed changes

chore: explanations

067d8fa

storenth approved these changes Dec 8, 2024

View reviewed changes

nicoddemus reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save teardown exceptions for further teardowns #13002

Save teardown exceptions for further teardowns #13002

Myp3a commented Nov 27, 2024

storenth left a comment •

edited

Loading

webknjaz left a comment

webknjaz Nov 27, 2024

graingert Nov 27, 2024

webknjaz Nov 27, 2024

storenth Nov 27, 2024 •

edited

Loading

storenth Nov 28, 2024

Myp3a Nov 28, 2024

storenth commented Dec 5, 2024

daara-s left a comment •

edited

Loading

daara-s Dec 5, 2024

jakkdl left a comment

jakkdl Dec 5, 2024

Myp3a Dec 5, 2024

jakkdl Dec 5, 2024

Myp3a Dec 5, 2024

jakkdl Dec 5, 2024

storenth Dec 5, 2024

jakkdl Dec 5, 2024

storenth Dec 6, 2024

jakkdl commented Dec 5, 2024

storenth commented Dec 8, 2024

jakkdl commented Dec 9, 2024

nicoddemus left a comment

nicoddemus Dec 9, 2024 •

edited

Loading

graingert commented Dec 9, 2024 •

edited

Loading

nicoddemus commented Dec 9, 2024 •

edited

Loading

storenth commented Dec 15, 2024 •

edited

Loading

nicoddemus commented Dec 15, 2024

		@@ -0,0 +1 @@
		Teardown fixtures now can access the information about current teardown exceptions in `node.teardown_exceptions`.

	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.

Save teardown exceptions for further teardowns #13002

Are you sure you want to change the base?

Save teardown exceptions for further teardowns #13002

Conversation

Myp3a commented Nov 27, 2024

storenth left a comment • edited Loading

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storenth Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storenth commented Dec 5, 2024

daara-s left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Dec 5, 2024

storenth commented Dec 8, 2024

jakkdl commented Dec 9, 2024

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

graingert commented Dec 9, 2024 • edited Loading

nicoddemus commented Dec 9, 2024 • edited Loading

storenth commented Dec 15, 2024 • edited Loading

nicoddemus commented Dec 15, 2024

storenth left a comment •

edited

Loading

storenth Nov 27, 2024 •

edited

Loading

daara-s left a comment •

edited

Loading

nicoddemus Dec 9, 2024 •

edited

Loading

graingert commented Dec 9, 2024 •

edited

Loading

nicoddemus commented Dec 9, 2024 •

edited

Loading

storenth commented Dec 15, 2024 •

edited

Loading