tests: switch from pytest.fixture to pytest_asyncio.fixture for async code #6726
Conversation
oliver-sanders
force-pushed
the
5995
branch
2 times, most recently
from
747c583 to
8cd327c
Compare
… code * Closes https://github.com/cylc/cylc-flow/issues/5995t * Asynchronous fixtures should use `pytest_asyncio.fixture` rather than `pytest.fixture` when attempting to override the default event loop scope. * In practice, this has to be done whenever the fixture scope is not "function" (which is the pytest_asyncio default).
| # BACK COMAPAT: pytest-asyncio 0.21.2 | ||
| # FROM: python 3 | ||
| # TO: python 3.7 | ||
| # URL: https://github.com/cylc/cylc-flow/pull/6726 | ||
| pytest-asyncio>=0.21.2,!=0.23.* |
There was a problem hiding this comment.
We can't drop 0.21 support just yet, because it's the last version to support Python 3.7 :(
| if pytest_asyncio.__version__.startswith('0.21'): | ||
| # BACK COMPAT: event_loop | ||
| # FROM: python 3 | ||
| # TO: python 3.7 | ||
| # URL: https://github.com/cylc/cylc-flow/pull/6726 | ||
| @pytest_asyncio.fixture(scope='module') | ||
| def event_loop(): | ||
| """This fixture defines the event loop used for each test. | ||
| The default scoping for this fixture is "function" which means that all | ||
| async fixtures must have "function" scoping. | ||
| Defining `event_loop` as a module scoped fixture opens the door to | ||
| module scoped fixtures but means all tests in a module will run in the | ||
| same event loop. This is fine, it's actually an efficiency win but also | ||
| something to be aware of. | ||
| See: https://github.com/pytest-dev/pytest-asyncio/issues/171 | ||
| """ | ||
| loop = asyncio.get_event_loop_policy().new_event_loop() | ||
| yield loop | ||
| # gracefully exit async generators | ||
| loop.run_until_complete(loop.shutdown_asyncgens()) | ||
| # cancel any tasks still running in this event loop | ||
| for task in asyncio.all_tasks(loop): | ||
| task.cancel() | ||
| loop.close() |
There was a problem hiding this comment.
We can get rid of this soon.
|
Note: This does not silence the Python warnings because setting the default loop scope (as recommended) appears to break our ability to override it. Other projects seem to be suppressing these warnings by the look of their GH issues. Soon they will finish this migration and the warnings will vanish. |
|
According to the warnings:
So for now, But this sounds like it will break, according to the docs:
Also, according to the migration guide from v0.23:
So I'm rather confused 
|
|
|
||
|
|
||
| @pytest.fixture(scope='module') | ||
| @pytest_asyncio.fixture(scope='module') |
There was a problem hiding this comment.
Are you supposed to change these when the fixture doesn't do anything async?
There was a problem hiding this comment.
Should be harmless, but not necessary.
There was a problem hiding this comment.
It's just occurred to me that any @pytest.fixtures added on 8.4.x will likely make it into master unnoticed
|
Drafting this whilst we investigate whether there is another way as this is pretty nasty really. |
|
pytest-asyncio have now released 1.0.0, strangely our tests seem to pass (I don't understand how), but our event loop scope is presumably being ignored? Unfortunately, they cut the issue to provide the old behaviour as an option, pushing it back to 1.1. |
pytest_asyncio.fixturerather thanpytest.fixturewhen attempting to override the default event loop scope.Getting this change out the way now before they drop support for the status quo.
To test, find a differently scoped fixture and jam a breakpoint into the code to ensure that it is called the expected number of times.
E.g, add a breakpoint into
tests/integration/network/test_client.py::harness, then runpytest -n0 --dist=no tests/integration/network/test_client.py::test_graphql tests/integration/network/test_client.py::test_protobuf. The breakpoint will only be hit once.Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).?.?.xbranch.