unraisablehook enhancements #12958

graingert · 2024-11-13T11:52:05Z

A number of unraisablehook plugin enhancements:

set the unraisablehook as early as possible and unset it as late as possible, to collect the most possible unraisable exceptions (please let me know if there's an earlier or later hook I can use)
call the garbage collector just before unsetting the unraisablehook, to collect any straggling exceptions
collect multiple unraisable exceptions per test phase
report the tracemalloc allocation traceback, if available!
avoid using a generator based hook to allow handling StopIteration in test failures
report the unraisable exception as the cause of the PytestUnraisableExceptionWarning exception if raised.
compute the repr of the unraisable.object in the unraisablehook so you get the latest information if available, and should help with resurrection of the object

A lot of these enhancements could also apply to the threading.excepthook plugin and I'll try to do that in a followup

Could help with #10404 it doesn't fix the issue yet because the warnings filter is removed before the session finishes, so any warnings that occur during session/config teardown are not reported. This PR improves the situation because it's now possible to configure warnings as errors by setting the PYTHONWARNINGS environment variable and ensure all unraisable exceptions are collected

It may also be a good idea to add a flag that calls gc.collect() at the end of every test phase to help associate errors with tests - but probably you don't want to run this every time as it impacts performance

graingert · 2024-11-13T12:01:44Z

# test_demo.py
import asyncio
import pytest

async def my_task():
    pass

def test_scheduler_must_be_created_within_running_loop() -> None:
    with pytest.raises(RuntimeError) as _:
        asyncio.create_task(my_task())

This is an improvement it works now if you do: python -W error -m pytest test_demo.py because there's an issue where warnings filters are removed too early

graingert · 2024-11-13T14:36:21Z

These fixes probably need to be applied to the thread hooks

graingert · 2024-11-20T09:29:42Z

src/_pytest/tracemalloc.py

+    try:
+        import tracemalloc
+    except ImportError:
+        return ""


this should be covered on pypy

Leave that comment in the code? More likely for future maintainers to see it that way 🙂

Ah what I mean to say is that the coverage check is failing for this branch, but it shouldn't and I don't know why

testing/test_unraisableexception.py

Zac-HD

Looks great overall! Some comments below, merge when you think it's ready.

Zac-HD · 2024-11-20T18:27:58Z

src/_pytest/tracemalloc.py

+    try:
+        import tracemalloc
+    except ImportError:
+        return ""


Leave that comment in the code? More likely for future maintainers to see it that way 🙂

Zac-HD · 2024-11-20T18:42:06Z

testing/test_unraisableexception.py

+    # TODO: should be a test failure or error
+    assert result.ret == pytest.ExitCode.INTERNAL_ERROR


Do we want to change this now?

Yes, but I don't know how

I haven't looked at the code at all, but maybe set a flag from the unraisable hook, check it where we choose the exit code, and exit-with-error if it's set?

I think I know how to do this, but I need to report the exception group traceback from pytest_session_finish then raise sys.exit(pytest.ExitCode.ERROR), then detect any absolutely straggling unraisables from the config callback and raise that as INTERNALERROR

I'm not sure how to do that though

I don't think we have a way to issue warnings at this late point.

I'm think that perhaps configure/unconfigure is not the best hooks for this, as they're too early/late. The purpose of the plugin is to catch unraisables from user test code, not from pytest internals. So maybe sessionstart/sessionfinish are the more appropriate hooks for setting up & catching stragglers. At this point we still have session which I think we can use to issue the straggler warnings/messages.

bluetech

Thanks, these are nice improvements.

I left some comments.

bluetech · 2024-11-20T19:24:02Z

src/_pytest/unraisableexception.py


+_unraisable_exceptions: collections.deque[UnraisableMeta | BaseException] = (


Can we use the config stash for this instead of a global?

Of course, unraisablehook itself is a global, so it's not like we can really make this plugin safe for multiple concurrent pytest sessions. But still, I prefer to avoid globals when possible.

How do I get to the config from the various test stage hooks?

They can all receive an pytest.Item, so you can access the config via item.config.

I put it in stash

bluetech · 2024-11-20T19:26:24Z

src/_pytest/unraisableexception.py

+        while True:
+            try:
+                meta = _unraisable_exceptions.pop()
+            except IndexError:
+                break


Perhaps like this? It's shorter and makes the stop condition more obvious.

Suggested change

while True:

try:

meta = _unraisable_exceptions.pop()

except IndexError:

break

while _unraisable_exceptions:

meta = _unraisable_exceptions.pop()

The current code only checks if the deque has an item once, and while some_deque: some_deque.pop() is an anti pattern because it's not thread safe (we don't care about it here, but we might as well use the thread safe version)

src/_pytest/unraisableexception.py

bluetech · 2024-11-20T19:47:57Z

src/_pytest/unraisableexception.py

+            )
+        )
+    except BaseException as e:
+        _unraisable_exceptions.append(e)


Can we make this an UnraisableMeta as well, perhaps preserving the original unraisable just without the extra info?

this BaseException handling needs to be as simple as possible - the failures here are the sorts of thing relating to missing globals and things. I've refactored to make this branch only load locals

src/_pytest/unraisableexception.py

testing/test_unraisableexception.py

Co-authored-by: Ran Benita <[email protected]>

graingert · 2024-11-20T20:18:14Z

I just noticed that raising an exception in a callback to Config.add_cleanup doesn't allow subsequent cleanups to run. I'll fix this in another pr

the sorts of errors possible include _unraisable_exceptions being removed from the unraisablehook plugin module dict, so we bind append as a local

nicoddemus

Overall great work, left some minor comments.

changelog/12958.improvement.rst

src/_pytest/unraisableexception.py

nicoddemus · 2024-11-21T20:26:15Z

src/_pytest/unraisableexception.py


+_unraisable_exceptions: collections.deque[UnraisableMeta | BaseException] = (


They can all receive an pytest.Item, so you can access the config via item.config.

src/_pytest/unraisableexception.py

Co-authored-by: Bruno Oliveira <[email protected]>

nicoddemus · 2024-11-25T12:24:51Z

Thanks @graingert!

bluetech · 2024-11-26T15:07:09Z

src/_pytest/unraisableexception.py

+
+
+def gc_collect_harder() -> None:
+    # Constant determined experimentally by the Trio project.


Suggested change

# Constant determined experimentally by the Trio project.

# A single collection doesn't necessarily collect everything.

# Constant determined experimentally by the Trio project.

bluetech · 2024-11-26T15:12:24Z

testing/test_unraisableexception.py

@@ -36,7 +38,8 @@ def test_2(): pass
            "  ",
            "  Traceback (most recent call last):",
            "  ValueError: del is broken",
-            "  ",


I would keep this empty line, I think it's a good delineation.

bluetech · 2024-11-26T15:24:26Z

src/_pytest/unraisableexception.py

+        )
+        tracemalloc_tb = tracemalloc_message(unraisable.object)
+        msg = summary + traceback_message + tracemalloc_tb
+        cause_msg = summary + tracemalloc_tb


Some separator is needed here, e.g.

Suggested change

cause_msg = summary + tracemalloc_tb

cause_msg = summary + "\n" + tracemalloc_tb

although are you sure you want to include tracemalloc_tb here? Maybe it's better to keep this a single line?

bluetech · 2024-11-26T15:36:23Z

testing/test_unraisableexception.py

+    # TODO: should be a test failure or error
+    assert result.ret == pytest.ExitCode.INTERNAL_ERROR


I don't think we have a way to issue warnings at this late point.

I'm think that perhaps configure/unconfigure is not the best hooks for this, as they're too early/late. The purpose of the plugin is to catch unraisables from user test code, not from pytest internals. So maybe sessionstart/sessionfinish are the more appropriate hooks for setting up & catching stragglers. At this point we still have session which I think we can use to issue the straggler warnings/messages.

graingert added 2 commits November 13, 2024 11:51

unraisablehook enhancements

85594a7

only raise errors if there are any

b79f652

graingert added 5 commits November 13, 2024 12:07

cause the traceback to only show up once

194f9ec

include tracemalloc message in unraisablehook failures

c78f84b

fix tests

7f30217

avoid keeping unraisable.object alive while waiting for collection

d2045ce

use the formatted traceback if there's no exc_value

7e7c965

graingert added 3 commits November 19, 2024 10:00

Merge branch 'main' into set-unraisable-hook-per-session

b0f803d

handle failures in unraisable plugin hook

d540ada

refactor tracemalloc message

a285cd5

graingert commented Nov 20, 2024

View reviewed changes

graingert added 6 commits November 20, 2024 09:58

test multiple errors

f853b2c

hide from the branch coverage check

34b3683

test failure in unraisable processing

04705b4

test that errors due to cyclic grabage are collected eventually

4f8aeef

handle errors on <3.10

90aa800

use trylast=True instead of a wrapper

055e673

graingert marked this pull request as ready for review November 20, 2024 10:59

graingert commented Nov 20, 2024

View reviewed changes

testing/test_unraisableexception.py Show resolved Hide resolved

Update testing/test_unraisableexception.py

3d6a023

graingert requested a review from Zac-HD November 20, 2024 11:01

Zac-HD approved these changes Nov 20, 2024

View reviewed changes

bluetech reviewed Nov 20, 2024

View reviewed changes

Update src/_pytest/unraisableexception.py

02ada4b

Co-authored-by: Ran Benita <[email protected]>

graingert mentioned this pull request Nov 20, 2024

raising an exception in a callback to pytest.Config.add_cleanup doesn't allow subsequent cleanups to run. #12981

Closed

add newsfragment

84e7311

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

graingert added 4 commits November 21, 2024 10:39

refactor gc_collect_harder

086c418

cleanup args/kwargs

6a76066

make unraisablehook last-resort exception handling as simple as possible

2cc6648

the sorts of errors possible include _unraisable_exceptions being removed from the unraisablehook plugin module dict, so we bind append as a local

use assert_outcomes

77c5126

nicoddemus reviewed Nov 21, 2024

View reviewed changes

graingert and others added 8 commits November 21, 2024 20:58

Update changelog/12958.improvement.rst

4741611

Co-authored-by: Bruno Oliveira <[email protected]>

Update src/_pytest/unraisableexception.py

a554f2a

Co-authored-by: Bruno Oliveira <[email protected]>

Update src/_pytest/unraisableexception.py

f33fb85

Co-authored-by: Bruno Oliveira <[email protected]>

Update src/_pytest/unraisableexception.py

af13058

Co-authored-by: Bruno Oliveira <[email protected]>

remove private prefix

69d0b22

put unraisable deque in config.stash

b0978e0

cleanup stash when finished

740bafb

Merge branch 'main' into set-unraisable-hook-per-session

b6e50ff

nicoddemus approved these changes Nov 25, 2024

View reviewed changes

Merge branch 'main' into set-unraisable-hook-per-session

b436d1c

bluetech reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unraisablehook enhancements #12958

unraisablehook enhancements #12958

graingert commented Nov 13, 2024 •

edited

Loading

graingert commented Nov 13, 2024 •

edited

Loading

graingert commented Nov 13, 2024

graingert Nov 20, 2024

Zac-HD Nov 20, 2024

graingert Nov 20, 2024

Zac-HD left a comment

Zac-HD Nov 20, 2024

Zac-HD Nov 20, 2024

graingert Nov 20, 2024

Zac-HD Nov 20, 2024

graingert Nov 22, 2024

bluetech Nov 26, 2024

bluetech left a comment

bluetech Nov 20, 2024

graingert Nov 20, 2024

nicoddemus Nov 21, 2024

graingert Nov 22, 2024

bluetech Nov 20, 2024

graingert Nov 20, 2024

bluetech Nov 20, 2024

graingert Nov 21, 2024

graingert commented Nov 20, 2024

nicoddemus left a comment

nicoddemus Nov 21, 2024

nicoddemus commented Nov 25, 2024

bluetech Nov 26, 2024

bluetech Nov 26, 2024

bluetech Nov 26, 2024

bluetech Nov 26, 2024

		# TODO: should be a test failure or error
		assert result.ret == pytest.ExitCode.INTERNAL_ERROR


		_unraisable_exceptions: collections.deque[UnraisableMeta \| BaseException] = (



		def gc_collect_harder() -> None:
		# Constant determined experimentally by the Trio project.

	# Constant determined experimentally by the Trio project.
	# A single collection doesn't necessarily collect everything.
	# Constant determined experimentally by the Trio project.

	cause_msg = summary + tracemalloc_tb
	cause_msg = summary + "\n" + tracemalloc_tb

unraisablehook enhancements #12958

Are you sure you want to change the base?

unraisablehook enhancements #12958

Conversation

graingert commented Nov 13, 2024 • edited Loading

graingert commented Nov 13, 2024 • edited Loading

graingert commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD 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

bluetech 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

graingert commented Nov 20, 2024

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Nov 25, 2024

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

graingert commented Nov 13, 2024 •

edited

Loading

graingert commented Nov 13, 2024 •

edited

Loading