Skip to content

warn for async generator functions #5734

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

Merged
merged 1 commit into from
Aug 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/5734.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Skip async generator test functions, and update the warning message to refer to ``async def`` functions.
14 changes: 8 additions & 6 deletions src/_pytest/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,16 @@ def is_generator(func):


def iscoroutinefunction(func):
"""Return True if func is a decorated coroutine function.
"""
Return True if func is a coroutine function (a function defined with async
def syntax, and doesn't contain yield), or a function decorated with
@asyncio.coroutine.

Note: copied and modified from Python 3.5's builtin couroutines.py to avoid import asyncio directly,
which in turns also initializes the "logging" module as side-effect (see issue #8).
Note: copied and modified from Python 3.5's builtin couroutines.py to avoid
importing asyncio directly, which in turns also initializes the "logging"
module as a side-effect (see issue #8).
"""
return getattr(func, "_is_coroutine", False) or (
hasattr(inspect, "iscoroutinefunction") and inspect.iscoroutinefunction(func)
)
return inspect.iscoroutinefunction(func) or getattr(func, "_is_coroutine", False)


def getlocation(function, curdir):
Expand Down
10 changes: 6 additions & 4 deletions src/_pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from _pytest.compat import getimfunc
from _pytest.compat import getlocation
from _pytest.compat import is_generator
from _pytest.compat import iscoroutinefunction
from _pytest.compat import NOTSET
from _pytest.compat import REGEX_TYPE
from _pytest.compat import safe_getattr
Expand Down Expand Up @@ -151,15 +152,16 @@ def pytest_configure(config):
@hookimpl(trylast=True)
def pytest_pyfunc_call(pyfuncitem):
testfunction = pyfuncitem.obj
iscoroutinefunction = getattr(inspect, "iscoroutinefunction", None)
if iscoroutinefunction is not None and iscoroutinefunction(testfunction):
msg = "Coroutine functions are not natively supported and have been skipped.\n"
if iscoroutinefunction(testfunction) or (
sys.version_info >= (3, 6) and inspect.isasyncgenfunction(testfunction)
):
msg = "async def functions are not natively supported and have been skipped.\n"
msg += "You need to install a suitable plugin for your async framework, for example:\n"
msg += " - pytest-asyncio\n"
msg += " - pytest-trio\n"
msg += " - pytest-tornasync"
warnings.warn(PytestUnhandledCoroutineWarning(msg.format(pyfuncitem.nodeid)))
skip(msg="coroutine function and no async plugin installed (see warnings)")
skip(msg="async def function and no async plugin installed (see warnings)")
funcargs = pyfuncitem.funcargs
testargs = {arg: funcargs[arg] for arg in pyfuncitem._fixtureinfo.argnames}
testfunction(**testargs)
Copy link
Member Author

Choose a reason for hiding this comment

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

@bluetech it might also be easier to just

Suggested change
testfunction(**testargs)
assert testfunction(**testargs) is None

This would catch generator, async def, async gen and t.i.d.inlineCallbacks all in one go

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't know enough to have an opinion on this.

If the test function should absolutely never return anything once it reaches here, maybe a warning is appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

there are legitimate test functions that return a value as a building block, so this one would be a breaking change

Copy link
Member Author

Choose a reason for hiding this comment

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

legitimate test functions

Sure I was thinking this would be a deprecation warning rather than a skip

Copy link
Member Author

@graingert graingert Aug 13, 2019

Choose a reason for hiding this comment

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

I think we can make the same call as was made when deprecating direct calls of fixtures and require code to look like:
before:

def test_a():
    assert something()
    return FooBar()

def test_b():
    fb = test_a()
    assert fb.something()

after:

def _prepare_for_test():
    assert something()
    return FooBar()

def test_a():
    _prepare_for_test()

def test_b():
    fb = _prepare_for_test()
    assert fb.something()

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why you'd need test_a in this case though

Copy link
Member

Choose a reason for hiding this comment

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

this is pretty much just code-reuse - test_a would simply be a "dependency/fixture" for the b test, but ots own a test anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not code-reuse - it's just redundant, there's no need for test_a

def test_b():
    assert something()
    fb =  FooBar()
    assert fb.something()

Copy link
Member

Choose a reason for hiding this comment

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

so what, it happens in practice, is used in practice and should have a transition period for users to adapt better structural practices

Expand Down
32 changes: 30 additions & 2 deletions testing/acceptance_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1199,11 +1199,39 @@ async def test_2():
[
"test_async.py::test_1",
"test_async.py::test_2",
"*Coroutine functions are not natively supported*",
"*async def functions are not natively supported*",
"*2 skipped, 2 warnings in*",
]
)
# ensure our warning message appears only once
assert (
result.stdout.str().count("Coroutine functions are not natively supported") == 1
result.stdout.str().count("async def functions are not natively supported") == 1
)


@pytest.mark.filterwarnings("default")
@pytest.mark.skipif(
sys.version_info < (3, 6), reason="async gen syntax available in Python 3.6+"
)
def test_warn_on_async_gen_function(testdir):
testdir.makepyfile(
test_async="""
async def test_1():
yield
async def test_2():
yield
"""
)
result = testdir.runpytest()
result.stdout.fnmatch_lines(
[
"test_async.py::test_1",
"test_async.py::test_2",
"*async def functions are not natively supported*",
"*2 skipped, 2 warnings in*",
]
)
# ensure our warning message appears only once
assert (
result.stdout.str().count("async def functions are not natively supported") == 1
)
26 changes: 23 additions & 3 deletions testing/test_compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ def test_is_generator_asyncio():
result.stdout.fnmatch_lines(["*1 passed*"])


@pytest.mark.skipif(
sys.version_info < (3, 5), reason="async syntax available in Python 3.5+"
)
def test_is_generator_async_syntax(testdir):
testdir.makepyfile(
"""
Expand All @@ -113,6 +110,29 @@ async def bar():
result.stdout.fnmatch_lines(["*1 passed*"])


@pytest.mark.skipif(
sys.version_info < (3, 6), reason="async gen syntax available in Python 3.6+"
)
def test_is_generator_async_gen_syntax(testdir):
testdir.makepyfile(
"""
from _pytest.compat import is_generator
def test_is_generator_py36():
async def foo():
yield
await foo()

async def bar():
yield

assert not is_generator(foo)
assert not is_generator(bar)
"""
)
result = testdir.runpytest()
result.stdout.fnmatch_lines(["*1 passed*"])


class ErrorsHelper:
@property
def raise_exception(self):
Expand Down