DeprecationWarning if sync test requests async fixture #12930

jakkdl · 2024-10-31T16:19:20Z

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

…ture is autouse.

jakkdl · 2024-10-31T16:24:43Z

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():
    ...

agronholm · 2024-11-01T11:16:55Z

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

jakkdl · 2024-11-01T11:21:30Z

src/_pytest/fixtures.py

+                inspect.iscoroutinefunction(fixturedef.func)
+                or inspect.isasyncgenfunction(fixturedef.func)
+            )
+        ):


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.

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

jakkdl · 2024-11-01T11:24:23Z

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 · 2024-11-01T13:55:25Z

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

jakkdl · 2024-11-01T14:49:15Z

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.

nicoddemus

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.

nicoddemus · 2024-11-06T11:01:43Z

src/_pytest/fixtures.py

+                inspect.iscoroutinefunction(fixturedef.func)
+                or inspect.isasyncgenfunction(fixturedef.func)
+            )
+        ):


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

nicoddemus · 2024-11-06T11:03:16Z

src/_pytest/fixtures.py

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


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

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

nicoddemus · 2024-11-06T11:07:00Z

changelog/10839.improvement.rst

@@ -0,0 +1 @@
+Synchronous tests that request asynchronous fixtures will now error, instead of silently accepting an unawaited coroutine object as the fixture value.


Let's add an example, like the one you posted:

@pytest.fixture async def fix(): return 1 def test_fix(fix): assert 1 == asyncio.run(fix)

Is this a valid workaround for the error (did not test it)?

@pytest.fixture def fix(): async def func(): return 1 return func def test_fix(fix): assert 1 == asyncio.run(fix)

If so, we might want to mention this in the changelog as well in case a test suite is using this approach.

I added an example to deprecations.rst and linked to it from the changelog 👍

nicoddemus · 2024-11-06T11:08:42Z

src/_pytest/fixtures.py

@@ -805,7 +840,7 @@ def formatrepr(self) -> FixtureLookupErrorRepr:
        stack = [self.request._pyfuncitem.obj]
        stack.extend(map(lambda x: x.func, self.fixturestack))
        msg = self.msg
-        if msg is not None:
+        if msg is not None and len(stack) > 1:


This change does not seem necessary?

ah yes, this was from an earlier version where I raised a FixtureLookupError - although I think the change might be good on its own because if formatrepr is called with msg is not None and len(stack) <= 1 then the function will crash due to fspath never getting set. I could also change it to an assert instead

Suggested change

if msg is not None and len(stack) > 1:

if msg is not None:

assert len(stack) > 1

I might be missing something, but the only thing that happens inside this if is stack[:-1], which cannot fail even if stack is empty.

yes the problem is further down. If len(stack) == 1 when entering this if statement then this loop isn't entered

pytest/src/_pytest/fixtures.py

Lines 847 to 848 in 283db4e

for function in stack:

fspath, lineno = getfslineno(function)

so fspath isn't set, and this line fails

pytest/src/_pytest/fixtures.py

Line 880 in 283db4e

return FixtureLookupErrorRepr(fspath, lineno, tblines, msg, self.argname)

I slightly misspoke saying <= 1, if the stack is empty then it doesn't matter if the if-statement is entered or not, but given that it's primed that can't happen

pytest/src/_pytest/fixtures.py

Lines 840 to 841 in 283db4e

stack = [self.request._pyfuncitem.obj]

stack.extend(map(lambda x: x.func, self.fixturestack))

Perhaps a more informative check/assert would be directly on self.fixturestack

unless we go back to raising an error with the new implementation I'll separate this out to a separate PR

A separate PR would make it easier to review and merge each part 👍

(and we won't make this an error immediately)

testing/acceptance_test.py

nicoddemus · 2024-11-06T11:10:52Z

src/_pytest/fixtures.py

@@ -1666,6 +1706,7 @@ def _register_fixture(
            params=params,
            ids=ids,
            _ispytest=True,
+            _autouse=autouse,


I'm surprised FixtureDef didn't already have an attribute marking it as autouse... 👀

src/_pytest/fixtures.py

jakkdl · 2024-11-06T11:22:04Z

@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 · 2024-11-06T11:57:21Z

@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 · 2024-11-06T12:56:06Z

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

…dd section to deprecations.rst

Zac-HD · 2024-11-06T15:42:09Z

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 · 2024-11-06T20:52:41Z

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 · 2024-11-10T14:14:20Z

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.

Follow up to pytest-dev#12346.

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>

…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 · 2024-11-11T14:42:14Z

(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).

Zac-HD

This is looking good - thanks @jakkdl!

Zac-HD · 2024-11-12T04:19:29Z

src/_pytest/fixtures.py

@@ -805,7 +840,7 @@ def formatrepr(self) -> FixtureLookupErrorRepr:
        stack = [self.request._pyfuncitem.obj]
        stack.extend(map(lambda x: x.func, self.fixturestack))
        msg = self.msg
-        if msg is not None:
+        if msg is not None and len(stack) > 1:


A separate PR would make it easier to review and merge each part 👍

(and we won't make this an error immediately)

src/_pytest/fixtures.py

testing/acceptance_test.py

Zac-HD · 2024-11-12T04:27:04Z

doc/en/deprecations.rst

+
+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 has some support for sync test + async fixtures currently.


Let's provide a hyperlink to the relevant anyio docs here.

those docs doesn't say much about how to do it, and it's currently finicky agronholm/anyio#789 (comment)

but added link regardless.

I don't see a link here?

Zac-HD · 2024-11-13T18:22:23Z

src/_pytest/compat.py

@@ -44,6 +44,7 @@ class NotSetType(enum.Enum):


 def is_generator(func: object) -> bool:
+    # note: this also returns true for async generator functions


I just checked, and no, it doesn't! Just delete the comment?

(see #12960 for followups)

Zac-HD · 2024-11-13T18:28:59Z

doc/en/deprecations.rst

+
+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 has some support for sync test + async fixtures currently.


I don't see a link here?

Raise error if sync test relies on async fixture, and warn if the fix…

6728ec5

…ture is autouse.

jakkdl added 2 commits November 1, 2024 11:08

Merge remote-tracking branch 'origin/main' into sync_test_async_fixture

5c41d50

fix tests

8e100ea

jakkdl force-pushed the sync_test_async_fixture branch from 2b88268 to 8e100ea Compare November 1, 2024 10:58

add changelog

283db4e

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

jakkdl commented Nov 1, 2024

View reviewed changes

jakkdl mentioned this pull request Nov 5, 2024

Add a warning when setting up async fixtures #10839

Open

nicoddemus requested changes Nov 6, 2024

View reviewed changes

jakkdl and others added 3 commits November 6, 2024 14:02

improve warning message. Make it warn regardless of autouse or not. A…

5beab07

…dd section to deprecations.rst

Merge branch 'main' into sync_test_async_fixture

2d06ff2

fix test

0de5302

jakkdl changed the title ~~Error if sync test requests async fixture~~ DeprecationWarning if sync test requests async fixture Nov 6, 2024

nicoddemus and others added 6 commits November 11, 2024 15:09

Rename changelog entries to 'breaking' (pytest-dev#12942)

1891fed

Follow up to pytest-dev#12346.

Upgrade pylint version, activate all extensions

94dd153

[pylint dict-init-mutate] Initialize a dict right off for speed

b19fd52

[automated] Update plugin list (pytest-dev#12950)

987904c

Co-authored-by: pytest bot <pytestbot@users.noreply.github.com>

dependabot bot and others added 5 commits November 11, 2024 15:09

change implementation so the check happens in pytest_fixture_setup af…

c98ef2b

…ter any hooks (from async plugins) has had a chance to resolve the awaitable

fix test

cd3eb98

Merge branch 'main' into sync_test_async_fixture

2d9bb86

update docs/changelog

876cc2a

jakkdl requested a review from nicoddemus November 11, 2024 14:44

jakkdl mentioned this pull request Nov 11, 2024

Sync methods do not trigger async fixtures even if pytest.mark.anyio annotated agronholm/anyio#803

Open

2 tasks

seifertm mentioned this pull request Nov 11, 2024

warn if asyncio test requests async pytest fixture in strict mode pytest-dev/pytest-asyncio#979

Open

Zac-HD reviewed Nov 12, 2024

View reviewed changes

Zac-HD added this to the 8.4 milestone Nov 13, 2024

Zac-HD reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DeprecationWarning if sync test requests async fixture #12930

DeprecationWarning if sync test requests async fixture #12930

jakkdl commented Oct 31, 2024 •

edited

Loading

jakkdl commented Oct 31, 2024

agronholm commented Nov 1, 2024

jakkdl Nov 1, 2024

nicoddemus Nov 6, 2024

jakkdl commented Nov 1, 2024

jakkdl commented Nov 1, 2024

jakkdl commented Nov 1, 2024

nicoddemus left a comment

nicoddemus Nov 6, 2024

nicoddemus Nov 6, 2024

jakkdl Nov 6, 2024

nicoddemus Nov 6, 2024

jakkdl Nov 6, 2024

nicoddemus Nov 6, 2024

jakkdl Nov 6, 2024

nicoddemus Nov 6, 2024

jakkdl Nov 6, 2024

jakkdl Nov 11, 2024

Zac-HD Nov 12, 2024

nicoddemus Nov 6, 2024 •

edited

Loading

jakkdl commented Nov 6, 2024

nicoddemus commented Nov 6, 2024

jakkdl commented Nov 6, 2024

Zac-HD commented Nov 6, 2024

jakkdl commented Nov 6, 2024

jakkdl commented Nov 10, 2024 •

edited

Loading

jakkdl commented Nov 11, 2024

Zac-HD left a comment

Zac-HD Nov 12, 2024

Zac-HD Nov 12, 2024

jakkdl Nov 13, 2024

Zac-HD Nov 13, 2024

Zac-HD Nov 13, 2024

Zac-HD Nov 13, 2024

		@@ -0,0 +1 @@
		Synchronous tests that request asynchronous fixtures will now error, instead of silently accepting an unawaited coroutine object as the fixture value.

	if msg is not None and len(stack) > 1:
	if msg is not None:
	assert len(stack) > 1

	for function in stack:
	fspath, lineno = getfslineno(function)

	stack = [self.request._pyfuncitem.obj]
	stack.extend(map(lambda x: x.func, self.fixturestack))


		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 has some support for sync test + async fixtures currently.

		@@ -44,6 +44,7 @@ class NotSetType(enum.Enum):


		def is_generator(func: object) -> bool:
		# note: this also returns true for async generator functions

DeprecationWarning if sync test requests async fixture #12930

Are you sure you want to change the base?

DeprecationWarning if sync test requests async fixture #12930

Conversation

jakkdl commented Oct 31, 2024 • edited Loading

jakkdl commented Oct 31, 2024

agronholm commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Nov 1, 2024

jakkdl commented Nov 1, 2024

jakkdl commented Nov 1, 2024

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

jakkdl commented Nov 6, 2024

nicoddemus commented Nov 6, 2024

jakkdl commented Nov 6, 2024

Zac-HD commented Nov 6, 2024

jakkdl commented Nov 6, 2024

jakkdl commented Nov 10, 2024 • edited Loading

jakkdl commented Nov 11, 2024

Zac-HD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Oct 31, 2024 •

edited

Loading

nicoddemus Nov 6, 2024 •

edited

Loading

jakkdl commented Nov 10, 2024 •

edited

Loading