Skip to content
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

DeprecationWarning if sync test requests async fixture #12930

Merged
merged 24 commits into from
Nov 17, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Oct 31, 2024

Fixes #10839, or at least the parts that pytest can handle. It also fixes agronholm/anyio#789
pytest-trio already failed sync tests that were marked with pytest.mark.trio, this will now make that check redundant and we instead mark the test as erroring.

I'm not sure if we care to have a deprecation period for sync-test + autouse-async-fixture, while users may currently have test suites that pass they're very close to shooting themselves in the foot and they will be riddled with RuntimeWarning: coroutine [...] was never awaited

I briefly tested the fix against pytest-asyncio, pytest-trio and anyio's pytest plugin in case any of them would cause false alarms with actual async tests, but as far as I can tell there were no problems. But maybe @agronholm or @seifertm would like to confirm.

If a sync test depends on a sync fixture which itself depends on an async fixture the error message is perhaps slightly confusing. It might be enough to simply name the async fixture in the error message though.

TODO:

  • write changelog entry

@jakkdl
Copy link
Member Author

jakkdl commented Oct 31, 2024

I'm not sure if we care to have a deprecation period for sync-test + autouse-async-fixture, while users may currently have test suites that pass they're very close to shooting themselves in the foot and they will be riddled with RuntimeWarning: coroutine [...] was never awaited

one of the footguns being that this fixture never runs, even for async tests, because the unawaited value is cached by the sync test

import pytest

@pytest.fixture(autouse=True, scope='session')
async def async_fixture():
    assert False

def test_foo():
    ...

@pytest.mark.anyio
async def test_foo_async():
    ...

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Nov 1, 2024
@agronholm
Copy link

Does this mean I won't be able to support async fixtures in sync tests even if I want to?

inspect.iscoroutinefunction(fixturedef.func)
or inspect.isasyncgenfunction(fixturedef.func)
)
):
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure this is the place to do the check, and am also not 100% it can't be triggered by a sync fixture requesting an async fixture. But for the latter I think it's covered by the self.scope == "function" check, where if self is a fixture requesting another fixture it's because it's higher-scoped.
So while this appears to function robustly, it might be making somewhat sketchy assumptions.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is correct, and I also see a test for this, so I guess we are good.

@jakkdl
Copy link
Member Author

jakkdl commented Nov 1, 2024

Does this mean I won't be able to support async fixtures in sync tests even if I want to?

I think it might be possible to get around it if you wrap the sync test in an async wrapper on collection, but if that's not desired and you want to support that use-case it might be possible to move the check somewhere it could be overriden

@jakkdl
Copy link
Member Author

jakkdl commented Nov 1, 2024

hrm. This breaks hypothesis, since @given wraps async tests into sync tests.

@jakkdl
Copy link
Member Author

jakkdl commented Nov 1, 2024

Requesting pytest plugins to update (given there's an easy way to do so) might be reasonable, but I realized that almost any approach will break on anything that looks like this

@pytest.fixture
async def fix():
  return 1

def test_fix(fix):
  assert 1 == asyncio.run(fix)

we could perhaps try to catch RuntimeWarning: coroutine ... was never awaited and make it error tests even if they don't run with -Werror, but that only gets run on garbage collection... see also #10404

So yeah if implementing this it'd need to be in a way that's quite easy to override. Maybe something like @pytest.mark.allow_async_fixture, and have the error message suggest adding it.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jakkdl for tackling this topic, it takes quite a time to look up at all the existing plugins, understanding the semantics, etc.

Please take a look at my comments.

inspect.iscoroutinefunction(fixturedef.func)
or inspect.isasyncgenfunction(fixturedef.func)
)
):
Copy link
Member

Choose a reason for hiding this comment

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

I think this is correct, and I also see a test for this, so I guess we are good.

if fixturedef._autouse:
warnings.warn(
PytestRemovedIn9Warning(
"Sync test requested an async fixture with autouse=True. "
Copy link
Member

Choose a reason for hiding this comment

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

Add the test name to "Sync test '{name}'" and the fixture name to "async fixture '{name}'" in the phrase here to help users understand the problem better.

We also should add an entry to "deprecations", with the rationale for this and guiding users on how to update their code (installing plugins, changing the async fixture, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

Was confused for a second what you meant with "installing plugins". The way the error for async test functions handles it is by printing a long message recommending async test plugins, maybe this message should do the same. Not sure it has much of a place in the deprecations doc - if a user has a test suite that currently works the fix almost surely whouldn't be to install a new async test plugin.

changelog/10839.deprecation.rst Outdated Show resolved Hide resolved
changelog/10839.improvement.rst Outdated Show resolved Hide resolved
src/_pytest/fixtures.py Outdated Show resolved Hide resolved
testing/acceptance_test.py Outdated Show resolved Hide resolved
src/_pytest/fixtures.py Show resolved Hide resolved
src/_pytest/fixtures.py Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member Author

jakkdl commented Nov 6, 2024

@nicoddemus what do you think about @pytest.mark.allow_async_fixture? I think requiring e.g. hypothesis to wrap all async fixtures just to work around this warning would be somewhat onerous

@nicoddemus
Copy link
Member

@nicoddemus what do you think about @pytest.mark.allow_async_fixture? I think requiring e.g. hypothesis to wrap all async fixtures just to work around this warning would be somewhat onerous

I'm hesitant to add a user-facing mark because of just this warning -- but of course we don't want to force hypothesis (and perhaps other test suites with similar customizations out there) to wrap their async fixtures manually...

Not sure, perhaps @Zac-HD can chime in.

@jakkdl
Copy link
Member Author

jakkdl commented Nov 6, 2024

@nicoddemus what do you think about @pytest.mark.allow_async_fixture? I think requiring e.g. hypothesis to wrap all async fixtures just to work around this warning would be somewhat onerous

I'm hesitant to add a user-facing mark because of just this warning -- but of course we don't want to force hypothesis (and perhaps other test suites with similar customizations out there) to wrap their async fixtures manually...

Not sure, perhaps @Zac-HD can chime in.

It would be possible to automatically wrap async fixtures upon collection, but that would get quite messy if a test suite has both hypothesis and non-hypothesis tests. And if plugin writers start unilaterally wrapping all their fixtures to look sync then we're defeating the point of this warning in a lot of cases in the first places.

If we don't want a public @pytest.mark, maybe an attribute on pytest.Function (or pytest.Item?) that can be set in pytest_runtest_call to disable the check?

@jakkdl jakkdl changed the title Error if sync test requests async fixture DeprecationWarning if sync test requests async fixture Nov 6, 2024
@Zac-HD
Copy link
Member

Zac-HD commented Nov 6, 2024

re: impact on Hypothesis - unclear overall. To date we've avoided having any async support, by offering plugins the ability to insert their make-it-sync wrapper between the user's test function and Hypothesis itself. We also nudge users away from function-scoped fixtures, because they're only invoked once for all of the calls we make; and early async fixtures were ~all function-scoped.

Maybe we end up having Hypothesis return an async wrapper function if we're wrapping an async test, keeping the other logic the same? That'd be pretty painful to implment though, and make tracebacks worse for all our current use-cases...

@jakkdl
Copy link
Member Author

jakkdl commented Nov 6, 2024

To date we've avoided having any async support, by offering plugins the ability to insert their make-it-sync wrapper between the user's test function and Hypothesis itself.

I'll look into this make-it-sync wrapper and see if it can hook into this check as well.

@jakkdl
Copy link
Member Author

jakkdl commented Nov 10, 2024

To date we've avoided having any async support, by offering plugins the ability to insert their make-it-sync wrapper between the user's test function and Hypothesis itself.

I'll look into this make-it-sync wrapper and see if it can hook into this check as well.

okay so hypothesis+async plugins handle this by adding an .hypothesis object to pytest.Function and messing around with .hypothesis.inner_test. Pytest obviously can't introspect that. I'm guessing there's good reasons why async plugins don't directly modify pytest.Function.obj? Primarily perhaps that's it not really documented as public API.
So yeah what's happening here is that this PR makes a strict assumption that we'll call pytest.Function.obj - but given the existence pytest_runtest_call that's not a great assumption.

..

hmm, thinking deeper about the problem there's perhaps a more fundamental approach we can take. The reason #10839 doesn't give an unawaited coroutine warning is because pytest internals treats fixtures as generators, calling send & close on them, and this suppresses the warning on garbage collection. For some reason the stdlib does not make use of __await__/__aiter__ to detect if a coroutine was unawaited - but we could add a wrapper to pytest.Fixturedef.func() if the underlying fixture function is async that detects these, and make it raise a warning/error if the fixture isn't properly awaited / set up+torn down.

Maybe there's some other possible downside to this (is it valid to just ... refuse to set up a fixture??), but I think that would be a more robust approach and shouldn't require any changes to plugins. Though will have to make sure the wrapper is properly transparent for plugins that are inspecting fixturedef.func.

It wouldn't directly catch the footgun of a sync func requesting a larger-scoped async fixture, where it will work depending on if an async func has previously requested the fixture, but async plugins can optionally handle this, and it will most likely work as the user expects - it's just very fragile.

nicoddemus and others added 6 commits November 11, 2024 15:09
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.6.9 → v0.7.2](astral-sh/ruff-pre-commit@v0.6.9...v0.7.2)
- [github.com/adamchainz/blacken-docs: 1.19.0 → 1.19.1](adamchainz/blacken-docs@1.19.0...1.19.1)
- [github.com/pre-commit/mirrors-mypy: v1.11.2 → v1.13.0](pre-commit/mirrors-mypy@v1.11.2...v1.13.0)
- [github.com/tox-dev/pyproject-fmt: 2.3.1 → v2.5.0](tox-dev/pyproject-fmt@2.3.1...v2.5.0)
- [github.com/asottile/pyupgrade: v3.18.0 → v3.19.0](asottile/pyupgrade@v3.18.0...v3.19.0)

[mypy] Remove useless noqa, add noqa for new false positives

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>
Co-authored-by: pytest bot <pytestbot@users.noreply.github.com>
…12951)

Bumps [django](https://github.com/django/django) from 5.1.2 to 5.1.3.
- [Commits](django/django@5.1.2...5.1.3)

---
updated-dependencies:
- dependency-name: django
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
dependabot bot and others added 5 commits November 11, 2024 15:09
…ytest-dev#12953)

Bumps [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) from 1.10.3 to 1.12.2.
- [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases)
- [Commits](pypa/gh-action-pypi-publish@v1.10.3...v1.12.2)

---
updated-dependencies:
- dependency-name: pypa/gh-action-pypi-publish
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ter any hooks (from async plugins) has had a chance to resolve the awaitable
@jakkdl
Copy link
Member Author

jakkdl commented Nov 11, 2024

(sorry for messy commit history from rebase/merge of main)

okay this is a much better way of doing it. Turns out I didn't need a wrapper at all and could instead move the check to pytest_fixture_setup which will run after any async plugins have had a chance to handle async fixtures. I tried this against the test suites of pytest-asyncio, pytest-trio, anyio and some of hypothesis - with zero failures other than warnings where expected. This means we could go back to directly raising an error, as I think the only somewhat likely case where this will trigger "incorrectly" is if a test suite has autouse async fixtures they don't care about being ignored in their sync tests.

Idk how much effort to put into the message, could customize it for any combination of autouse & coroutine/asyncgen and could also tell people about available async plugins or direct to the pytest_fixture_setup hook.

This now also catches async tests incorrectly handling async fixtures, making pytest-dev/pytest-asyncio#979 somewhat redundant (though might still keep it for more helpful message for users).

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

This is looking good - thanks @jakkdl!

src/_pytest/fixtures.py Outdated Show resolved Hide resolved
src/_pytest/fixtures.py Outdated Show resolved Hide resolved
testing/acceptance_test.py Show resolved Hide resolved
doc/en/deprecations.rst Outdated Show resolved Hide resolved
@Zac-HD Zac-HD added this to the 8.4 milestone Nov 13, 2024
src/_pytest/compat.py Outdated Show resolved Hide resolved
doc/en/deprecations.rst Outdated Show resolved Hide resolved
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Alright, I think we're ready to merge! Ping @nicoddemus to confirm?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM thanks @jakkdl for the great work and @Zac-HD for the review/chiming in about Hypothesis.

Left a few last minor requests, please take a look.

okay so hypothesis+async plugins handle this by adding an .hypothesis object to pytest.Function and messing around with .hypothesis.inner_test. Pytest obviously can't introspect that.

Hmm not sure, if that's all it would take for us to handle this without bothering users and Hypothesis' mainteinares, I would be up for it! Is a simple check, Hypothesis is a well known and widely used library, being "partners" with pytest for a long time, I would consider that for sure if it would mean we could avoid the warning with Hypothesis... but is not clear to me if this is still relevant under the new implementation or not.


You can also make use of `pytest_fixture_setup` to handle the coroutine/asyncgen before pytest sees it - this is the way current async pytest plugins handle it.

If a user has an async fixture with ``autouse=True`` in their ``conftest.py``, or in a file where they also have synchronous tests, they will also get this warning. We strongly recommend against this practice, and they should restructure their testing infrastructure so the fixture is synchronous or to separate the fixture from their synchronous tests. Note that the `anyio pytest plugin <https://anyio.readthedocs.io/en/stable/testing.html>`_ has some support for sync test + async fixtures currently.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should be more explicit why we recommend against this practice in core pytest? I mean this is something a plugin could support correctly in theory?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's some fundamental issues with how to handle it correctly, currently anyio will suspend the async runloop during execution of the sync test. And we also can't simply wrap the test as an async task, as that will lack checkpoints and will hug the runloop. You probably could run it in a separate thread, but yeah anyio doesn't support that currently.

src/_pytest/fixtures.py Outdated Show resolved Hide resolved
src/_pytest/fixtures.py Outdated Show resolved Hide resolved
jakkdl

This comment was marked as resolved.

@jakkdl
Copy link
Member Author

jakkdl commented Nov 17, 2024

LGTM thanks @jakkdl for the great work and @Zac-HD for the review/chiming in about Hypothesis.

Left a few last minor requests, please take a look.

okay so hypothesis+async plugins handle this by adding an .hypothesis object to pytest.Function and messing around with .hypothesis.inner_test. Pytest obviously can't introspect that.

Hmm not sure, if that's all it would take for us to handle this without bothering users and Hypothesis' mainteinares, I would be up for it! Is a simple check, Hypothesis is a well known and widely used library, being "partners" with pytest for a long time, I would consider that for sure if it would mean we could avoid the warning with Hypothesis... but is not clear to me if this is still relevant under the new implementation or not.

Yeah this new implementation has no problem with hypothesis - or any plugins as far as I know, so don't need to go down that road :)

@jakkdl
Copy link
Member Author

jakkdl commented Nov 17, 2024

just need a brief pass at the updated deprecation docs and then we should be good to merge :)

@Zac-HD
Copy link
Member

Zac-HD commented Nov 17, 2024

Looks great - let's ship it!

@Zac-HD Zac-HD merged commit 5611bdd into pytest-dev:main Nov 17, 2024
29 checks passed
@jakkdl jakkdl deleted the sync_test_async_fixture branch November 18, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
5 participants