-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
unraisablehook enhancements #12958
base: main
Are you sure you want to change the base?
unraisablehook enhancements #12958
Changes from all commits
85594a7
b79f652
194f9ec
c78f84b
7f30217
d2045ce
7e7c965
b0f803d
d540ada
a285cd5
f853b2c
34b3683
04705b4
4f8aeef
90aa800
055e673
3d6a023
02ada4b
84e7311
086c418
6a76066
2cc6648
77c5126
4741611
a554f2a
f33fb85
af13058
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
A number of :ref:`unraisable <unraisable>` enhancements: | ||
|
||
* Set the unraisable hook as early as possible and unset it as late as possible, to collect the most possible number of unraisable exceptions. | ||
* Call the garbage collector just before unsetting the unraisable hook, to collect any straggling exceptions. | ||
* Collect multiple unraisable exceptions per test phase. | ||
* Report the :mod:`tracemalloc` allocation traceback (if available). | ||
* Avoid using a generator based hook to allow handling :class:`StopIteration` in test failures. | ||
* Report the unraisable exception as the cause of the :class:`pytest.PytestUnraisableExceptionWarning` exception if raised. | ||
* Compute the ``repr`` of the unraisable object in the unraisable hook so you get the latest information if available, and should help with resurrection of the object. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
from __future__ import annotations | ||
|
||
|
||
def tracemalloc_message(source: object) -> str: | ||
if source is None: | ||
return "" | ||
|
||
try: | ||
import tracemalloc | ||
except ImportError: | ||
return "" | ||
|
||
tb = tracemalloc.get_object_traceback(source) | ||
if tb is not None: | ||
formatted_tb = "\n".join(tb.format()) | ||
# Use a leading new line to better separate the (large) output | ||
# from the traceback to the previous warning text. | ||
return f"\nObject allocated at:\n{formatted_tb}" | ||
# No need for a leading new line. | ||
url = "https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings" | ||
return ( | ||
"Enable tracemalloc to get traceback where the object was allocated.\n" | ||
f"See {url} for more info." | ||
) |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,100 +1,149 @@ | ||||||||||||||||
from __future__ import annotations | ||||||||||||||||
|
||||||||||||||||
import collections | ||||||||||||||||
import functools | ||||||||||||||||
import gc | ||||||||||||||||
import sys | ||||||||||||||||
import traceback | ||||||||||||||||
from types import TracebackType | ||||||||||||||||
from typing import Any | ||||||||||||||||
from typing import Callable | ||||||||||||||||
from typing import Generator | ||||||||||||||||
from typing import NamedTuple | ||||||||||||||||
from typing import TYPE_CHECKING | ||||||||||||||||
import warnings | ||||||||||||||||
|
||||||||||||||||
from _pytest.config import Config | ||||||||||||||||
from _pytest.tracemalloc import tracemalloc_message | ||||||||||||||||
import pytest | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
if TYPE_CHECKING: | ||||||||||||||||
from typing_extensions import Self | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
# Copied from cpython/Lib/test/support/__init__.py, with modifications. | ||||||||||||||||
class catch_unraisable_exception: | ||||||||||||||||
"""Context manager catching unraisable exception using sys.unraisablehook. | ||||||||||||||||
|
||||||||||||||||
Storing the exception value (cm.unraisable.exc_value) creates a reference | ||||||||||||||||
cycle. The reference cycle is broken explicitly when the context manager | ||||||||||||||||
exits. | ||||||||||||||||
|
||||||||||||||||
Storing the object (cm.unraisable.object) can resurrect it if it is set to | ||||||||||||||||
an object which is being finalized. Exiting the context manager clears the | ||||||||||||||||
stored object. | ||||||||||||||||
|
||||||||||||||||
Usage: | ||||||||||||||||
with catch_unraisable_exception() as cm: | ||||||||||||||||
# code creating an "unraisable exception" | ||||||||||||||||
... | ||||||||||||||||
# check the unraisable exception: use cm.unraisable | ||||||||||||||||
... | ||||||||||||||||
# cm.unraisable attribute no longer exists at this point | ||||||||||||||||
# (to break a reference cycle) | ||||||||||||||||
""" | ||||||||||||||||
|
||||||||||||||||
def __init__(self) -> None: | ||||||||||||||||
self.unraisable: sys.UnraisableHookArgs | None = None | ||||||||||||||||
self._old_hook: Callable[[sys.UnraisableHookArgs], Any] | None = None | ||||||||||||||||
|
||||||||||||||||
def _hook(self, unraisable: sys.UnraisableHookArgs) -> None: | ||||||||||||||||
# Storing unraisable.object can resurrect an object which is being | ||||||||||||||||
# finalized. Storing unraisable.exc_value creates a reference cycle. | ||||||||||||||||
self.unraisable = unraisable | ||||||||||||||||
|
||||||||||||||||
def __enter__(self) -> Self: | ||||||||||||||||
self._old_hook = sys.unraisablehook | ||||||||||||||||
sys.unraisablehook = self._hook | ||||||||||||||||
return self | ||||||||||||||||
|
||||||||||||||||
def __exit__( | ||||||||||||||||
self, | ||||||||||||||||
exc_type: type[BaseException] | None, | ||||||||||||||||
exc_val: BaseException | None, | ||||||||||||||||
exc_tb: TracebackType | None, | ||||||||||||||||
) -> None: | ||||||||||||||||
assert self._old_hook is not None | ||||||||||||||||
sys.unraisablehook = self._old_hook | ||||||||||||||||
self._old_hook = None | ||||||||||||||||
del self.unraisable | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def unraisable_exception_runtest_hook() -> Generator[None]: | ||||||||||||||||
with catch_unraisable_exception() as cm: | ||||||||||||||||
try: | ||||||||||||||||
yield | ||||||||||||||||
finally: | ||||||||||||||||
if cm.unraisable: | ||||||||||||||||
if cm.unraisable.err_msg is not None: | ||||||||||||||||
err_msg = cm.unraisable.err_msg | ||||||||||||||||
else: | ||||||||||||||||
err_msg = "Exception ignored in" | ||||||||||||||||
msg = f"{err_msg}: {cm.unraisable.object!r}\n\n" | ||||||||||||||||
msg += "".join( | ||||||||||||||||
traceback.format_exception( | ||||||||||||||||
cm.unraisable.exc_type, | ||||||||||||||||
cm.unraisable.exc_value, | ||||||||||||||||
cm.unraisable.exc_traceback, | ||||||||||||||||
) | ||||||||||||||||
) | ||||||||||||||||
warnings.warn(pytest.PytestUnraisableExceptionWarning(msg)) | ||||||||||||||||
pass | ||||||||||||||||
|
||||||||||||||||
if sys.version_info < (3, 11): | ||||||||||||||||
from exceptiongroup import ExceptionGroup | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def gc_collect_harder() -> None: | ||||||||||||||||
# Constant determined experimentally by the Trio project. | ||||||||||||||||
for _ in range(5): | ||||||||||||||||
gc.collect() | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
@pytest.hookimpl(wrapper=True, tryfirst=True) | ||||||||||||||||
def pytest_runtest_setup() -> Generator[None]: | ||||||||||||||||
yield from unraisable_exception_runtest_hook() | ||||||||||||||||
class UnraisableMeta(NamedTuple): | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd use a dataclass; named tuples feel a bit outdated these days. But no problem if you want to keep it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a gut feeling that NamedTuple is better here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate? It is only being used for attribute access, never as an actual tuple. I agree with @bluetech, in general I go with https://codereview.doctor/features/python/best-practice/avoid-named-tuple-use-dataclass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the end though, is not a big problem if you really want to use a |
||||||||||||||||
msg: str | ||||||||||||||||
cause_msg: str | ||||||||||||||||
exc_value: BaseException | None | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
@pytest.hookimpl(wrapper=True, tryfirst=True) | ||||||||||||||||
def pytest_runtest_call() -> Generator[None]: | ||||||||||||||||
yield from unraisable_exception_runtest_hook() | ||||||||||||||||
_unraisable_exceptions: collections.deque[UnraisableMeta | BaseException] = ( | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use the config stash for this instead of a global? Of course, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do I get to the config from the various test stage hooks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They can all receive an |
||||||||||||||||
collections.deque() | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
@pytest.hookimpl(wrapper=True, tryfirst=True) | ||||||||||||||||
def pytest_runtest_teardown() -> Generator[None]: | ||||||||||||||||
yield from unraisable_exception_runtest_hook() | ||||||||||||||||
def collect_unraisable() -> None: | ||||||||||||||||
errors: list[pytest.PytestUnraisableExceptionWarning | RuntimeError] = [] | ||||||||||||||||
meta = None | ||||||||||||||||
hook_error = None | ||||||||||||||||
try: | ||||||||||||||||
while True: | ||||||||||||||||
try: | ||||||||||||||||
meta = _unraisable_exceptions.pop() | ||||||||||||||||
except IndexError: | ||||||||||||||||
break | ||||||||||||||||
Comment on lines
+47
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps like this? It's shorter and makes the stop condition more obvious.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current code only checks if the deque has an item once, and |
||||||||||||||||
|
||||||||||||||||
if isinstance(meta, BaseException): | ||||||||||||||||
hook_error = RuntimeError("Failed to process unraisable exception") | ||||||||||||||||
hook_error.__cause__ = meta | ||||||||||||||||
errors.append(hook_error) | ||||||||||||||||
continue | ||||||||||||||||
|
||||||||||||||||
msg = meta.msg | ||||||||||||||||
try: | ||||||||||||||||
warnings.warn(pytest.PytestUnraisableExceptionWarning(msg)) | ||||||||||||||||
except pytest.PytestUnraisableExceptionWarning as e: | ||||||||||||||||
# This except happens when the warning is treated as an error (e.g. `-Werror`). | ||||||||||||||||
if meta.exc_value is not None: | ||||||||||||||||
# Exceptions have a better way to show the traceback, but | ||||||||||||||||
# warnings do not, so hide the traceback from the msg and | ||||||||||||||||
# set the cause so the traceback shows up in the right place. | ||||||||||||||||
e.args = (meta.cause_msg,) | ||||||||||||||||
e.__cause__ = meta.exc_value | ||||||||||||||||
errors.append(e) | ||||||||||||||||
|
||||||||||||||||
if len(errors) == 1: | ||||||||||||||||
raise errors[0] | ||||||||||||||||
if errors: | ||||||||||||||||
raise ExceptionGroup("multiple unraisable exception warnings", errors) | ||||||||||||||||
finally: | ||||||||||||||||
del errors, meta, hook_error | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def _cleanup(*, prev_hook: Callable[[sys.UnraisableHookArgs], object]) -> None: | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't matter much, asking more for consistency, but any reason why we are using a leading |
||||||||||||||||
try: | ||||||||||||||||
gc_collect_harder() | ||||||||||||||||
collect_unraisable() | ||||||||||||||||
finally: | ||||||||||||||||
sys.unraisablehook = prev_hook | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def unraisable_hook( | ||||||||||||||||
unraisable: sys.UnraisableHookArgs, | ||||||||||||||||
/, | ||||||||||||||||
*, | ||||||||||||||||
append: Callable[[UnraisableMeta | BaseException], object], | ||||||||||||||||
) -> None: | ||||||||||||||||
try: | ||||||||||||||||
graingert marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
err_msg = ( | ||||||||||||||||
"Exception ignored in" if unraisable.err_msg is None else unraisable.err_msg | ||||||||||||||||
) | ||||||||||||||||
summary = f"{err_msg}: {unraisable.object!r}" | ||||||||||||||||
traceback_message = "\n\n" + "".join( | ||||||||||||||||
traceback.format_exception( | ||||||||||||||||
unraisable.exc_type, | ||||||||||||||||
unraisable.exc_value, | ||||||||||||||||
unraisable.exc_traceback, | ||||||||||||||||
) | ||||||||||||||||
) | ||||||||||||||||
tracemalloc_tb = tracemalloc_message(unraisable.object) | ||||||||||||||||
msg = summary + traceback_message + tracemalloc_tb | ||||||||||||||||
cause_msg = summary + tracemalloc_tb | ||||||||||||||||
|
||||||||||||||||
append( | ||||||||||||||||
UnraisableMeta( | ||||||||||||||||
# we need to compute these strings here as they might change after | ||||||||||||||||
# the unraisablehook finishes and before the unraisable object is | ||||||||||||||||
# collected by a hook | ||||||||||||||||
msg=msg, | ||||||||||||||||
cause_msg=cause_msg, | ||||||||||||||||
exc_value=unraisable.exc_value, | ||||||||||||||||
) | ||||||||||||||||
) | ||||||||||||||||
except BaseException as e: | ||||||||||||||||
append(e) | ||||||||||||||||
# Raising this will cause the exception to be logged twice, once in our | ||||||||||||||||
# collect_unraisable and once by the unraisablehook calling machinery | ||||||||||||||||
# which is fine - this should never happen anyway and if it does | ||||||||||||||||
# it should probably be reported as a pytest bug. | ||||||||||||||||
raise | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
def pytest_configure(config: Config) -> None: | ||||||||||||||||
prev_hook = sys.unraisablehook | ||||||||||||||||
config.add_cleanup(functools.partial(_cleanup, prev_hook=prev_hook)) | ||||||||||||||||
sys.unraisablehook = functools.partial( | ||||||||||||||||
unraisable_hook, append=_unraisable_exceptions.append | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
@pytest.hookimpl(trylast=True) | ||||||||||||||||
def pytest_runtest_setup() -> None: | ||||||||||||||||
collect_unraisable() | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
@pytest.hookimpl(trylast=True) | ||||||||||||||||
def pytest_runtest_call() -> None: | ||||||||||||||||
collect_unraisable() | ||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
@pytest.hookimpl(trylast=True) | ||||||||||||||||
def pytest_runtest_teardown() -> None: | ||||||||||||||||
collect_unraisable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be covered on pypy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave that comment in the code? More likely for future maintainers to see it that way 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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