-
Notifications
You must be signed in to change notification settings - Fork 149
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
warn if asyncio test requests async pytest fixture in strict mode #979
base: main
Are you sure you want to change the base?
warn if asyncio test requests async pytest fixture in strict mode #979
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #979 +/- ##
==========================================
- Coverage 91.15% 90.83% -0.33%
==========================================
Files 2 2
Lines 509 513 +4
Branches 64 66 +2
==========================================
+ Hits 464 466 +2
- Misses 27 28 +1
- Partials 18 19 +1 ☔ View full report in Codecov by Sentry. |
actually, it's not that unlikely that this will break tests for some users if they're using |
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.
I'm loosely following the linked pytest issue. I really appreciate all the work you've put into this and I will try to support the initiative from the pytest-asyncio side.
It would be great, if you could also add a corresponding entry to docs/reference/changelog.rst, since this is a user-facing change.
This looks great, thank you! You mentioned in pytest-dev/pytest#12930 (comment) that this change may become redundant once the same check is in place in pytest. I'm generally okay with having the check in pytest-asyncio, too, at least temporarily. At some point, pytest-asyncio will bump the minimum required pytest version to a version where this check is included in pytest and we can remove it here. I'd like to wait for a decision on your pytest PR before merging (whether you go with a deprecation or plain error), but other than that it looks good to me. Thanks for the effort! |
partially fixes pytest-dev/pytest#10839, see pytest-dev/pytest#10839 (comment)
It got a bit messy to implement, and relies on non-public API in
_fixtureinfo
, so I'm not 100% that this is the best way to approach it. Another approach would be to try and catch this on collection, but then you end up having to handle overriding fixtures and the like.It's also possible that this breaks tests for end users, but if they really do want an unawaited object from a fixture they can resolve it in various ways by returning an
Awaitable
from their fixture.