cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

tests: switch from pytest.fixture to pytest_asyncio.fixture for async code

Open oliver-sanders opened this issue 10 months ago • 4 comments

  • Closes https://github.com/cylc/cylc-flow/issues/5995
  • 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).

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 run pytest -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

  • [x] I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • [x] Contains logically grouped changes (else tidy your branch by rebase).
  • [x] Does not contain off-topic changes (use other PRs for other changes).
  • [x] Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • [x] Tests are included (or explain why tests are not needed).
  • [x] Changelog entry included if this is a change that can affect users
  • [x] Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • [x] If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

oliver-sanders avatar Apr 14 '25 10:04 oliver-sanders

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.

oliver-sanders avatar Apr 14 '25 15:04 oliver-sanders

According to the warnings:

The event loop scope for asynchronous fixtures will default to the fixture caching scope. Future versions of pytest-asyncio will default the loop scope for asynchronous fixtures to function scope. Set the default fixture loop scope explicitly in order to avoid unexpected behavior in the future.

So for now, @pytest_asyncio.fixture(scope="module") is the same as scope="module", loop_scope="module", but in the future this will change to scope="module", loop_scope="function"?

But this sounds like it will break, according to the docs:

The loop_scope of a fixture can be chosen independently from its caching scope. However, the event loop scope must be larger or the same as the fixture’s caching scope. In other words, it’s possible to reevaluate an async fixture multiple times within the same event loop, but it’s not possible to switch out the running event loop in an async fixture.

Also, according to the migration guide from v0.23:

Explicitly set the loop_scope of async fixtures by replacing occurrences of @pytest.fixture(scope="…") and @pytest_asyncio.fixture(scope="…") with @pytest_asyncio.fixture(loop_scope="…", scope="…") such that loop_scope and scope are the same.

So I'm rather confused

MetRonnie avatar Apr 15 '25 09:04 MetRonnie

Drafting this whilst we investigate whether there is another way as this is pretty nasty really.

oliver-sanders avatar Apr 15 '25 12:04 oliver-sanders

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.

oliver-sanders avatar Jun 11 '25 12:06 oliver-sanders