bluesky / ophyd-async

Hardware abstraction for bluesky written using asyncio
https://blueskyproject.io/ophyd-async
BSD 3-Clause "New" or "Revised" License
8 stars 23 forks source link

Correctly handle pending tasks in our tests #519

Open olliesilvester opened 1 month ago

olliesilvester commented 1 month ago

Running unit tests sometimes gives us a few Task was destroyed but it is pending! messages. This is because, in our pytests, we either:

  1. Use the RE fixture to create an event loop, do the test, then the fixture closes the event loop at the end of the pytest using pytest's add_finalizer. This can run before awaiting for pending tasks, so pending tasks can be cancelled.

  2. Use an Asyncio event loop outside of the RE and the same as above

In both cases, if we create a task using asyncio.create_taskand then finish the test before any awaits are ran, there is a chance that the event loop gets closed with pending tasks, which causes that warning message To make sure these pending tasks get completed, we can either:

  1. Make our event loop fixtures do an await at the end of a pytest, eg

    def RE(request)
    ...
    if pending:
            loop.run_until_complete(asyncio.gather(*pending, return_exceptions=True))
  2. Make our event loop fixtures raise an error if there are pending tasks at the end, and tell us which tasks they are

  3. Something else clever which I didn't think of

evalott100 commented 1 month ago

Closing https://github.com/bluesky/ophyd-async/issues/260 in favour of this

jwlodek commented 1 month ago

I think #498 is also a duplicate of this. Suggest we close that since this goes into quite a bit more detail.

coretl commented 1 month ago
  1. Make our event loop fixtures raise an error if there are pending tasks at the end, and tell us which tasks they are

That gets my vote. Do you know how we would do that?

evalott100 commented 4 weeks ago

That gets my vote. Do you know how we would do that?

We could do this for tests using the RE fixture by getting pending from the run engine loop.

Or more generally, something like:

@pytest_asyncio.fixture(autouse=True)
async def assert_no_pending_tasks(request):
    # There should be no tasks pending after a test has finished
    try:
        yield
    finally:
        loop = asyncio.get_event_loop()
        pending = asyncio.all_tasks(loop)
        if pending # and some_filtering_perhaps:
            unfinished_tasks = ...
            raise RuntimeError(
                "Not all tasks {unfinished_tasks} closed during test."
            )
        # Don't close the event loop
olliesilvester commented 4 weeks ago

We could do this for tests using the RE fixture by getting pending from the run engine loop.

Yep, this is what I initially tried in that other PR. It raises errors but it's not necessarily obvious as to which test an unfinished task came from

evalott100 commented 4 weeks ago

Yep, this is what I initially tried in that other PR. It raises errors but it's not necessarily obvious as to which test an unfinished task came from

I think we can use traceback in the standard library and raise a new UnresolvedTaskError (or a better name) encapsulating that info.

We should also only raise the UnresolvedTaskError there if the test didn't fail, otherwise we just raise the excepted error after cancelling the background tasks.