Fix event_loop_policy parametrization scope#1454
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1454 +/- ##
==========================================
- Coverage 94.50% 94.38% -0.13%
==========================================
Files 2 2
Lines 510 534 +24
Branches 62 69 +7
==========================================
+ Hits 482 504 +22
- Misses 22 23 +1
- Partials 6 7 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
golikovichev
left a comment
There was a problem hiding this comment.
Approach looks clean. Dropping autouse=True from the event_loop_policy fixture and gating its injection on pytest_generate_tests only when the test uses asyncio fixtures (or is async itself) is the right shape to fix #796 without breaking the loop-policy parametrization use case.
Two observations from a downstream-user read:
_uses_asyncio_fixtureswalksmetafunc._arg2fixturedefsand short-circuits on first match. In AUTO mode it falls back to_is_coroutine_or_asyncgen, which is what catches sync tests that happen to request async fixtures - exactly the gap #796 surfaced. Worth keeping the AUTO branch as-is even though it adds a per-fixture check.- The negative-case regression test
test_parametrized_loop_policy_does_not_parametrize_sync_testsis paired withtest_parametrized_loop_policy_parametrizes_sync_tests_with_async_fixtures(positive case). Good coverage pattern.
CI green across the full matrix (Py 3.10-3.14 + Windows + pytest main). Lint + mypy + pyright also clean per PR body.
Not a maintainer, leaving this as a non-blocking review.
| ) | ||
|
|
||
|
|
||
| def _add_fixture_to_metafunc(metafunc: pytest.Metafunc, fixture_name: str) -> None: |
There was a problem hiding this comment.
I think this changes pytest's internals and breaks valid usage. For example, on main, this passes.
$ cat >mre.py <<'# EOF'
import asyncio
import pytest
@pytest.fixture(params=[asyncio.DefaultEventLoopPolicy(), asyncio.DefaultEventLoopPolicy()])
def policy(request):
return request.param
@pytest.fixture
def event_loop_policy(policy):
return policy
@pytest.mark.asyncio
async def test_async():
pass
async def test_sync():
pass
# EOF
$ pytest mre.py
==================================================== test session starts ====================================================
platform darwin -- Python 3.13.0, pytest-9.0.3, pluggy-1.6.0
rootdir: /Users/tjkuson/workspace/pytest-asyncio
configfile: pyproject.toml
plugins: asyncio-1.4.0a3.dev24
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
collected 2 items
mre.py EE [100%]
========================================================== ERRORS ===========================================================
_______________________________________________ ERROR at setup of test_async ________________________________________________
The requested fixture has no parameter defined for test:
mre.py::test_async
Requested fixture 'policy' defined in:
mre.py:6
Requested here:
.venv/lib/python3.13/site-packages/_pytest/fixtures.py:627
________________________________________________ ERROR at setup of test_sync ________________________________________________
The requested fixture has no parameter defined for test:
mre.py::test_sync
Requested fixture 'policy' defined in:
mre.py:6
Requested here:
.venv/lib/python3.13/site-packages/_pytest/fixtures.py:627
===================================================== 2 errors in 0.02s =====================================================
There was a problem hiding this comment.
@tjkuson Have your concerns been addressed by the latest commit? If not, can you elaborate?
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
@seifertm The specific issue is addressed, but I still feel a bit concerned about the intervention in pytest's internals and possible breakage for users. For instance, event_loop_policy injected late, meaning users can no longer do this:
def pytest_collection_modifyitems(items):
for item in items:
if "event_loop_policy" in item.fixturenames:
item.add_marker(pytest.mark.uses_loop_policy)It's admittedly low-level and I doubt many users are doing things like this, but it's still IMO valid pytest usage of a documented and public API. I don't have any ideas at the moment about how to resolve this.
There was a problem hiding this comment.
Thanks, this is a valid compatibility concern. I pushed 2bdec76 to keep the injected event_loop_policy name visible before user pytest_collection_modifyitems hooks run, while still avoiding the old autouse behavior for plain sync tests.
I added test_injected_loop_policy_is_visible_during_collection_modifyitems, which mirrors the hook pattern from your example: marked async items get the marker via item.fixturenames, and the plain sync test does not.
Local verification before pushing:
UV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uv run --extra testing python -m pytest tests/markers/test_function_scope.py::test_injected_loop_policy_is_visible_during_collection_modifyitems -q->1 passedUV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uv run --extra testing python -m pytest tests/markers/test_function_scope.py -q->17 passedUV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uv run --extra testing python -m pytest tests/test_set_event_loop.py -q->61 passedUV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uv run --extra testing python -m pytest -q->304 passedUV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uvx ruff check pytest_asyncio/plugin.py tests/markers/test_function_scope.py->All checks passed!git diff --check
golikovichev
left a comment
There was a problem hiding this comment.
Good catch with the MRE. My read was too superficial - I focused on the autouse change and the regression test pair, but missed that _add_fixture_to_metafunc injecting event_loop_policy breaks valid fixture chains where event_loop_policy depends on another parametrized fixture (like your policy). The patch shifts the parametrization origin out from under pytest's resolver.
Leaving this to the PR author and maintainers.
golikovichev
left a comment
There was a problem hiding this comment.
Re-reviewed after 8305b3a. The new _add_fixture_to_metafunc walks the closure via fixturemanager.getfixtureclosure() instead of attaching just the named fixture, so parametrize chains like event_loop_policy(policy) from @tjkuson's MRE are now preserved end to end.
The new test test_parametrized_loop_policy_can_depend_on_parametrized_fixture mirrors the MRE shape directly (parametrized policy fixture, event_loop_policy(policy) depending on it, both async and sync tests), so the regression is pinned. The two surrounding sync-vs-async parametrization tests cover the adjacent cases I would have asked for, so I do not have anything to add.
Sorry again for missing this in my first read - your MRE made the failure mode obvious, and the fix landed cleanly.
gmail-precheck-done
|
Pushed cd89044 to cover the remaining event loop policy fixture discovery paths from the Codecov report. Added coverage for auto-mode regular async fixtures and strict-mode unmarked async tests that still need the policy fixture closure. Verified locally:
|
seifertm
left a comment
There was a problem hiding this comment.
@puneetdixit200 I appreciate the initiative. The proposed changes look reasonable. The PR solves a long-standing issue in pytest-asyncio.
I wish you had reached out before authoring the PR, though. The policy system has been deprecated in Python and will consequently be deprecated in pytest-asyncio. The latest pytest-asyncio version (v1.4.0a2, stable release today) already contains an approach that allows parametrizing tests with different loops, leaving the policy system deprecated. I'm not saying we cannot merge this, but I think your efforts would have been better spent on a different issue.
If you could have a look at my comments, that would be great :) I suggest to first read through my text comments and leave the code suggestions for last
| pass | ||
| """)) | ||
| result = pytester.runpytest("--asyncio-mode=strict") | ||
| result.assert_outcomes(failed=2) |
There was a problem hiding this comment.
This test case parametrizes the loop policy and provides an async fixture. Pytest-asyncio is run in strict mode and test_unmarked_async is not marked, so it should never execute in my understanding.
Can you explain (1) why we expect two test cases to run, (2) why they should fail, and how the test contributes to the functionality of this PR?
| ) | ||
|
|
||
|
|
||
| def _add_fixture_to_metafunc(metafunc: pytest.Metafunc, fixture_name: str) -> None: |
There was a problem hiding this comment.
@tjkuson Have your concerns been addressed by the latest commit? If not, can you elaborate?
|
@golikovichev I do appreciate reviews from non-maintainers. Thank you. |
|
Pushed Changes:
Verification:
|
63cbfa3 to
e450cc8
Compare
|
Follow-up on the latest push: current head Verified locally before pushing:
The GitHub check suite is now green on |
|
The author seems unwilling to answer questions about the submitted code. They resolve review comments prematurely, instead of engaging in a discussion about the underlying problem. At the same time, they don't interact with the many code suggestions provided by reviewers at all and seem to push code for no obvious reason. My impression is that I'm not talking to a human, but solely to a bot. I go out of my way to pretend this is not the case, since OP's account is not labelled as such. There appears to be no consideration whatsoever for the time invested by maintainers. The wanton disregard for other people's time–by this PR and other recent ones–reached a point where I find this behavior and communication style personally offensive. I'm simply unwilling to invest further time into this PR. Pytest-asyncio needs a contribution guide. |
Summary
Fixes #796
Tests