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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6728ec5
Raise error if sync test relies on async fixture, and warn if the fix…
jakkdl Oct 31, 2024
5c41d50
Merge remote-tracking branch 'origin/main' into sync_test_async_fixture
jakkdl Nov 1, 2024
8e100ea
fix tests
jakkdl Nov 1, 2024
283db4e
add changelog
jakkdl Nov 1, 2024
5beab07
improve warning message. Make it warn regardless of autouse or not. A…
jakkdl Nov 6, 2024
2d06ff2
Merge branch 'main' into sync_test_async_fixture
jakkdl Nov 6, 2024
0de5302
fix test
jakkdl Nov 6, 2024
1891fed
Rename changelog entries to 'breaking' (#12942)
nicoddemus Nov 6, 2024
6b9de2a
[pre-commit.ci] pre-commit autoupdate
pre-commit-ci[bot] Oct 21, 2024
94dd153
Upgrade pylint version, activate all extensions
Pierre-Sassoulas Oct 29, 2024
b19fd52
[pylint dict-init-mutate] Initialize a dict right off for speed
Pierre-Sassoulas Oct 29, 2024
987904c
[automated] Update plugin list (#12950)
github-actions[bot] Nov 10, 2024
70639ef
build(deps): Bump django in /testing/plugins_integration (#12951)
dependabot[bot] Nov 11, 2024
7256c0c
build(deps): Bump pypa/gh-action-pypi-publish from 1.10.3 to 1.12.2 (…
dependabot[bot] Nov 11, 2024
c98ef2b
change implementation so the check happens in pytest_fixture_setup af…
jakkdl Nov 11, 2024
cd3eb98
fix test
jakkdl Nov 11, 2024
2d9bb86
Merge branch 'main' into sync_test_async_fixture
jakkdl Nov 11, 2024
876cc2a
update docs/changelog
jakkdl Nov 11, 2024
1a4dfbb
remove incorrect comments, add link
jakkdl Nov 14, 2024
d35e4eb
revert now unrelated fix
jakkdl Nov 14, 2024
ef096cd
small wording changes
jakkdl Nov 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/10839.deprecation.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Synchronous tests that request an asynchronous fixture with ``autouse=True`` will now give a DeprecationWarning.
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions changelog/10839.improvement.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Synchronous tests that request asynchronous fixtures will now error, instead of silently accepting an unawaited coroutine object as the fixture value.
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
43 changes: 42 additions & 1 deletion src/_pytest/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
from _pytest.scope import _ScopeName
from _pytest.scope import HIGH_SCOPES
from _pytest.scope import Scope
from _pytest.warning_types import PytestRemovedIn9Warning


if sys.version_info < (3, 11):
Expand Down Expand Up @@ -575,6 +576,7 @@
# The are no fixtures with this name applicable for the function.
if not fixturedefs:
raise FixtureLookupError(argname, self)

# A fixture may override another fixture with the same name, e.g. a
# fixture in a module can override a fixture in a conftest, a fixture in
# a class can override a fixture in the module, and so on.
Expand All @@ -593,6 +595,39 @@
raise FixtureLookupError(argname, self)
fixturedef = fixturedefs[index]

# Check for attempted use of an async fixture by a sync test
# `self.scope` here is not the scope of the requested fixture, but the scope of
# the requester.
if (
self.scope == "function"
and not inspect.iscoroutinefunction(self._pyfuncitem.obj)
and (
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.

if fixturedef._autouse:
warnings.warn(

Check warning on line 610 in src/_pytest/fixtures.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/fixtures.py#L610

Added line #L610 was not covered by tests
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.

"If you intended to use the fixture you may want to make the "
"test asynchronous. If you did not intend to use it you should "
"restructure your test setup. "
"This will turn into an error in pytest 9."
),
stacklevel=3,
)
else:
raise FixtureLookupError(

Check warning on line 621 in src/_pytest/fixtures.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/fixtures.py#L621

Added line #L621 was not covered by tests
Zac-HD marked this conversation as resolved.
Show resolved Hide resolved
argname,
self,
(
"ERROR: Sync test requested async fixture. "
"You may want to make the test asynchronous and run it with "
"a suitable async framework test plugin."
),
)

# Prepare a SubRequest object for calling the fixture.
try:
callspec = self._pyfuncitem.callspec
Expand Down Expand Up @@ -805,7 +840,7 @@
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:
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
# The last fixture raise an error, let's present
# it at the requesting side.
stack = stack[:-1]
Expand Down Expand Up @@ -959,6 +994,8 @@
ids: tuple[object | None, ...] | Callable[[Any], object | None] | None = None,
*,
_ispytest: bool = False,
# only used to emit a deprecationwarning, can be removed in pytest9
_autouse: bool = False,
) -> None:
check_ispytest(_ispytest)
# The "base" node ID for the fixture.
Expand Down Expand Up @@ -1005,6 +1042,9 @@
self.cached_result: _FixtureCachedResult[FixtureValue] | None = None
self._finalizers: Final[list[Callable[[], object]]] = []

# only used to emit a deprecationwarning, can be removed in pytest9
self._autouse = _autouse

@property
def scope(self) -> _ScopeName:
"""Scope string, one of "function", "class", "module", "package", "session"."""
Expand Down Expand Up @@ -1666,6 +1706,7 @@
params=params,
ids=ids,
_ispytest=True,
_autouse=autouse,
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
)

faclist = self._arg2fixturedefs.setdefault(name, [])
Expand Down
87 changes: 87 additions & 0 deletions testing/acceptance_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1286,6 +1286,93 @@ def test_3():
result.assert_outcomes(failed=3)


def test_error_on_sync_test_async_fixture(pytester: Pytester) -> None:
pytester.makepyfile(
test_sync="""
import pytest

@pytest.fixture
async def async_fixture():
...

def test_foo(async_fixture):
...
"""
)
result = pytester.runpytest()
Zac-HD marked this conversation as resolved.
Show resolved Hide resolved
result.stdout.fnmatch_lines(
[
(
"*ERROR: Sync test requested async fixture. "
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
"You may want to make the test asynchronous and run it with "
"a suitable async framework test plugin.*"
),
"*ERROR test_sync.py::test_foo*",
]
)
result.assert_outcomes(errors=1)


def test_error_on_sync_test_async_fixture_gen(pytester: Pytester) -> None:
pytester.makepyfile(
test_sync="""
import pytest

@pytest.fixture
async def async_fixture():
yield

def test_foo(async_fixture):
...
"""
)
result = pytester.runpytest()
result.stdout.fnmatch_lines(
[
(
"*ERROR: Sync test requested async fixture. "
"You may want to make the test asynchronous and run it with "
"a suitable async framework test plugin.*"
),
"*ERROR test_sync.py::test_foo*",
]
)
result.assert_outcomes(errors=1)


def test_warning_on_sync_test_async_autouse_fixture(pytester: Pytester) -> None:
pytester.makepyfile(
test_sync="""
import pytest

@pytest.fixture(autouse=True)
async def async_fixture():
...

# We explicitly request the fixture to be able to
# suppress the RuntimeWarning for unawaited coroutine.
def test_foo(async_fixture):
try:
async_fixture.send(None)
except StopIteration:
pass
"""
)
result = pytester.runpytest()
result.stdout.fnmatch_lines(
[
(
"*Sync test requested an async fixture with autouse=True. "
"If you intended to use the fixture you may want to make the "
"test asynchronous. If you did not intend to use it you should "
"restructure your test setup. "
"This will turn into an error in pytest 9.*"
),
]
)
result.assert_outcomes(passed=1)


def test_pdb_can_be_rewritten(pytester: Pytester) -> None:
pytester.makepyfile(
**{
Expand Down
Loading