pytest-dev / pytest-asyncio

Asyncio support for pytest
https://pytest-asyncio.readthedocs.io
Apache License 2.0
1.43k stars 152 forks source link

Preprocess async fixtures for each event loop #906

Closed cstruct closed 3 months ago

cstruct commented 3 months ago

Caching of fixture preprocessing is now also keyed by event loop id, sync fixtures can be processed if they are wrapping a async fixture, each async fixture has a mapping from root scope name to fixture id that is now used to dynamically get the event loop fixture.

3 tests fail for me locally, 2 more are flaky, some of these fail without my changes.

I'm note sure if I'm conflating fixture and loop scope and if that is contributing to the failed tests.

I feel like I have not written the clearest code but can only see a broader refactoring as a way out of that and would like feedback if this approach is even something worth doing before undertaking that.

This fixes #862.

@seifertm

seifertm commented 3 months ago

Thanks! I'll have a look at it right away.

It seems like this issue should be tackled as part of the v0.24 release. Version 0.24 is supposed to allow users of v0.21 and v0.23 to upgrade to a common version before overrides of the event_loop fixture is removed. The removal will allow a bunch of refactoring and simplification of the code. Therefore, I think we should wait with larger refactorings until v0.24 is out.

minrk commented 3 months ago

I ran into this issue myself in https://github.com/jupyterhub/jupyterhub/pull/4664, and this PR seems to mostly fix it.

The one fixture that can still reproduce it for me is adding an autouse=True function-scoped async fixture, combined with module-scoped loop:

@pytest_asyncio.fixture(loop_scope="module", autouse=True)
async def cleanup_after():
    yield
    print("async cleanup after")

run in this sample test environment:

> pytest
================================= test session starts =================================
platform darwin -- Python 3.11.9, pytest-8.3.2, pluggy-1.5.0 -- /Users/minrk/.virtualenvs/pyt-asy/bin/python
cachedir: .pytest_cache
rootdir: /Users/minrk/dev/jpy/temp/pytest-atest
configfile: pytest.ini
plugins: asyncio-0.24.0a1.dev1+gee75d65
asyncio: mode=Mode.AUTO, default_loop_scope=module
collected 2 items
run-last-failure: rerun previous 1 failure first

test_a.py::test_a ERROR                                                         [ 50%]
test_b.py::test_a PASSED                                                        [100%]

======================================= ERRORS ========================================
______________________________ ERROR at setup of test_a _______________________________
file /Users/minrk/dev/jpy/temp/pytest-atest/test_a.py, line 5
  async def test_a(current_loop):
      assert current_loop is asyncio.get_running_loop()
file /Users/minrk/dev/jpy/temp/pytest-atest/conftest.py, line 8
  @pytest_asyncio.fixture(loop_scope="module", autouse=True)
  async def cleanup_after():
      yield
      print("cleaning up after each test")
E       fixture 'test_b.py::<event_loop>' not found
>       available fixtures: _session_event_loop, cache, capfd, capfdbinary, caplog, capsys, capsysbinary, cleanup_after, current_loop, doctest_namespace, event_loop, event_loop_policy, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, test_a.py::<event_loop>, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory, unused_tcp_port, unused_tcp_port_factory, unused_udp_port, unused_udp_port_factory
>       use 'pytest --fixtures [testpath]' for help on them.

/Users/minrk/dev/jpy/temp/pytest-atest/conftest.py:8
=============================== short test summary info ===============================
ERROR test_a.py::test_a
============================= 1 passed, 1 error in 0.01s ==============================
seifertm commented 3 months ago

@cstruct I managed to get parametrization tests of the _eventloop fixture working, by adding the event_loop argument injection next to injection of the request arg. I ended up having tests reference (non-autouse) fixtures from completely different modules, which I didn't manage to solve.

Therefore, I combined your idea of using request.getfixturevalue(…) in the fixture synchronizers with determining the event loop at runtime (see #668, #669). Back then, I had to close the PR, because it relied on the fact the event_loop is no longer an explicit argument in fixtures and tests.

It looks like we got it working, thanks to your efforts. I left the commit history as is, but most of it should be squashed in my opinion.

The new code creates problems with pytest 7, though. I cannot say why this is the case at the moment.

@minrk Thanks for raising this. I haven't checked your example together with the most recent changes, yet.

minrk commented 3 months ago

jupyterhub fixtures are now working with f228b0d

cstruct commented 3 months ago

Thank you @seifertm! I played whack-a-mole with failing tests this morning to no avail so I am very happy you picked this up.

seifertm commented 3 months ago

Apparently, the changes require at least pytest 8.2. Otherwise, the test suite emits ResourceWarnings. This is possibly related to https://github.com/pytest-dev/pytest/issues/1489

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.30%. Comparing base (f45aa18) to head (171da4f).

Files Patch % Lines
pytest_asyncio/plugin.py 92.10% 1 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #906 +/- ## ========================================== - Coverage 91.50% 91.30% -0.20% ========================================== Files 2 2 Lines 506 506 Branches 100 98 -2 ========================================== - Hits 463 462 -1 - Misses 25 26 +1 Partials 18 18 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cstruct commented 3 months ago

Thank you for your hard work @seifertm! It was a pleasure to be involved!