Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks
SyntaxColoring opened this issue Jul 28, 2021 · 7 comments
Labels
api Affects the `api` project python General Python tickets and PRs; e.g. tech debt or Python guild activities refactor robot server Affects the `robot-server` project

Comments

@SyntaxColoring
Copy link
Contributor

SyntaxColoring commented Jul 28, 2021

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:

  • Add pytest-asyncio as a dev dependency.
  • Explicitly mark our async test functions with pytest.mark.asyncio.
  • Remove our dependency on pytest-aiohttp, assuming it's not used anywhere else.
@SyntaxColoring SyntaxColoring added api Affects the `api` project refactor robot server Affects the `robot-server` project python General Python tickets and PRs; e.g. tech debt or Python guild activities labels Jul 28, 2021
@mcous
Copy link
Contributor

mcous commented Jul 28, 2021

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

@SyntaxColoring SyntaxColoring changed the title Python tests: Explicitly mark async functions instead of relying on aiohttp magic Python tests: Explicitly mark async functions instead of relying on pytest-aiohttp magic Jul 29, 2021
@SyntaxColoring
Copy link
Contributor Author

SyntaxColoring commented Jul 29, 2021

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: pytest-dev/pytest-asyncio#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
Copy link
Contributor

mcous commented Aug 26, 2021

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
Copy link
Contributor

mcous commented Aug 26, 2021

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

  • Cancel all outstanding tasks
  • Shudown async generators
  • Close the loop

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
Copy link
Contributor

mcous commented Aug 26, 2021

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
Copy link
Contributor Author

SyntaxColoring commented Apr 16, 2022

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project python General Python tickets and PRs; e.g. tech debt or Python guild activities refactor robot server Affects the `robot-server` project
Projects
None yet
Development

No branches or pull requests

2 participants