Fail test runs on unraisable exceptions?

[Adam Johnson]

Python has the concept of unraisable exceptions - exceptions that cannot be raised at the time they occur because they would break something about the runtime. For example, an exception raised during an object's __del__ method would break garbage collection.

Python 3.8 added the ability to take action when unraisable exceptions occur, by setting sys.unraisablehook to a callable ( https://docs.python.org/3.8/library/sys.html#sys.unraisablehook ). I propose that we extend Django's test runner to add such a hook that records if any unraisable exceptions occur during a test run, and fails the test run if this is the case.

The use case I came across for this was using asyncio in debug mode, which warns about unawaited coroutines and unretrieved futures ( https://docs.python.org/3.8/library/asyncio-dev.html#detect-never-awaited-coroutines ). It can be very handy to add a warning filter to turn these RuntimeWarnings into exceptions in tests and development. Unfortunately such exceptions are always (or often?) unraisable, so Python prints their stack trace with the default unraisable hook, but carries on as if no error occurred. Failing test runs if unraisable exceptions occur could help highlight such issues.

I'm writing to the mailing list because I'm not sure if failing the test run would always make sense. Does anyone else have experience with unraisable exceptions?

FWIW, pytest has only one stalled discussion: https://github.com/pytest-dev/pytest/issues/5299

--

Adam

[Carlton Gibson]

Hey Adam.

The asyncio example is a good one. Sometimes you're trying hard to build a test in the first place — it might just be that you didn't manage to await your coroutine. Oh well... — for me, I'd be making sure we cleaned that up before merge (or, can you filter those... — I guess you can 🤔) but I wouldn't want to block development on a "I can't get the tests to pass". Does that make sense?

Maybe we should be stricter, but that's my first thought.

Kind Regards,

Carlton

[Adam Johnson]

> I'd be making sure we cleaned that up before merge (or, can you filter those... — I guess you can 🤔)

In my experience I’d something doesn’t fail, it gets ignored. Few people read the logs of passing test runs and most projects I’ve seen have a pile of noisy warnings making detecting unraisable exceptions even more unreasonable.

In the asyncio case, an unraisable exception could indeed represent bad test code, but it could also reveal a bug in the production code which no test assertion would (or even could?) pick up.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/df093ae7-5559-42ef-8ca7-3af551a0d14an%40googlegroups.com.

--

Adam

[Carlton Gibson]

On 30 Oct 2020, at 13:56, Adam Johnson <m...@adamj.eu> wrote:

> I'd be making sure we cleaned that up before merge (or, can you filter those... — I guess you can 🤔)

In my experience I’d something doesn’t fail, it gets ignored. Few people read the logs of passing test runs and most projects I’ve seen have a pile of noisy warnings making detecting unraisable exceptions even more unreasonable.

In the asyncio case, an unraisable exception could indeed represent bad test code, but it could also reveal a bug in the production code which no test assertion would (or even could?) pick up.

Not sure… In my experience of these it’s normally that the test case exited (having passed) before the asyncio task (being run in another thread) fully exited. So you end up putting a (e.g.) communicator.wait() in to clean it up. Not saying that shouldn’t be there, but these things can be so hard to write at all that to fail on that might be setting the bar too high. (You want to see green — yes, my test worked.) 

You’re right that these can linger, but not with Mariusz on the case :) 

These errors though are raised at process exit thought right? (i.e. aren’t they after the test suite already reported as passed/failed?) 🤔

Kind Regards,

Carlton

[Adam Johnson]

> You’re right that these can linger, but not with Mariusz on the case :) 

My proposal is to put this into the test runner for Django users rather than (only) Django’s own test suite. Sadly we can’t have Mariusz on every project.

> These errors though are raised at process exit thought right? (i.e. aren’t they after the test suite already reported as passed/failed?) 🤔

No, by default python will print a trace back when they occur and that’s all. I’d like to add a check at test suite end that if any occurred, the suite fails rather than passes - the same as if a raisavle exception occurred.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/5B0EA3FD-CC0D-446E-BACC-3ACDEFEB2682%40gmail.com.

--

Adam

[Carlton Gibson]

On 30 Oct 2020, at 14:20, Adam Johnson <m...@adamj.eu> wrote:

My proposal is to put this into the test runner for Django users

OK, I misunderstood. Gotcha. 

>  … I'm not sure if failing the test run would always make sense.

Always no, for the reasons given. Always awaiting every coroutine can be hard, and not always (often not 🤔) significant. 

Let me rephrase, why isn’t python -Werror (or a more nuanced version of that) sufficient? 

[Adam Johnson]

-Werror transforms warnings into exceptions. This is what happens with the asyncio case yes - but the exception is unraisable so python prints the stack trace and carries on as if no problem occurred.

Focussing only on the asyncio case may be counter productive. They are warnings by default and it requires a warning filter to convert them to exceptions, which are then unraisable.

It’s easy to create unraisable exceptions through other mechanisms, such as in __del__ methods, but in practice thankfully rare. But I think in general any unraisable exception represents something you would want to know about in your test suite - just like a normal raisable exception.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/9AD9DB42-5B0D-41CA-995F-2DF4CD92783A%40gmail.com.

--

Adam

[Tom Forbes]

Would we need to run “gc.collect” after each test to make it deterministic?

I’m all for this change - I think it’s a sensible thing to do.

Tom

On 30 Oct 2020, at 16:30, Adam Johnson <m...@adamj.eu> wrote:



To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM1cDG0vE_EYiLJ_5ih7%3D4Zh4Keofk3oQRx%2BFLFqEhM-ig%40mail.gmail.com.

[Carlton Gibson]

On 30 Oct 2020, at 17:29, Adam Johnson <m...@adamj.eu> wrote:

Focussing only on the asyncio case may be counter productive. They are warnings by default and it requires a warning filter to convert them to exceptions, which are then unraisable.

OK, makes sense. Thanks for explaining it. Given that, seems worth a try yes. +1

[Adam Johnson]

Would we need to run “gc.collect” after each test to make it deterministic?

Not particularly. In CPython, most objects are deterministically deleted when they go out of scope. Only circular references are gathered by the garbage collector, and I think it's okay to leave them as-is - the exception should be clear enough to debug such rare occurences.

I finally created a ticket for this, linking to the pytest reference implementation: https://code.djangoproject.com/ticket/32557

I also created a ticket for a similar feature, tracking exceptions in threads that aren't picked up by the main thread: https://code.djangoproject.com/ticket/32558

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/351D56E9-B52D-4F3A-9CCE-072C206FEA96%40gmail.com.

--

Adam