Clarify docs about requesting the `event_loop` fixture
tuukkamustonen opened this issue · 10 comments
https://pytest-asyncio.readthedocs.io/en/latest/reference/fixtures/index.html#event-loop suggests that it's fine to grab event_loop fixture in a non-async function.
0.21+ migration guide in https://pytest-asyncio.readthedocs.io/en/latest/how-to-guides/migrate_from_0_21.html suggests to convert all sync functions requesting the event_loop fixture into async functions, and then acquire the loop via asyncio.get_running_loop().
Which one is it, and what is the rationale of not depending on the event_loop fixture directly?
The use of event_loop is deprecated. We should adjust the docs to reflect that.
The rationale is summarized here:
#706 (comment)
Slowly getting back here.
The thing why I'm asking is that currently we do something like:
def some_fixture(event_loop):
# do some sync stuff etc...
# then trigger async actions:
event_loop.run_until_complete(...)Similar pattern is not possible with asyncio.get_running_loop() because:
This function can only be called from a coroutine or a callback.
Being able to run invocations in the event loop in a sync function is convenient, as we don't have 100% async codebase, and tests are mostly written in sync.
Do you see the use case here @seifertm? Is the advice to convert the usages to async and then asyncio.to_thread() to sync parts, instead?
The migration guide suggests converting such tests to async tests and use get_running_loop inside the async test. Following your example:
@pytest_asyncio.fixture()
async def some_fixture():
# do some sync stuff etc...
# then trigger async actions:
event_loop = asyncio.get_running_loop()
event_loop.run_until_complete(...)You could possibly even skip asyncio.get_running_loop() in favour of:
@pytest_asyncio.fixture()
async def some_fixture():
# do some sync stuff etc...
# then trigger async actions:
await ...@tuukkamustonen Do any of those approaches work for your code base?
@seifertm Hey hey, sorry I totally missed your response (in December!)
The problem was that earlier it was possible to inject the loop into a sync test, e.g.:
def test_something(event_loop):
event_loop.run_until_complete(...)But with the fixture deprecated, you're no longer supposed to do that.
Unless I define my own fixture:
@pytest.fixture
async def event_loop():
return asyncio.get_running_loop()
def test_something(event_loop):
event_loop.run_until_complete(...)Didn't try that, but I guess it would work?
Or, do something like:
class TestSomeClass:
loop: asyncio.AbstractEventLoop
@pytext.fixture(autouse=True)
def init(self):
self.loop = asyncio.get_running_loop()
def test_something(self):
self.loop.run_until_complete(...)How do you see those? Bad? Naughty?
It's convenient to access the loop from sync test method - even with async codebases, tests may often by written as sync (and FastAPI et al perfectly allow that).
@tuukkamustonen would it be possible for you to replace
def test_something(event_loop):
event_loop.run_until_complete(...)
with
async def test_something():
event_loop = asyncio.get_running_loop( )
event_loop.run_until_complete(...)or
async def test_something():
await …?
My understanding is that these are functionally equivalent and they would allow pytest-asyncio to proceed with the event_loop fixture deprecation.
No, that converts the whole test as async, we want to keep them as sync.
The tests might be doing a lot of things, ie. calling DB, reading network, file system, etc and we'd need to convert everything to async. Don't want to take that effort / see the benefit.
Of course, sync IO works in async context, it just blocks the loop. On a usual application that would be disastrous, I don't know what the effect in this case would be? Are async tests still run one-by-one, ie. not concurrently, so that blocking the loop would actually do no harm?
No, that converts the whole test as
async, we want to keep them as sync.The tests might be doing a lot of things, ie. calling DB, reading network, file system, etc and we'd need to convert everything to async. Don't want to take that effort / see the benefit.
Your benefit is only indirect. Getting rid of event_loop allows pytest-asyncio to adjust to recent asyncio development and simplify its code base. This will hopefully result in more contributions and/or faster development.
Of course, sync IO works in async context, it just blocks the loop. On a usual application that would be disastrous, I don't know what the effect in this case would be? Are async tests still run one-by-one, ie. not concurrently, so that blocking the loop would actually do no harm?
Pytest-asyncio still runs its tests one-by-one. This is a constraint by pytest itself. The asyncio plugin merely tries to make it easier to test async code without having to use asyncio.Runner or asyncio.get_event_loop directly.
I think we're on the same page here: Doing a sync call in an async test will simply block test execution in the same way as in a sync test. There should be no functional change from the conversion I suggested above.
There was an idea to provide automatic code rewriting for such changes using libCST. It's definitely an option to include such a rewriter in pytest-asyncio, but someone would have to contribute it. It's only worth it for large test suites, I guess.
Hey, just for clarity's sake, I'm not arguing pytest-asyncio should do something differently 😃 Merely expressing the use case, and that the change might bring some users some troubles.
As discussed, there are two workarounds:
- Grab the event loop and store it as property somewhere the sync test method can acquire it
- Just convert to
asynctest methods, and keep calling sync IO - the blocking shouldn't harm anything, and tests would run just the same / just as fast 🤷♂
These could have been explained/suggested in the migration docs, though both are a bit hacky, so I understand you don't probably want to give such suggestions 😄 Well, at least they're now documented here, in this ticket.
There was an idea to provide automatic code rewriting for such changes using libCST. It's definitely an option to include such a rewriter in pytest-asyncio, but someone would have to contribute it. It's only worth it for large test suites, I guess.
Honestly, that (complexity of it) sounds like a trap. Could be a separate (and more generic) tool, if someone needs that. Surely not part of pytest-asyncio, is how I'd feel!
(I think we can conclude this discussion - thanks for the help and your work on the lib in general 🙏🏻)
Hey, just for clarity's sake, I'm not arguing
pytest-asyncioshould do something differently 😃 Merely expressing the use case, and that the change might bring some users some troubles.As discussed, there are two workarounds:
1. Grab the event loop and store it as property somewhere the _sync_ test method can acquire it 2. Just convert to `async` test methods, and keep calling sync IO - the blocking shouldn't harm anything, and tests would run just the same / just as fast 🤷♂
Sorry if my comment came across too strongly. I do appreciate these discussions, because it's the only way to find out about real use cases and how well pytest-asyncio is doing with regards to transparency and documentation.
These could have been explained/suggested in the migration docs, though both are a bit hacky, so I understand you don't probably want to give such suggestions 😄 Well, at least they're now documented here, in this ticket.
There's a migration guide for v0.21 users that mentions this approach. Do you feel anything is missing or is it just hard to find (suggestions are welcome)?
There was an idea to provide automatic code rewriting for such changes using libCST. It's definitely an option to include such a rewriter in pytest-asyncio, but someone would have to contribute it. It's only worth it for large test suites, I guess.
Honestly, that (complexity of it) sounds like a trap. Could be a separate (and more generic) tool, if someone needs that. Surely not part of
pytest-asyncio, is how I'd feel!
The Hypothesis library provided such a migration tool as part of their packages. The experience was really enjoyable. That's why I think something similar could be done for pytest-asyncio.
Hey, thanks for following through!
There's a migration guide for v0.21 users that mentions this approach. Do you feel anything is missing or is it just hard to find (suggestions are welcome)?
Well, the guide suggests to:
Convert all synchronous test cases requesting the event_loop fixture to asynchronous test cases.
But normally, converting from sync to async means converting all IO (and maybe some extra exotic bits) to async. It's not a small task on an established codebase/testbase. For example in my case here, we don't have resources (money) for such work.
So, it's a suggestion one probably cannot follow. My thought was "nah we can't do that, there must be a better way". Others might think the same.
One workaround is to convert the test to async, but harshly keep doing sync IO, as we've discussed. The guide could mention that, but I also understand if you don't want to give such hacky tips for the readers.
The Hypothesis library provided such a migration tool as part of their packages. The experience was really enjoyable. That's why I think something similar could be done for pytest-asyncio.
Oh, interesting. Got to check that out, thanks!