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.

Sign up for GitHub

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

Jump to bottom

support async scenarios #426

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

support async scenarios #426

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
34f68dc
support async scenarios
graingert Jun 3, 2021
677a47f
fix tox -e *-coverage running against source files
graingert Jun 3, 2021
abbf669
xfail async scenarios test on py3.10
graingert Jun 3, 2021
750e3dc
collect reporting from pytest_bdd package
graingert Jun 3, 2021
7e35215
ignore coverage part files
graingert Jun 3, 2021
c1e1ee1
upgrade xdist for sys.path fix
graingert Jun 16, 2021
72be7f2
Revert "xfail async scenarios test on py3.10"
graingert Jun 27, 2021
92bac99
remove unasync
graingert Jun 27, 2021
a33ad33
avoid _PYTEST_CMD
graingert Jun 27, 2021
ac90974
Merge branch 'master' into support-async-scenarios
graingert Jul 29, 2021
702a1e8
Merge branch 'master' into support-async-scenarios
graingert Jul 31, 2021
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
11 changes: 8 additions & 3 deletions .coveragerc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
[run]
branch = true
include =
pytest_bdd/*
tests/*
source_pkgs = pytest_bdd
source = tests

[paths]
source =
.
.tox/*/lib/*/site-packages/
.tox\\*\\Lib\\site-packages\\
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pip-log.txt

# Unit test / coverage reports
.coverage
.coverage.*
.tox
nosetests.xml

Expand Down
2 changes: 1 addition & 1 deletion pytest_bdd/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@

__version__ = "4.1.0"

__all__ = [given.__name__, when.__name__, then.__name__, scenario.__name__, scenarios.__name__]
__all__ = ["given", "when", "then", "scenario", "scenarios"]
63 changes: 48 additions & 15 deletions pytest_bdd/scenario.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
scenario_name="Publishing the article",
)
"""
import contextlib
import collections
import os
import re
Expand Down Expand Up @@ -81,7 +82,7 @@ def _find_step_function(request, step, scenario):
)


def _execute_step_function(request, scenario, step, step_func):
async def _execute_step_function(request, scenario, step, step_func, sync):
"""Execute step function.

:param request: PyTest request.
Expand All @@ -103,7 +104,10 @@ def _execute_step_function(request, scenario, step, step_func):
request.config.hook.pytest_bdd_before_step_call(**kw)
target_fixture = getattr(step_func, "target_fixture", None)
# Execute the step.
return_value = step_func(**kwargs)
if sync:
return_value = step_func(**kwargs)
else:
return_value = await step_func(**kwargs)
Comment on lines +107 to +110
Copy link
Contributor

@TBBle TBBle Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Rather than relying on the sync parameter, perhaps you could use inspect.isawaitable to decide whether to await or not. That way it's natural for the user, and you don't need to include sync=False in each of several step decorators for a single function.

It might also make sense to use inspect.isasyncgenfunction to catch and reject situations where the coroutine is calling yield rather than return, as that implies it expects to be called repeatedly for multiple values, and it won't be in this structure.

Copy link
Member Author

Choose a reason for hiding this comment

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

the sync flag tells this function how its return value is to be handled. And so it can't await when it's true, and must await when false.

Copy link
Member Author

Choose a reason for hiding this comment

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

def scenarios could use inspect.iscoroutinefunction to choose how to set this flag, however, pytest itself handles this by raising a warning and requiring explicit opt in.

Copy link
Contributor

Choose a reason for hiding this comment

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

pytest's handling of that is under a hook which pytest plugins which handle asyncio can use to intercept that action; it's the plugins that may require explicit opt-in, e.g. we use pytest-aiohttp which supports async def tests with no further opt-in required.

Perhaps you can use the same hook pytest_pyfunc_call to call step_func, and then we get the support for asyncio for free by using a plugin that gives us pytest/asyncio integration.

Or if that's not feasible (as it turns each step into a test-func), introduce a hook like pytest_bdd_call_step, and those plugins can implement that hook in the same way they hook pytest_pyfunc_call. Again, the user isn't troubled by needing to add an extra flag to each decorator, since they must already be doing whatever their particular pytest/asyncio plugin needs done.

Maybe I'm not understanding the idea, but wouldn't doing detection only in def scenarios require that either all steps or no steps are async def?

Copy link
Member Author

Choose a reason for hiding this comment

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

the scenarios shortcut means you only need to use the flag once: https://github.com/pytest-dev/pytest-bdd/pull/426/files#diff-ddd190a8c08e1a7c1418871a5ea37984346a9fea970b373f3f843cc635d301d6R74

Copy link
Member Author

@graingert graingert Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

also we want to get the error: <stdin>:1: RuntimeWarning: coroutine 'step_func' was never awaited if this code always awaited any awaitable we'd get RuntimeError("coro did not stop") instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I understanding correctly, that an async def function under trio would return false for inspect.isawaitable, even though you are required to await it? That is surprisingly different to the given definition of inspect.isawaitable.

Return True if the object can be used in await expression.

unless there's some nuance I'm missing. Is the documentation incorrect, and inspect.isawaitable is actually looking for an asyncio-specific return type (this Awaitable concept, that you mentioned?), rather than a function with the appropriate type-flag set?

For the rest, do we need to support as the default having such things as anyio.current_time or aiostream.core.Stream() passed in as step_func, with the cost that using a step_func of a straight forward async def function needs an extra marker to say "Yes, I meant to type async there, please honour it".

Since this marker is on the scenario, not the step, it seems that you cannot have step_func that is like those examples, if you also have step_funcs you do want to await, because you have to pick one or the other at the scenario level. And since both those examples are async-related, it seems odd that you'd use them that way, and then not expect to be able to write async def for any other steps in the scenario.

As an alternative, what if the sync flag was on the step decorator, and defaulted to False, and (assuming some suitable replacement for inspect.inawaitable that really means "Can be used with await"), then the logic becomes

if can_be_awaited(step_func) and not getattr(step_func, "sync", False):
 return_value = await step_func(...)
else
 return_value = step_func(...)

so that in the default case

scenarios(...)

@given(..., target_fixture=the_given)
def the_given():
 return real_setup()

@when(...)
async def(the_given):
 await act_on(the_given)

the right thing happens.

If users are going to write things like

given(...)(anyio.current_time)

they will get an error from their step, and need to instead write

given(..., sync=True)(anyio.current_time)

to indicate that they've passed a thing that can be used with await, but shouldn't be; or even better, they should write

@given(...)
async def current_time():
 return anyio.current_time()

to turn the thing that shouldn't be awaited (but needs to be called under an event loop, I assume) into a thing that can be awaited. I would encourage that third form, because then I don't need to ever see a "sync" flag, I can just see a coroutine, and I'm using the decorator as a decorator, which is the least-surprising place to see it.

Looking at anyio.current_time specifically, I see that it used to be a coroutine, and now isn't, so calling await on it isn't actually invalid in the sense of the other examples, and optimising the behaviour to make that work trivially seems to come at a high price of making normal things noiser and non-trivial. I guess that third anyio.current_time doesn't need the async at all, since it's not calling a coroutine. But for a general case of "Thing that works with await but shouldn't be used with await here", the idea still applies.

If the above was done, then the flag shouldn't be spelled sync but perhaps force_sync_call or dont_await perhaps. Something that explicitly says "Even though you passed a thing I can use with await, I trust you when you say you don't want me to use it with await, but pretend it's a non-coroutine". If it's only used in unusual circumstances, it can be more-verbose than if you have to use it on every use of the API in the default async case.

I don't see how RuntimeWarning: coroutine 'step_func' was never awaited' was never awaited is better than RuntimeError("coro did not stop"), and in my opinion, the RuntimeError case seems a better outcome:

  • Errors are a better result than Warnings for "We know it's not doing what you meant it to do, but couldn't tell you until later"
  • That RuntimeWarning implies almost muscle-memory "You forgot to write await", but there's no where to add an await for the user (instead, they must set a flag that isn't even spelt async or await), so we're mismatching expectations.
  • Unless I'm missing some particular driving use-case, the RuntimeWarning is triggered by "User missed a parameter in the common case", the RuntimeError is triggered by "User used a function decorator with something that wasn't a function, and failed to tell the decorator this". I don't see a use-case for this in the tests here, and feel that "User forgot to do something" should be a possible result for the less-common case, rather than the more-common case.

A clarification on my earlier comment

with pytest-aiohttp, we cannot use scenarios as we have to add a loop fixture to each scenario call, and scenarios does not (yet) provide a way to add a fixture to the generated scenarios.

This was needed because without this PR, the kwargs = {arg: request.getfixturevalue(arg) for arg in get_args(step_func)} is happening outside an async context, and so pytest-aiohttp complains if you try to depend on loop in the step that actually cares about it, without having already depended on it in a real pytest function (i.e. the @scenario-decorated function), because of the way it implements the loop fixture. (It's a bit magic, similar to how pytest-trio implements trio-mode).

With this PR, that getfixturevalue call happens inside an async context, and so I think that means we might be able to move the loop fixture back to where it's actually used, and hence resume using scenarios(...). But I'm not sure about that (because it's a bit magic), and it doesn't change the rest of this comment 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.

indeed no async function is awaitable:

>>> import inspect, asyncio
>>> inspect.isawaitable(asyncio.sleep)
False

trio treats the call and the await of all async functions as a single atomic operation. Coroutines and Awaitables are never exposed to the end user.

regarding the sync flag, the return value of the step_func doesn't relate to whether _execute_step_function can await it or not.
eg at this point:

async def _execute_step_function(request, scenario, step, step_func, sync):

the sync=True/False flag here is about how the _execute_step_function is expected to behave. Eg if sync is True, it must never delegate to a coroutine that could yield because the await_ trampoline has no way to handle it

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 happy to call this flag async_, await_, do_not_await etc, whatever you think is clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I can see my mistake with inspect.isawaitable, as that is True for the thing returned by asyncio.sleep(), not asyncio.sleep itself. inspect.iscoroutinefunction is the test I was looking for, but it's only True for a subset of the things where await thing() is valid.

My main concern remains that what seems like the obvious use-case

scenarios(...)

@given(..., target_fixture=the_given)
def the_given():
 return real_setup()

@when(...)
async def the_when():
 await act_on(the_given)

is impossible with the current PR, as there is no value for sync you can pass to scenarios that will work, it will either await the_given() faultily, or not await the_when() faultily. And if you share steps between scenarios, it's pretty viral and so in-effect, the sync flag usage will grow out to all scenarios, which is probably why pytest handles this with hooks instead.

I expect that this should work as-written, without an explicit sync flag at all, as the default use-case. But I acknowledge that this might not be wholly feasible, since while inspect.iscoroutinefunction would work here, there are other situations that it will not catch; my ideal would be that extra flags be reserved for those cases, not the general setup of decorating def and async def functions.

But at this point, I'm speculating without trying things in code, so I'm probably at the limit of contributions here.

Given that the default state of the flag as-is is to not await, it could be inverted to default-False, and named await_all_steps. That's very clear about what it's doing, including the limitation it is operating under, that either every step has await called on its return value, or no steps do.

Comment on lines +107 to +110
Copy link
Member Author

Choose a reason for hiding this comment

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

so you're saying?

        if (force_async or iscoroutinefunction(step_func)) and not sync:
            return_value = await step_func(**kwargs)
        else:
            return_value = step_func(**kwargs)

this way Twisted users would write:

@given(force_async=True)
def has_something():
    return treq.get("https://example.com")

Copy link
Member Author

Choose a reason for hiding this comment

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

or something like this?

def force_async(fn):
    async def wrapper(*args, **kwargs):
         return await fn(*args, **kwargs)

    return wrapper

def force_sync(fn):
    async def wrapper(*args, **kwargs:
        return fn(*args, **kwargs)
    return wrapper


scenarios(..., sync=False)

@given(...)
@force_async
def has_something():
    return treq.get("https://example.com")

@given(..., target_fixture=the_given)
@force_sync
def the_given():
 return real_setup()

@when(...)
async def(the_given):
 await act_on(the_given)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, something like one of those. I think I prefer the former, but obviously haven't used them in anger.

My underlying assumption is that if you have a coroutinefunction, you are very likely to expect to await it, e.g. in trio you must await it; with asyncio you should await it, unless you are actually trying to return the "awaitable" to use later with the requisite risk of accidentally never awaiting it. I think the "unless" version there is rare enough that flagging it explicitly in the step definition is fine; it's also probably a particularly odd thing to do in a BDD-style test.

If it's not a coroutinefunction, I'd suggest that it's more commonly just a plain function to call (i.e. code that is working in pytest-bdd today; I'd make @force_sync the default behaviour there), and making it possible to say "This non-coroutine function is returning an awaitable to be awaited" covers the treq.get case there.

Could the treq.get case be simplified as

@given(...)
async def has_something():
    return await treq.get("https://example.com")

so you don't need to @force_async there? This step_func would be awaited by default (being async def), and that will await treq.get explicitly in the step, where we know it's the right thing to do, assuming we know that treq.get's return value needs to be awaited.

That feels more natural to me, but I haven't used treq though, so I might be overlooking something there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but Twisted projects are the opposite of Trio, in that await is currently rarely used and .addCallbacks is used instead

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, thank you for clarifying. I used Twisted heavily almost a decade ago, i.e. on Python 2, before await; so wasn't sure if the common usage patterns had shifted towards await in the meantime.

if target_fixture:
inject_fixture(request, target_fixture, return_value)

Expand All @@ -113,7 +117,7 @@ def _execute_step_function(request, scenario, step, step_func):
raise


def _execute_scenario(feature, scenario, request):
async def _execute_scenario(feature, scenario, request, sync):
"""Execute the scenario.

:param feature: Feature.
Expand All @@ -133,15 +137,26 @@ def _execute_scenario(feature, scenario, request):
request=request, feature=feature, scenario=scenario, step=step, exception=exception
)
raise
_execute_step_function(request, scenario, step, step_func)
await _execute_step_function(request, scenario, step, step_func, sync)
finally:
request.config.hook.pytest_bdd_after_scenario(request=request, feature=feature, scenario=scenario)


FakeRequest = collections.namedtuple("FakeRequest", ["module"])


def _get_scenario_decorator(feature, feature_name, scenario, scenario_name):
def await_(fn, *args):
v = fn(*args)
with contextlib.closing(v.__await__()) as gen:
try:
gen.send(None)
except StopIteration as e:
return e.value
else:
raise RuntimeError("coro did not stop")


def _get_scenario_decorator(feature, feature_name, scenario, scenario_name, *, sync):
# HACK: Ideally we would use `def decorator(fn)`, but we want to return a custom exception
# when the decorator is misused.
# Pytest inspect the signature to determine the required fixtures, and in that case it would look
Expand All @@ -160,10 +175,19 @@ def decorator(*args):
if arg not in function_args:
function_args.append(arg)

@pytest.mark.usefixtures(*function_args)
def scenario_wrapper(request):
_execute_scenario(feature, scenario, request)
return fn(*(request.getfixturevalue(arg) for arg in args))
if sync:

@pytest.mark.usefixtures(*function_args)
def scenario_wrapper(request):
await_(_execute_scenario, feature, scenario, request, sync)
return fn(*(request.getfixturevalue(arg) for arg in args))

else:

@pytest.mark.usefixtures(*function_args)
async def scenario_wrapper(request):
await _execute_scenario(feature, scenario, request, sync)
return await fn(*(request.getfixturevalue(arg) for arg in args))

for param_set in scenario.get_params():
if param_set:
Expand All @@ -180,7 +204,7 @@ def scenario_wrapper(request):
return decorator


def scenario(feature_name, scenario_name, encoding="utf-8", example_converters=None, features_base_dir=None):
def scenario(feature_name, scenario_name, encoding="utf-8", example_converters=None, features_base_dir=None, sync=True):
"""Scenario decorator.

:param str feature_name: Feature file name. Absolute or relative to the configured feature base path.
Expand Down Expand Up @@ -213,7 +237,7 @@ def scenario(feature_name, scenario_name, encoding="utf-8", example_converters=N
scenario.validate()

return _get_scenario_decorator(
feature=feature, feature_name=feature_name, scenario=scenario, scenario_name=scenario_name
feature=feature, feature_name=feature_name, scenario=scenario, scenario_name=scenario_name, sync=sync
)


Expand Down Expand Up @@ -263,7 +287,7 @@ def get_name():
suffix = f"_{index}"


def scenarios(*feature_paths, **kwargs):
def scenarios(*feature_paths, sync=True, **kwargs):
"""Parse features from the paths and put all found scenarios in the caller module.

:param *feature_paths: feature file paths to use for scenarios
Expand Down Expand Up @@ -293,9 +317,18 @@ def scenarios(*feature_paths, **kwargs):
# skip already bound scenarios
if (scenario_object.feature.filename, scenario_name) not in module_scenarios:

@scenario(feature.filename, scenario_name, **kwargs)
def _scenario():
pass # pragma: no cover
decorator = scenario(feature.filename, scenario_name, sync=sync, **kwargs)
if sync:

@decorator
def _scenario():
pass # pragma: no cover

else:

@decorator
async def _scenario():
pass # pragma: no cover

for test_name in get_python_name_generator(scenario_name):
if test_name not in caller_locals:
Expand Down
2 changes: 1 addition & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ install_requires =
pytest>=4.3

tests_require = tox
packages = pytest_bdd
packages = find:
include_package_data = True

[options.entry_points]
Expand Down
104 changes: 104 additions & 0 deletions tests/feature/test_async_scenarios.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
"""Test scenarios shortcut."""
import textwrap

from tests.utils import assert_outcomes


def test_scenarios(testdir, pytest_params):
"""Test scenarios shortcut (used together with @scenario for individual test override)."""
testdir.makeini(
"""
[pytest]
console_output_style=classic
"""
)
testdir.makeconftest(
"""
import pytest
from pytest_bdd import given

import anyio

@given('I have a bar')
async def i_have_bar():
await anyio.sleep(0)
print('bar!')
return 'bar'
"""
)
features = testdir.mkdir("features")
features.join("test.feature").write_text(
textwrap.dedent(
"""
@anyio
Scenario: Test scenario
Given I have a bar
"""
),
"utf-8",
ensure=True,
)
features.join("subfolder", "test.feature").write_text(
textwrap.dedent(
"""
@anyio
Scenario: Test subfolder scenario
Given I have a bar

@anyio
Scenario: Test failing subfolder scenario
Given I have a failing bar

@anyio
Scenario: Test already bound scenario
Given I have a bar

@anyio
Scenario: Test scenario
Given I have a bar
"""
),
"utf-8",
ensure=True,
)
testdir.makepyfile(
"""
import pytest
from pytest_bdd import scenarios, scenario

@pytest.mark.anyio
@scenario('features/subfolder/test.feature', 'Test already bound scenario', sync=False)
async def test_already_bound():
pass

scenarios('features', sync=False)
"""
)
result = testdir.runpytest_subprocess("-v", "-s", *pytest_params)
assert_outcomes(result, passed=8, failed=2)
result.stdout.fnmatch_lines(["*collected 10 items"])
result.stdout.fnmatch_lines(["*test_test_subfolder_scenario[[]asyncio[]] *bar!", "PASSED"])
result.stdout.fnmatch_lines(["*test_test_subfolder_scenario[[]trio[]] *bar!", "PASSED"])
result.stdout.fnmatch_lines(["*test_test_scenario[[]asyncio[]] *bar!", "PASSED"])
result.stdout.fnmatch_lines(["*test_test_scenario[[]trio[]] *bar!", "PASSED"])
result.stdout.fnmatch_lines(["*test_test_failing_subfolder_scenario[[]asyncio[]] *FAILED"])
result.stdout.fnmatch_lines(["*test_test_failing_subfolder_scenario[[]trio[]] *FAILED"])
result.stdout.fnmatch_lines(["*test_already_bound[[]asyncio[]] *bar!", "PASSED"])
result.stdout.fnmatch_lines(["*test_already_bound[[]trio[]] *bar!", "PASSED"])
result.stdout.fnmatch_lines(["*test_test_scenario_1[[]asyncio[]] *bar!", "PASSED"])
result.stdout.fnmatch_lines(["*test_test_scenario_1[[]trio[]] *bar!", "PASSED"])


def test_scenarios_none_found(testdir, pytest_params):
"""Test scenarios shortcut when no scenarios found."""
testpath = testdir.makepyfile(
"""
import pytest
from pytest_bdd import scenarios

scenarios('.', sync=False)
"""
)
result = testdir.runpytest_subprocess(testpath, *pytest_params)
assert_outcomes(result, errors=1)
result.stdout.fnmatch_lines(["*NoScenariosFound*"])
11 changes: 8 additions & 3 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
[tox]
minversion=3.23.1
requires=
virtualenv>=20.4.7
isolated_build=true
distshare = {homedir}/.tox/distshare
envlist = py38-pytestlatest-linters,
py39-pytest{43,44,45,46,50,51,52,53,54,60,61,62, latest}-coverage,
Expand All @@ -8,7 +12,7 @@ skip_missing_interpreters = true

[testenv]
setenv =
coverage: _PYTEST_CMD=coverage run --append -m pytest
coverage: _PYTEST_MORE_ARGS=--cov
xdist: _PYTEST_MORE_ARGS=-n3 -rfsxX
deps =
pytestlatest: pytest
Expand All @@ -25,9 +29,10 @@ deps =
pytest44: pytest~=4.4.0
pytest43: pytest~=4.3.0

coverage: coverage
xdist: pytest-xdist
coverage: pytest-cov
xdist: pytest-xdist>=2.3.0
-r{toxinidir}/requirements-testing.txt
anyio[trio]
commands = {env:_PYTEST_CMD:pytest} {env:_PYTEST_MORE_ARGS:} {posargs:-vvl}

; Black doesn't support >py38 now
Expand Down