Opentrons / opentrons

Software for writing protocols and running them on the Opentrons Flex and Opentrons OT-2
https://opentrons.com
Apache License 2.0
423 stars 178 forks source link

Python tests: Explicitly mark `async` functions instead of relying on `pytest-aiohttp` magic #8176

Closed SyntaxColoring closed 10 months ago

SyntaxColoring commented 3 years ago

Our Python tests define asynchronous test functions like this:

async def test_my_thing() -> None:
    assert await foo() == bar

pytest doesn't natively support running async functions. It currently happens to magically work for us because we have the pytest-aiohttp plugin installed as a dev dependency.

For explicitness and normalcy, we instead want to:

mcous commented 3 years ago

AnyIO ships with a pytest plugin with the same pytestmark interface, if we wanted to add AnyIO as a dep or dev dep

SyntaxColoring commented 3 years ago

Whichever library we choose, we should do some due diligence to see how it handles the fringes of setting up and tearing down the async environments that it runs test functions in. I only tenuously understand these concerns, but they seem surprisingly nuanced. Here are some questions I just opened on pytest-asyncio: https://github.com/pytest-dev/pytest-asyncio/issues/222

But, on the other hand, it's not like we get to avoid these concerns by doing nothing and sticking with pytest-aiohttp magic.

mcous commented 3 years ago

Now that #8228 has been merged, we have anyio available in the api and robot-server projects, which means its opt-in pytest plugin is also available in tests for those projects.

Due diligence still required, but if anyio's setup/teardown logic looks sound, then this seems like a tempting option

mcous commented 3 years ago

In terms of some really cursory inspection of the anyio test runner for the asyncio backend, it looks like anyio will:

It does not appear to shutdown the default executor explicitely. So far find no evidence that pytest-aiohttp or pytest-asyncio do either, though.

mcous commented 3 years ago

Upon even further digging, it appears that in asyncio, loop.close will also shut down the default executor. This appears true in both Python 3.7 and Python 3.10

SyntaxColoring commented 2 years ago

I tried implementing this with AnyIO's pytest plugin, but I found that its API has an unfortunate trap.

If you have something like this:

@pytest.fixture
async def my_async_fixture():
    ...

def my_non_async_test_func(fixture):
    ...

Then you'd expect my_async_fixture() to execute before my_non_async_test_func(), but in fact it never does.

This is because my_non_async_test_func() is, understandably, outside of the AnyIO plugin's consideration. It's just a normal non-async test function. So its fixture processing falls under pytest's normal rules. pytest "calls" my_async_fixture() like any other function. But because it's a coroutine, "calling" it just returns an awaitable coroutine object; it doesn't actually run its code.

Here's the specific fixture that tipped me off, used by this test.

At best, this causes confusing errors, and at worst, it makes tests silently run under the wrong environment. After migrating around 90% of the api test suite and trying a couple workarounds to keep the tests passing, I consider it a deal-breaker for the AnyIO plugin. It makes things too fragile, and it's too surprising for the unwary. Especially for us in particular, since we frequently switch tests and fixtures between async and non-async, and we have a lot of fixtures living in conftest.py files far away from the things using them.

I believe the pytest-asyncio plugin avoids this trap.

SyntaxColoring commented 10 months ago

Closed by #9981, #10012, and #10022.