Opened 3 years ago

Last modified 21 months ago

#32557 new New feature

Fail tests when unraisable exceptions occur

Reported by: Adam Johnson Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Carlton Gibson, Tom Forbes, Chris Jerdonek Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Following django-developers discussion: https://groups.google.com/g/django-developers/c/ea21KjiGiY4/m/7HTnQaS8BwAJ

Django's test runner should install an unraisable exception hook and fail the test run if it is ever triggered. The check could be done just once at the end of the test run (in subprocesses when run in parallel), because unraisable exceptions already print their traceback by default.

Change History (6)

comment:1 by Adam Johnson, 3 years ago

There's a reference implementation in pytest we might be able to copy: https://github.com/pytest-dev/pytest/pull/8055

Additionally here's an implementation I've used - calling the context manager in an overridden DiscoverRunner.run_tests to wrap the whole test run:

@contextmanager
def fail_if_any_unraisable_exceptions():
    orig_unraisable_hook = sys.unraisablehook
    hook_called = multiprocessing.Value("b", lock=False)
    hook_called.value = 0

    def hook(unraisable, /):
        hook_called.value = 1
        orig_unraisable_hook(unraisable)

    sys.unraisablehook = hook

    try:
        yield
    finally:
        sys.unraisablehook = orig_unraisable_hook

        if hook_called.value:
            print(
                "❌ Unraisable exceptions reported, failing test run - see above.",
                file=sys.stderr,
            )
            sys.exit(1)

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Carlton Gibson Tom Forbes added
Triage Stage: UnreviewedAccepted

Tentatively accepted. I would like to check this in practice.

comment:3 by Mariusz Felisiak, 3 years ago

Cc: Chris Jerdonek added

comment:4 by Chris Jerdonek, 3 years ago

One comment: the sys.unraisablehook() docs say:

Called when an exception has occurred but there is no way for Python to handle it. For example, when a destructor raises an exception or during garbage collection (gc.collect()).

Thus, with this patch, if we disable garbage collection as in this PR:
https://github.com/django/django/pull/14142
then it seems like we won't get the benefit of this patch in checking for exceptions that occur during garbage collection.

Last edited 3 years ago by Chris Jerdonek (previous) (diff)

comment:5 by Adam Johnson, 3 years ago

Exceptions in __del__ are only one kind of unraisable exception. The other kind I know of that motivated me to look into them are from async tasks that were not joined by the task that spawned them.

Python already prints tracebacks for unraisable exceptions, the main point of this ticket is to ensure that such tracebacks in tests do not get missed because the tests exit with a 0 status code. The default behaviour looks like this:

$ cat test.py
class Naughty:
    def __del__(self):
        1 / 0


Naughty()

$ python test.py
Exception ignored in: <function Naughty.__del__ at 0x1015da8b0>
Traceback (most recent call last):
  File "/Users/chainz/tmp/test/test.py", line 3, in __del__
    1 / 0
ZeroDivisionError: division by zero

$ echo $?
0

The PR disabling gc affects only objects in cycles, which are a minority. Moreover all garbage is still collected at interpreter shutdown, so any unraisable exceptions would have their traceback printed at that point, although they indeed not be picked up by a sys.unraisablehook installed only for the duration of the test run. That said, the gc PR affects only the Django test suite which has no unraisable exceptions at the moment, and it's likely if any were added someone would spot them.

comment:6 by Thomas Grainger, 21 months ago

You should be able to run gc.collect in a loop until no more objects get collected just before you unset your unraisablehook

Or just a constant number of gc.collect cycles 4 should be enough for anyone
https://github.com/python-trio/trio/blob/7b86d20549bba2629eb929a72022a19b999c91d5/trio/_core/tests/tutil.py#L55

Last edited 21 months ago by Thomas Grainger (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top