-
Notifications
You must be signed in to change notification settings - Fork 162
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
Preprocess async fixtures for each event loop #906
Conversation
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 |
I ran into this issue myself in jupyterhub/jupyterhub#4664, and this PR seems to mostly fix it. The one fixture that can still reproduce it for me is adding an @pytest_asyncio.fixture(loop_scope="module", autouse=True)
async def cleanup_after():
yield
print("async cleanup after") run in this sample test environment:
|
@cstruct I managed to get parametrization tests of the event_loop fixture working, by adding the Therefore, I combined your idea of using 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. |
f228b0d
to
e7b5c69
Compare
jupyterhub fixtures are now working with f228b0d |
32704d2
to
0bf2566
Compare
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. |
0aa2854
to
2044e56
Compare
Apparently, the changes require at least pytest 8.2. Otherwise, the test suite emits ResourceWarnings. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion it's fine to require pytest 8.2 or newer. If it solves a bug, I'd rather do that than trying to work around whatever issue is causing ResourceWarnings.
…n reused in other modules. Async fixture synchronizers now choose the event loop for the async fixutre at runtime rather than relying on collection-time information. This fixes pytest-dev#862.
…ert_async_functions_to_subclass.
Thank you for your hard work @seifertm! It was a pleasure to be involved! |
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