-
Notifications
You must be signed in to change notification settings - Fork 221
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
support async scenarios #426
base: master
Are you sure you want to change the base?
Changes from 8 commits
34f68dc
677a47f
abbf669
750e3dc
7e35215
c1e1ee1
72be7f2
92bac99
a33ad33
ac90974
702a1e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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\\ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ pip-log.txt | |
|
||
# Unit test / coverage reports | ||
.coverage | ||
.coverage.* | ||
.tox | ||
nosetests.xml | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
[build-system] | ||
requires = ["setuptools", "wheel"] | ||
build-backend = "setuptools.build_meta" | ||
|
||
[tool.black] | ||
line-length = 120 | ||
target-version = ['py36', 'py37', 'py38'] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
scenario_name="Publishing the article", | ||
) | ||
""" | ||
import contextlib | ||
import collections | ||
import os | ||
import re | ||
|
@@ -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. | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Could the treq.get case be simplified as
so you don't need to That feels more natural to me, but I haven't used There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if target_fixture: | ||
inject_fixture(request, target_fixture, return_value) | ||
|
||
|
@@ -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. | ||
|
@@ -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 | ||
|
@@ -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: | ||
|
@@ -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. | ||
|
@@ -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 | ||
) | ||
|
||
|
||
|
@@ -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 | ||
|
@@ -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: | ||
|
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*"]) |
There was a problem hiding this comment.
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 useinspect.isawaitable
to decide whether toawait
or not. That way it's natural for the user, and you don't need to includesync=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 callingyield
rather thanreturn
, as that implies it expects to be called repeatedly for multiple values, and it won't be in this structure.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def scenarios
could useinspect.iscoroutinefunction
to choose how to set this flag, however, pytest itself handles this by raising a warning and requiring explicit opt in.There was a problem hiding this comment.
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 callstep_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 hookpytest_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 areasync def
?There was a problem hiding this comment.
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-ddd190a8c08e1a7c1418871a5ea37984346a9fea970b373f3f843cc635d301d6R74There was a problem hiding this comment.
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 getRuntimeError("coro did not stop")
insteadThere was a problem hiding this comment.
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 returnfalse
forinspect.isawaitable
, even though you are required toawait
it? That is surprisingly different to the given definition ofinspect.isawaitable
.unless there's some nuance I'm missing. Is the documentation incorrect, and
inspect.isawaitable
is actually looking for an asyncio-specific return type (thisAwaitable
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
oraiostream.core.Stream()
passed in asstep_func
, with the cost that using astep_func
of a straight forwardasync def
function needs an extra marker to say "Yes, I meant to typeasync
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 havestep_func
s 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 writeasync def
for any other steps in the scenario.As an alternative, what if the
sync
flag was on the step decorator, and defaulted toFalse
, and (assuming some suitable replacement forinspect.inawaitable
that really means "Can be used withawait
"), then the logic becomesso that in the default case
the right thing happens.
If users are going to write things like
they will get an error from their step, and need to instead write
to indicate that they've passed a thing that can be used with
await
, but shouldn't be; or even better, they should writeto 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 callingawait
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 thirdanyio.current_time
doesn't need theasync
at all, since it's not calling a coroutine. But for a general case of "Thing that works withawait
but shouldn't be used withawait
here", the idea still applies.If the above was done, then the flag shouldn't be spelled
sync
but perhapsforce_sync_call
ordont_await
perhaps. Something that explicitly says "Even though you passed a thing I can use withawait
, I trust you when you say you don't want me to use it withawait
, 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 thanRuntimeError("coro did not stop")
, and in my opinion, theRuntimeError
case seems a better outcome:RuntimeWarning
implies almost muscle-memory "You forgot to writeawait
", but there's no where to add anawait
for the user (instead, they must set a flag that isn't even speltasync
orawait
), so we're mismatching expectations.RuntimeWarning
is triggered by "User missed a parameter in the common case", theRuntimeError
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
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 onloop
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 theloop
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 theloop
fixture back to where it's actually used, and hence resume usingscenarios(...)
. But I'm not sure about that (because it's a bit magic), and it doesn't change the rest of this comment anyway.There was a problem hiding this comment.
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:
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 thestep_func
doesn't relate to whether_execute_step_function
can await it or not.eg at this point:
the
sync=True/False
flag here is about how the _execute_step_function is expected to behave. Eg ifsync
isTrue
, it must never delegate to a coroutine that couldyield
because theawait_
trampoline has no way to handle itThere was a problem hiding this comment.
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 clearerThere was a problem hiding this comment.
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 isTrue
for the thing returned byasyncio.sleep()
, notasyncio.sleep
itself.inspect.iscoroutinefunction
is the test I was looking for, but it's only True for a subset of the things whereawait thing()
is valid.My main concern remains that what seems like the obvious use-case
is impossible with the current PR, as there is no value for
sync
you can pass toscenarios
that will work, it will eitherawait
the_given()
faultily, or notawait
the_when()
faultily. And if you share steps between scenarios, it's pretty viral and so in-effect, thesync
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 whileinspect.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 decoratingdef
andasync 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 hasawait
called on its return value, or no steps do.