pytest.raises swallows warning about unawaited coroutine

[seifertm]

Binding the return value of the pytest.raises context manager to a variable prevents some warnings from being triggered as part of the test. As a result, the warning is not displayed in the test run summary, but only on stderr, and -W error will not fail the test.

Reproducible example:

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())

Running the above code:

$ pytest -s -W error
===== test session starts =====
platform linux -- Python 3.10.8, pytest-7.1.3, pluggy-1.0.0
rootdir: /tmp/t
collected 1 item                                                                                                                                                                                                                                                             

test_a.py .

===== 1 passed in 0.00s =====
sys:1: RuntimeWarning: coroutine 'my_task' was never awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

When replacing with pytest.raises(RuntimeError) as _: with with pytest.raises(RuntimeError):, everything works as expected and the warning causes the test run to fail.

My understanding is that the coroutine triggers the warning when it gets garbage collected. It looks like with pytest.raises as _ keeps a reference to the coroutine that should trigger the warning. The reference prevents garbage collection of the coroutine until after the hook wrappers in unraisableexception have returned. As a result, the warning appears outside of the test case.

Additional information

$ pip list
Package    Version
---------- -------
attrs      22.1.0
iniconfig  1.1.1
packaging  21.3
pip        22.3
pluggy     1.0.0
py         1.11.0
pyparsing  3.0.9
pytest     7.1.3
setuptools 65.4.1
tomli      2.0.1

Related issues: #8021 #5676

[Zac-HD]

This sounds like a classic "holding the traceback keeps everything in referenced frames alive" problem, which we will want to fix.

That said, there is no guarantee that garbage collection will run at any particular time (or at all!) even if we do fix it, so I don't consider this a serious bug.

[Dreamsorcerer]

That said, there is no guarantee that garbage collection will run at any particular time (or at all!)

Even if you do a gc.collect() at the end of each test run?

[RonnyPfannschmidt]

A next generation raises could return a intentionally cleaned up traveback representation

I'm not Fond of the py.code remnant, but it's quite a bit of work to make it nice

[simon-liebehenschel]

I wasted some time reading all issues here to find how to fail a test when a coroutine was not awaited. In case if someone will need a solution:

# pytest.ini

filterwarnings=
    error:coroutine .* was never awaited:RuntimeWarning
    # A trick to fail the `PytestUnraisableExceptionWarning: Exception ignored in:` which happens when we catch a `was never awaited` warning
    error:.*Exception ignored in.*::_pytest

[Dreamsorcerer]

I wasted some time reading all issues here to find how to fail a test when a coroutine was not awaited.

That's unrelated to this issue though. We use filterwarnings = error to error on all warnings. The issue here is that a warning occurs outside the actual test run and therefore doesn't cause a test failure.

[jakkdl]

type checkers (at least mypy) now warns about unused coroutines, and ruff is working on it as well astral-sh/ruff#9911, so most instances of this can be caught with static analysis.

[Dreamsorcerer]

In the trivial example above, yes. In real world use cases we've seen, no.

We've been using that option in mypy for several years, it cannot catch the issues we encountered (and still produces many false positives with TaskGroup etc.), which were caused by something deeper in the library with more complex low-level logic.

[graingert]

I think this is a bug in create_task.

Also FYI this doesn't happen with unittest assertRaises because it clears the Traceback. I don't think pytest should do this however as in some tests I rely on the Traceback not being cleared

[jakkdl]

I think this is a bug in create_task.

hmm, really? It doesn't seem to do anything particularly spicy, I can minify the repro to this:

import pytest

async def my_task():
    pass

def blah(coro):
    raise RuntimeError

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

[RonnyPfannschmidt]

indeed its not a bug with create_task

unit test annihilates the traceback in https://github.com/python/cpython/blob/6245ee279db8d85d7a72bc6ec24021eab06987fe/Lib/unittest/case.py#L253-L278

pytest does not annihilate the traceback

keeping the traceback prevents timely GC

[jakkdl]

A next generation raises could return a intentionally cleaned up traceback representation

I'm failing to understand how one could clean up the traceback to avoid this issue? I at first thought we could just skip some frames in it, but it seems this needs to be a fairly involved modification to manage to make any difference.

That said, there is no guarantee that garbage collection will run at any particular time (or at all!)

Even if you do a gc.collect() at the end of each test run?

I tried to add a gc.collect(0) to pytest_runtest_call, and afaict it gets the warning raised correctly right away. I suppose it might be a slight performance hit, but am not sure if there's any other problems with that approach?

Always annihilating the traceback is probably not great, but I could see a keyword argument save_traceback=True to pytest.raises where a user could opt into keeping it? And then we can encourage the user in the docs for that parameter to delete the object.

[graingert]

Definitely make clearing the traceback opt in rather than opt out

I suspect a gc.collect between each test will be quite a bit of overhead on test suites with lots of small fast tests, eg hypothesis or parameterized test.

Perhaps instead it would make sense to delay unsetting the unraisablehook until the absolute last possible moment in pytest session teardown, and issue 5 gc.collect calls before unsetting it

Also I think the unraisablehook is set and reset each test, I think it would be better to set it once and update a variable with the current test to associate errors with, to make sure there's no window for a gc to run between tests

[jakkdl]

Definitely make clearing the traceback opt in rather than opt out

having an opt-in traceback clear won't help ~anybody with the OP's issue, there's already a warning in the documentation telling users they should be vary of garbage collection: https://docs.pytest.org/en/stable/reference/reference.html#pytest.raises so best-practice users should be deleting their caught exceptions.

Perhaps instead it would make sense to delay unsetting the unraisablehook until the absolute last possible moment in pytest session teardown, and issue 5 gc.collect calls before unsetting it

Also I think the unraisablehook is set and reset each test, I think it would be better to set it once and update a variable with the current test to associate errors with, to make sure there's no window for a gc to run between tests

this sounds like a decent approach though. You won't get the error on the correct test, but you'll get it somewhere, and the warning/error should contain enough info to figure it out. Although in that case perhaps better to signal it as a general teardown error instead of blaming some random specific test - i.e. set the variable for current test in unraisablehook to None between/after tests.

[RonnyPfannschmidt]

Let's have exceptioninfo from raises trigger a resource warning if it's not cleared after a test

Also a discard traceback option might be helpful

It could enable a warning and a path towards clearing by default as the stdlib does

A opt in towards a policy is also thinkable

[graingert]

Let's have exceptioninfo from raises trigger a resource warning if it's not cleared after a test

This would raise a ResourceWarning when you failed to do:

with pytest.raises(x) as exc_info:
    ...

with contextlib.closing(exc_info):
    ...

I think it's cleaner to make the unraisablehook more reliable

[jakkdl]

@graingert was this fixed with #12958?

[graingert]

Almost! To use the new behavior you need to set the warning filter with the python environment variable. I'll need to make a PR to make changes to the built in warnings filters so they're applied asap and persist until the config teardown