support async scenarios

[graingert]

Support async scenarios without requiring any specific async framework

[codecov]

Codecov Report

Merging #426 (702a1e8) into master (c7faa9f) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
+ Coverage   96.14%   96.36%   +0.22%     
==========================================
  Files          50       51       +1     
  Lines        1684     1731      +47     
  Branches      159      163       +4     
==========================================
+ Hits         1619     1668      +49     
+ Misses         38       37       -1     
+ Partials       27       26       -1     

Impacted Files	Coverage Δ
pytest_bdd/__init__.py	100.00% <100.00%> (ø)
pytest_bdd/scenario.py	98.34% <100.00%> (+5.79%)	⬆️
tests/feature/test_async_scenarios.py	100.00% <100.00%> (ø)
tests/utils.py	66.66% <0.00%> (-33.34%)	⬇️
tests/conftest.py	77.77% <0.00%> (-22.23%)	⬇️
tests/feature/test_tags.py	96.15% <0.00%> (-3.85%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7faa9f...702a1e8. Read the comment docs.

[youtux]

I'm sorry, but I'm not a big fan of this approach. Specifically, I don't like having a program that produces code, I find it hard to debug and to reason about.

I would instead try a different approach using a library like greenletio.
It would be ideal if we can keep the sync version of the code as it is (or at least with as few modifications as possible), and instead provide only the async version of the main functions (async_scenario, async_scenarios).

This is the same approach that SQLAlchemy took when introducing compatibility with asyncio, and it seem to have worked out well.

[graingert]

greenletio is specific for asyncio, whereas I'm looking for support for Trio

[youtux]

I also see another library, https://greenback.readthedocs.io/, but I didn't have time to check it properly.

[graingert]

Using green threads seems like it would be harder to debug rather than easier.

Would using unasync like a linter be better? As in it just fails CI if the awaits have not been removed by the developer correctly from the sync version of the code?

This would also help with getting the coverage to pass

[graingert]

A note from the greenback docs:

Should I trust this in production? Maybe; try it and see. The technique is in some ways an awful hack

[graingert]

@youtux looking at those green utilities I worked out I could make my own await_ function

[TBBle]

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.

[graingert]

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.

[graingert]

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.

[TBBle]

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?

[graingert]

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

[graingert]

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

[TBBle]

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.

[graingert]

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

[graingert]

I'm happy to call this flag async_, await_, do_not_await etc, whatever you think is clearer

[TBBle]

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.

[graingert]

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

[graingert]

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)

[TBBle]

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.

[graingert]

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

[TBBle]

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.