DeprecationWarning if sync test requests async fixture

[jakkdl]

Fixes #10839, or at least the parts that pytest can handle. It also fixes agronholm/anyio#789
pytest-trio already failed sync tests that were marked with pytest.mark.trio, this will now make that check redundant and we instead mark the test as erroring.

I'm not sure if we care to have a deprecation period for sync-test + autouse-async-fixture, while users may currently have test suites that pass they're very close to shooting themselves in the foot and they will be riddled with RuntimeWarning: coroutine [...] was never awaited

I briefly tested the fix against pytest-asyncio, pytest-trio and anyio's pytest plugin in case any of them would cause false alarms with actual async tests, but as far as I can tell there were no problems. But maybe @agronholm or @seifertm would like to confirm.

If a sync test depends on a sync fixture which itself depends on an async fixture the error message is perhaps slightly confusing. It might be enough to simply name the async fixture in the error message though.

TODO:

• write changelog entry

[jakkdl]

I'm not sure if we care to have a deprecation period for sync-test + autouse-async-fixture, while users may currently have test suites that pass they're very close to shooting themselves in the foot and they will be riddled with RuntimeWarning: coroutine [...] was never awaited

one of the footguns being that this fixture never runs, even for async tests, because the unawaited value is cached by the sync test

import pytest

@pytest.fixture(autouse=True, scope='session')
async def async_fixture():
    assert False

def test_foo():
    ...

@pytest.mark.anyio
async def test_foo_async():
    ...

[agronholm]

Does this mean I won't be able to support async fixtures in sync tests even if I want to?

[jakkdl]

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.

[nicoddemus]

I think this is correct, and I also see a test for this, so I guess we are good.

[jakkdl]

Does this mean I won't be able to support async fixtures in sync tests even if I want to?

I think it might be possible to get around it if you wrap the sync test in an async wrapper on collection, but if that's not desired and you want to support that use-case it might be possible to move the check somewhere it could be overriden

[jakkdl]

hrm. This breaks hypothesis, since @given wraps async tests into sync tests.

[jakkdl]

Requesting pytest plugins to update (given there's an easy way to do so) might be reasonable, but I realized that almost any approach will break on anything that looks like this

@pytest.fixture
async def fix():
  return 1

def test_fix(fix):
  assert 1 == asyncio.run(fix)

we could perhaps try to catch RuntimeWarning: coroutine ... was never awaited and make it error tests even if they don't run with -Werror, but that only gets run on garbage collection... see also #10404

So yeah if implementing this it'd need to be in a way that's quite easy to override. Maybe something like @pytest.mark.allow_async_fixture, and have the error message suggest adding it.

[nicoddemus]

Thanks a lot @jakkdl for tackling this topic, it takes quite a time to look up at all the existing plugins, understanding the semantics, etc.

Please take a look at my comments.

[nicoddemus]

I think this is correct, and I also see a test for this, so I guess we are good.

[nicoddemus]

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

[jakkdl]

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.

[jakkdl]

@nicoddemus what do you think about @pytest.mark.allow_async_fixture? I think requiring e.g. hypothesis to wrap all async fixtures just to work around this warning would be somewhat onerous

[nicoddemus]

@nicoddemus what do you think about @pytest.mark.allow_async_fixture? I think requiring e.g. hypothesis to wrap all async fixtures just to work around this warning would be somewhat onerous

I'm hesitant to add a user-facing mark because of just this warning -- but of course we don't want to force hypothesis (and perhaps other test suites with similar customizations out there) to wrap their async fixtures manually...

Not sure, perhaps @Zac-HD can chime in.

[jakkdl]

@nicoddemus what do you think about @pytest.mark.allow_async_fixture? I think requiring e.g. hypothesis to wrap all async fixtures just to work around this warning would be somewhat onerous

I'm hesitant to add a user-facing mark because of just this warning -- but of course we don't want to force hypothesis (and perhaps other test suites with similar customizations out there) to wrap their async fixtures manually...

Not sure, perhaps @Zac-HD can chime in.

It would be possible to automatically wrap async fixtures upon collection, but that would get quite messy if a test suite has both hypothesis and non-hypothesis tests. And if plugin writers start unilaterally wrapping all their fixtures to look sync then we're defeating the point of this warning in a lot of cases in the first places.

If we don't want a public @pytest.mark, maybe an attribute on pytest.Function (or pytest.Item?) that can be set in pytest_runtest_call to disable the check?

[Zac-HD]

re: impact on Hypothesis - unclear overall. To date we've avoided having any async support, by offering plugins the ability to insert their make-it-sync wrapper between the user's test function and Hypothesis itself. We also nudge users away from function-scoped fixtures, because they're only invoked once for all of the calls we make; and early async fixtures were ~all function-scoped.

Maybe we end up having Hypothesis return an async wrapper function if we're wrapping an async test, keeping the other logic the same? That'd be pretty painful to implment though, and make tracebacks worse for all our current use-cases...

[jakkdl]

To date we've avoided having any async support, by offering plugins the ability to insert their make-it-sync wrapper between the user's test function and Hypothesis itself.

I'll look into this make-it-sync wrapper and see if it can hook into this check as well.

[jakkdl]

To date we've avoided having any async support, by offering plugins the ability to insert their make-it-sync wrapper between the user's test function and Hypothesis itself.

I'll look into this make-it-sync wrapper and see if it can hook into this check as well.

okay so hypothesis+async plugins handle this by adding an .hypothesis object to pytest.Function and messing around with .hypothesis.inner_test. Pytest obviously can't introspect that. I'm guessing there's good reasons why async plugins don't directly modify pytest.Function.obj? Primarily perhaps that's it not really documented as public API.
So yeah what's happening here is that this PR makes a strict assumption that we'll call pytest.Function.obj - but given the existence pytest_runtest_call that's not a great assumption.

..

hmm, thinking deeper about the problem there's perhaps a more fundamental approach we can take. The reason #10839 doesn't give an unawaited coroutine warning is because pytest internals treats fixtures as generators, calling send & close on them, and this suppresses the warning on garbage collection. For some reason the stdlib does not make use of __await__/__aiter__ to detect if a coroutine was unawaited - but we could add a wrapper to pytest.Fixturedef.func() if the underlying fixture function is async that detects these, and make it raise a warning/error if the fixture isn't properly awaited / set up+torn down.

Maybe there's some other possible downside to this (is it valid to just ... refuse to set up a fixture??), but I think that would be a more robust approach and shouldn't require any changes to plugins. Though will have to make sure the wrapper is properly transparent for plugins that are inspecting fixturedef.func.

It wouldn't directly catch the footgun of a sync func requesting a larger-scoped async fixture, where it will work depending on if an async func has previously requested the fixture, but async plugins can optionally handle this, and it will most likely work as the user expects - it's just very fragile.

[jakkdl]

(sorry for messy commit history from rebase/merge of main)

okay this is a much better way of doing it. Turns out I didn't need a wrapper at all and could instead move the check to pytest_fixture_setup which will run after any async plugins have had a chance to handle async fixtures. I tried this against the test suites of pytest-asyncio, pytest-trio, anyio and some of hypothesis - with zero failures other than warnings where expected. This means we could go back to directly raising an error, as I think the only somewhat likely case where this will trigger "incorrectly" is if a test suite has autouse async fixtures they don't care about being ignored in their sync tests.

Idk how much effort to put into the message, could customize it for any combination of autouse & coroutine/asyncgen and could also tell people about available async plugins or direct to the pytest_fixture_setup hook.

This now also catches async tests incorrectly handling async fixtures, making pytest-dev/pytest-asyncio#979 somewhat redundant (though might still keep it for more helpful message for users).

[Zac-HD]

This is looking good - thanks @jakkdl!

[Zac-HD]

Alright, I think we're ready to merge! Ping @nicoddemus to confirm?

[nicoddemus]

LGTM thanks @jakkdl for the great work and @Zac-HD for the review/chiming in about Hypothesis.

Left a few last minor requests, please take a look.

okay so hypothesis+async plugins handle this by adding an .hypothesis object to pytest.Function and messing around with .hypothesis.inner_test. Pytest obviously can't introspect that.

Hmm not sure, if that's all it would take for us to handle this without bothering users and Hypothesis' mainteinares, I would be up for it! Is a simple check, Hypothesis is a well known and widely used library, being "partners" with pytest for a long time, I would consider that for sure if it would mean we could avoid the warning with Hypothesis... but is not clear to me if this is still relevant under the new implementation or not.

[nicoddemus]

Perhaps we should be more explicit why we recommend against this practice in core pytest? I mean this is something a plugin could support correctly in theory?

[jakkdl]

There's some fundamental issues with how to handle it correctly, currently anyio will suspend the async runloop during execution of the sync test. And we also can't simply wrap the test as an async task, as that will lack checkpoints and will hug the runloop. You probably could run it in a separate thread, but yeah anyio doesn't support that currently.

[jakkdl]

LGTM thanks @jakkdl for the great work and @Zac-HD for the review/chiming in about Hypothesis.

Left a few last minor requests, please take a look.

okay so hypothesis+async plugins handle this by adding an .hypothesis object to pytest.Function and messing around with .hypothesis.inner_test. Pytest obviously can't introspect that.

Hmm not sure, if that's all it would take for us to handle this without bothering users and Hypothesis' mainteinares, I would be up for it! Is a simple check, Hypothesis is a well known and widely used library, being "partners" with pytest for a long time, I would consider that for sure if it would mean we could avoid the warning with Hypothesis... but is not clear to me if this is still relevant under the new implementation or not.

Yeah this new implementation has no problem with hypothesis - or any plugins as far as I know, so don't need to go down that road :)

[jakkdl]

just need a brief pass at the updated deprecation docs and then we should be good to merge :)

[Zac-HD]

Looks great - let's ship it!