-
Notifications
You must be signed in to change notification settings - Fork 159
warn if asyncio test requests async pytest fixture in strict mode #979
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
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. 🚨 Try these New Features:
|
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! |
Looks like we're going with a warning in the upstream PR. |
fix test make it raise PytestDeprecationWarning instead add changelog entry docs: Changed release date of v0.25.0 to UNRELEASED.
3fedb5a
to
b208df6
Compare
I adjusted the release date of v0.25.0 to UNRELEASED and squashed the commits. |
Soooooo...how am I supposed to get the semantics that this now-deprecated approach provided on modern pytest-asyncio? Previously, when in strict mode, I could mark it as |
fixtures are intended to be used for setup/teardown of tests, so if you're using it to provide awaitables to your tests my first instinct is that you should perhaps rethink the structure of your tests - make the fixtures standalone helper functions, or if the fixtures require additional setup before they can be run then make them depend on some other fixtures. But if you simply do want to suppress it you indeed do need to wrap it (though you can wrap in a sync func), as documented in the yet-unreleased pytest 8.4 docs https://docs.pytest.org/en/latest/deprecations.html#sync-test-depending-on-async-fixture def raw_awaitable(async_fixture):
def inner():
return async_fixture()
return inner
@pytest.fixture
@raw_awaitable
async def my_fixture():
return 5 |
The problem in my case is that my weird fixtures pull in other fixtures--mostly monkeypatch. I think I can restructure it so I just pass that as an argument to these instead of being fixtures themselves, but it was a lot of warnings so I haven't had the chance to go over all of it yet. I think at least one of them does teardown at the end though. I have to keep going through the list |
Looks like none of them did teardown, so just replacing the fixture usages with calls worked. |
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.