threadexception enhancements #13016

graingert · 2024-12-01T07:30:17Z

A number of threadexception enhancements:

Set the excepthook as early as possible and unset it as late as possible, to collect the most possible number of unhandled exceptions from threads.
join threads for 1 second just before unsetting the excepthook, to collect any straggling exceptions
Collect multiple thread 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 thread exception as the cause of the pytest.PytestUnhandledThreadExceptionWarning exception if raised.
Extract the name of the thread object in the excepthook which should help with resurrection of the thread.

graingert · 2024-12-01T08:29:53Z

src/_pytest/threadexception.py

+    # threads. However there might be threads that depend on some shutdown
+    # signal that happens after pytest finishes, so we want to limit the
+    # join time somewhat. A one second timeout seems reasonable.
+    timeout = 1


this is the bit of the PR I'm most unsure about. I think maybe we want to raise a deprecation warning for straggling threads - and make it into a error in pytest 9?

Yeah, that seems reasonable to me.

I think it is a reasonable thing for someone to run some thread unrelated to pytest, like a monitoring thread or similar (there are many which do this), and then run pytest.main(). If I understand correctly, this would then cause an unnecessary 1s delay to the pytest run. Best case, they report a bug, worst case they just think pytest is slow and suffer through it.

So IMO, unless we can somehow restrict join_threads only to threads started by tests, we shouldn't do it.

removed in #13027

changelog/13016.improvement.rst

testing/test_threadexception.py

changelog/13016.improvement.rst

Zac-HD

Looks like a clear improvement to me! Merge when comments are addressed to your satisfaction 👍

Zac-HD · 2024-12-04T04:41:42Z

src/_pytest/threadexception.py

+    # threads. However there might be threads that depend on some shutdown
+    # signal that happens after pytest finishes, so we want to limit the
+    # join time somewhat. A one second timeout seems reasonable.
+    timeout = 1


Yeah, that seems reasonable to me.

Zac-HD · 2024-12-04T04:44:10Z

src/_pytest/threadexception.py

+
+        append(
+            ThreadExceptionMeta(
+                # Compute these strings here as they might change later


stale/misplaced comment?

I think this is valid, I meant compute the strings in the hook rather than later on in the collect-from-deque phase

I've moved the comment to the correct place. And I've gone through and checked the comments match the unraisablehook (or diverge where changes are needed)

Note the first commit in this PR was generated by claude - given the unraisablehook patch and the existing threadexception.py. It did well on the code but not so well on the comments!

Zac-HD · 2024-12-04T04:48:10Z

testing/test_threadexception.py

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


Should we wait for this? Or open an issue to track it?

I think open an issue to track this, the unraisablehook code has the same issue

graingert · 2024-12-04T04:53:34Z

Hmm the coverage is looking low, I'll look into it

graingert · 2024-12-04T08:48:29Z

src/_pytest/threadexception.py

+        if len(errors) == 1:
+            raise errors[0]
+        if errors:
+            raise ExceptionGroup("multiple thread exception warnings", errors)


this is absolutely definitely covered by tests:

pytest/testing/test_threadexception.py

Lines 136 to 162 in 7a56153

@pytest.mark.filterwarnings("error::pytest.PytestUnhandledThreadExceptionWarning")

def test_threadexception_warning_multiple_errors(pytester: Pytester) -> None:

pytester.makepyfile(

test_it="""

import threading

def test_it():

def oops():

raise ValueError("Oops")

t = threading.Thread(target=oops, name="MyThread")

t.start()

t.join()

t = threading.Thread(target=oops, name="MyThread2")

t.start()

t.join()

def test_2(): pass

"""

)

result = pytester.runpytest()

assert result.ret == pytest.ExitCode.TESTS_FAILED

result.assert_outcomes(passed=1, failed=1)

result.stdout.fnmatch_lines(

[" | *ExceptionGroup: multiple thread exception warnings (2 sub-exceptions)"]

)

what's going on?

resolved, yeah? Looks like 100% coverage wherever I look

Oh, in the review window codecov is being dumb. idk what exactly triggers it but the codecov app/bot/thing is fairly often behind (not just in this repo), and the CI run is the best place to go look for coverage status.

CI refers to https://github.com/pytest-dev/pytest/pull/13016/checks?check_run_id=33901516563, but the "Files Changed" codecov alerts refers to https://github.com/pytest-dev/pytest/pull/13016/checks?check_run_id=33900516688 - which while on the same commit it displays an orange "refresh" button that will redirect to 33901516563. Not sure how those runs differ

Hmm, I think this is due to
as seen on https://docs.codecov.com/docs/github-checks

Which I think means we (and every project) needs to disable annotations

opened codecov/codecov-action#1710 requesting the default be changed, but will otherwise open PR's/edit the org-level yaml to disable them.

src/_pytest/threadexception.py

graingert · 2024-12-04T09:23:34Z

I've done a bit more than just resolve your comments, @Zac-HD, so I'd like a re-review - if that's ok

Zac-HD

Reviewed further changes, confirming that I'm happy to merge 🙂

As per review comment: #13016 (comment). Closes #13028. --------- Co-authored-by: Bruno Oliveira <[email protected]>

graingert added 2 commits December 1, 2024 07:29

threadexception enhancements

e44969c

add changelog

2615916

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

graingert added 4 commits December 1, 2024 08:16

join straggling threads for 1 second before removing excepthook

c24c03e

add tests

f1195c3

use assert_outcomes

ee5cb32

Merge branch 'main' into threadexception-enhancements

d2377b5

graingert marked this pull request as ready for review December 1, 2024 08:24

Apply suggestions from code review

ec782db

graingert commented Dec 1, 2024

View reviewed changes

changelog/13016.improvement.rst Outdated Show resolved Hide resolved

Update changelog/13016.improvement.rst

bf770cf

graingert commented Dec 1, 2024

View reviewed changes

testing/test_threadexception.py Outdated Show resolved Hide resolved

Update testing/test_threadexception.py

82f60d6

graingert requested review from bluetech and nicoddemus December 1, 2024 11:04

Merge branch 'main' into threadexception-enhancements

4efa96a

graingert commented Dec 1, 2024

View reviewed changes

changelog/13016.improvement.rst Outdated Show resolved Hide resolved

graingert added 2 commits December 1, 2024 19:45

Apply suggestions from code review

9e26040

Merge branch 'main' into threadexception-enhancements

faf769c

graingert mentioned this pull request Dec 3, 2024

unraisablehook enhancements #12958

Merged

graingert requested a review from Zac-HD December 3, 2024 07:16

Zac-HD reviewed Dec 4, 2024

View reviewed changes

Zac-HD approved these changes Dec 4, 2024

View reviewed changes

align comments between threadexception and unraisableexception plugins

7a56153

graingert commented Dec 4, 2024

View reviewed changes

src/_pytest/threadexception.py Outdated Show resolved Hide resolved

graingert added 2 commits December 4, 2024 08:55

correct statement about where thread_exception_hook exceptions go

6224f17

move thread exception even later

8af4991

test when hook args are all None

4b95be7

graingert requested a review from Zac-HD December 4, 2024 09:23

Zac-HD approved these changes Dec 4, 2024

View reviewed changes

graingert merged commit ea1a79a into main Dec 4, 2024
28 checks passed

graingert deleted the threadexception-enhancements branch December 4, 2024 21:19

This was referenced Dec 5, 2024

remove 1-second join from threadexception plugin #13027

Merged

remove 1-second join from threadexception plugin #13028

Closed

nicoddemus added a commit that referenced this pull request Dec 5, 2024

Remove thread join from threadexception plugin (#13027)

80da427

As per review comment: #13016 (comment). Closes #13028. --------- Co-authored-by: Bruno Oliveira <[email protected]>

jakkdl mentioned this pull request Dec 10, 2024

Codecov stuff python-trio/trio#3153

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

threadexception enhancements #13016

threadexception enhancements #13016

graingert commented Dec 1, 2024 •

edited

Loading

graingert Dec 1, 2024

Zac-HD Dec 4, 2024

bluetech Dec 4, 2024

graingert Dec 5, 2024

Zac-HD left a comment

Zac-HD Dec 4, 2024

Zac-HD Dec 4, 2024

graingert Dec 4, 2024

graingert Dec 4, 2024

Zac-HD Dec 4, 2024

graingert Dec 4, 2024

graingert commented Dec 4, 2024

graingert Dec 4, 2024

jakkdl Dec 4, 2024

graingert Dec 4, 2024

jakkdl Dec 4, 2024 •

edited

Loading

jakkdl Dec 4, 2024

jakkdl Dec 4, 2024

jakkdl Dec 4, 2024

graingert commented Dec 4, 2024

Zac-HD left a comment

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

	@pytest.mark.filterwarnings("error::pytest.PytestUnhandledThreadExceptionWarning")
	def test_threadexception_warning_multiple_errors(pytester: Pytester) -> None:
	pytester.makepyfile(
	test_it="""
	import threading

	def test_it():
	def oops():
	raise ValueError("Oops")

	t = threading.Thread(target=oops, name="MyThread")
	t.start()
	t.join()

	t = threading.Thread(target=oops, name="MyThread2")
	t.start()
	t.join()

	def test_2(): pass
	"""
	)
	result = pytester.runpytest()
	assert result.ret == pytest.ExitCode.TESTS_FAILED
	result.assert_outcomes(passed=1, failed=1)
	result.stdout.fnmatch_lines(
	[" \| *ExceptionGroup: multiple thread exception warnings (2 sub-exceptions)"]
	)

threadexception enhancements #13016

threadexception enhancements #13016

Conversation

graingert commented Dec 1, 2024 • edited Loading

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

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

graingert commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl Dec 4, 2024 • edited Loading

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 Dec 4, 2024

Zac-HD left a comment

Choose a reason for hiding this comment

graingert commented Dec 1, 2024 •

edited

Loading

jakkdl Dec 4, 2024 •

edited

Loading