Skip to content

Fix event_loop_policy parametrization scope#1454

Closed
puneetdixit200 wants to merge 5 commits into
pytest-dev:mainfrom
puneetdixit200:fix/796-event-loop-policy
Closed

Fix event_loop_policy parametrization scope#1454
puneetdixit200 wants to merge 5 commits into
pytest-dev:mainfrom
puneetdixit200:fix/796-event-loop-policy

Conversation

@puneetdixit200
Copy link
Copy Markdown

Summary

  • stop making the default event_loop_policy fixture autouse
  • request event_loop_policy only for pytest-asyncio tests and tests using asyncio fixtures, so plain sync tests are not parametrized
  • add regression coverage for plain sync tests and sync tests that use asyncio fixtures
  • avoid re-importing pytest_asyncio in one subprocess test under -W error; the entry point already loads the plugin

Fixes #796

Tests

  • .venv/bin/python -m pytest -q
  • .venv/bin/python -m pytest tests/test_set_event_loop.py::test_asyncio_run_after_async_fixture_does_not_leak_loop tests/markers/test_function_scope.py::test_parametrized_loop_policy_does_not_parametrize_sync_tests tests/markers/test_function_scope.py::test_parametrized_loop_policy_parametrizes_sync_tests_with_async_fixtures -q
  • .venv/bin/python -m pytest tests/markers/test_function_scope.py tests/markers/test_module_scope.py tests/markers/test_class_scope.py tests/markers/test_session_scope.py tests/test_loop_factory_parametrization.py tests/test_asyncio_fixture.py -q
  • .venv/bin/python -m ruff check pytest_asyncio/plugin.py tests/markers/test_function_scope.py tests/test_set_event_loop.py
  • .venv/bin/python -m mypy --python-version 3.13 pytest_asyncio/
  • .venv/bin/python -m pyright --pythonpath .venv/bin/python --pythonversion 3.13 pytest_asyncio/
  • git diff --check

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 21, 2026

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.38%. Comparing base (9190447) to head (e450cc8).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
pytest_asyncio/plugin.py 92.59% 1 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@golikovichev golikovichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. _uses_asyncio_fixtures walks metafunc._arg2fixturedefs and 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.
  2. The negative-case regression test test_parametrized_loop_policy_does_not_parametrize_sync_tests is paired with test_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.

Comment thread pytest_asyncio/plugin.py
)


def _add_fixture_to_metafunc(metafunc: pytest.Metafunc, fixture_name: str) -> None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 =====================================================

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjkuson Have your concerns been addressed by the latest commit? If not, can you elaborate?

This comment was marked as low quality.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 passed
  • UV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uv run --extra testing python -m pytest tests/markers/test_function_scope.py -q -> 17 passed
  • UV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uv run --extra testing python -m pytest tests/test_set_event_loop.py -q -> 61 passed
  • UV_CACHE_DIR=/private/tmp/pytest_asyncio_1454_uv_cache uv run --extra testing python -m pytest -q -> 304 passed
  • UV_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

Copy link
Copy Markdown

@golikovichev golikovichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@golikovichev golikovichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@puneetdixit200
Copy link
Copy Markdown
Author

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:

  • coverage run -m pytest -q (304 passed)
  • coverage report -m pytest_asyncio/plugin.py (the previously missing new lines are now covered)
  • python -m pytest tests/markers/test_function_scope.py tests/test_set_event_loop.py -q (78 passed)
  • ruff check tests/markers/test_function_scope.py
  • git diff --check

Copy link
Copy Markdown
Contributor

@seifertm seifertm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Comment thread tests/markers/test_function_scope.py Outdated
Comment thread tests/markers/test_function_scope.py Outdated
pass
"""))
result = pytester.runpytest("--asyncio-mode=strict")
result.assert_outcomes(failed=2)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread tests/markers/test_function_scope.py
Comment thread tests/markers/test_function_scope.py Outdated
Comment thread tests/markers/test_function_scope.py Outdated
Comment thread tests/markers/test_function_scope.py Outdated
Comment thread tests/markers/test_function_scope.py Outdated
Comment thread tests/markers/test_function_scope.py Outdated
Comment thread tests/markers/test_function_scope.py Outdated
Comment thread pytest_asyncio/plugin.py
)


def _add_fixture_to_metafunc(metafunc: pytest.Metafunc, fixture_name: str) -> None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjkuson Have your concerns been addressed by the latest commit? If not, can you elaborate?

@seifertm
Copy link
Copy Markdown
Contributor

@golikovichev I do appreciate reviews from non-maintainers. Thank you.

@puneetdixit200
Copy link
Copy Markdown
Author

Pushed 63cbfa3 for the review feedback.

Changes:

  • Removed the strict-mode unmarked async branch from pytest_generate_tests; unmarked async tests are not managed by pytest-asyncio in strict mode, so they should not get event_loop_policy fixture injection.
  • Removed the remaining custom IDs from the Pytester loop policy fixtures.
  • Kept the simplified Pytester bodies from the prior cleanup commit.

Verification:

  • UV_CACHE_DIR=/tmp/pytest-asyncio-1454-uv-cache uv run --extra testing python -m pytest tests/markers/test_function_scope.py tests/test_set_event_loop.py -q (77 passed)
  • UV_CACHE_DIR=/tmp/pytest-asyncio-1454-uv-cache uv run --with ruff ruff check pytest_asyncio/plugin.py tests/markers/test_function_scope.py
  • UV_CACHE_DIR=/tmp/pytest-asyncio-1454-uv-cache uv run --extra testing coverage run -m pytest -q (303 passed)
  • UV_CACHE_DIR=/tmp/pytest-asyncio-1454-uv-cache uv run --extra testing coverage report -m pytest_asyncio/plugin.py
  • git diff --check

@puneetdixit200 puneetdixit200 force-pushed the fix/796-event-loop-policy branch from 63cbfa3 to e450cc8 Compare May 26, 2026 09:25
@puneetdixit200
Copy link
Copy Markdown
Author

Follow-up on the latest push: current head e450cc8 supersedes my earlier 63cbfa3 note. This head is test-only and leaves the plugin implementation from cd89044 intact while simplifying the review-covered Pytester cases.

Verified locally before pushing:

  • tests/markers/test_function_scope.py -q (16 passed)
  • tests/test_set_event_loop.py -q (61 passed)
  • ruff check tests/markers/test_function_scope.py
  • git diff --check

The GitHub check suite is now green on e450cc8 as well.

@golikovichev
Copy link
Copy Markdown

Thanks @seifertm, good to hear. Nice to see #1346 wrapped up via #1373 in the same window too.

@seifertm
Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parametrizing event_loop_policy parametrizes all tests

5 participants