Breaking change in 0.23.*

[tkukushkin]

Hello! Something has been broken with the latest pytest-asyncio releases.

Consider such code:

import asyncio
import pytest

@pytest.fixture(scope='session')
def event_loop():
    loop = asyncio.get_event_loop_policy().new_event_loop()
    yield loop
    loop.close()

@pytest.fixture(scope='session')
async def some_async_fixture():
    return asyncio.get_running_loop()

async def test_something(some_async_fixture):
    assert asyncio.get_running_loop() is some_async_fixture

pytest.ini:

[pytest]
asyncio_mode = auto

This test passes with pytest-asyncio 0.21.1 but fails with 0.23.0. I am not sure if it's ok, but if it is, IMHO it is breaking change.

[seifertm]

@GrammAcc Honestly, I don't think I did a very good job explaining why the changes to pytest-asyncio are needed. Therefore, I think you questions are very relevant to this issue and I'll gladly try again to share the motivation behind the changes. After that, I'll share my thoughts on your code example.

1. A bunch of the low-level asyncio functionality is being deprecated, such as asyncio.get_event_loop(). Upstream is also leaning towards deprecating asyncio.set_event_loop(). The policy system is being deprecated, too. While it will still be possible to use asyncio.new_event_loop to request a new loop, pytest-asyncio heavily relies on get_event_loop and its side-effect of creating a new loop when no loop has been set. That means something has to change in pytest-asyncio or it will break in future Python versions.
2. The current practice of overriding the event_loop fixture was a frequent source of head ache and subsequent bug reports. Since the fixture was a "free-form" function where you could write all kinds of code, the fixture was sometimes abused to contain other kinds of setup code.
3. The fact that nearly all users of pytest-asyncio had a custom copy of the _eventloop fixture, means that all the burden of moving to newer Python versions is on the users. It also means that it's practically impossible for pytest-asyncio to make non-breaking changes the _eventloop fixture or that those changes had no effect. However, some of these changes are required to fix point 1.

That's why pytest-asyncio tried to come up with a new way of having event loops with different scopes without requiring a carbon copy of the _eventloop fixture implementation.

Is it to isolate concurrent test setup/code? Wouldn't it be sufficient to simply collect all of the async test cases and fixtures and await them in a sync for-loop?

Depending on the kind of testing you do, it may be desirable that each test case in your test suite is isolated from the other tests. Your specific example runs all test cases in a single asyncio event loop. That means they can potentially influence each other, for example through race conditions from background tasks or context variables. Pytest-asyncio, on the other hand, runs each async test case in an isolated asyncio event loop by default, in an effort to avoid these kinds of pitfalls.

The goal is definitely to have a smooth development experience where you don't have to call low-level asyncio functions. Currently, pytest-asyncio is in a bit of a in-between state, because its trying to provide a migration path for all users towards a "no event_loop fixture overrides" state. Once this is done, a lot of internal refactorings should be unblocked, which should solve some longstanding bugs, such as #127.

I hope this answers your questions! If not, feel free to follow up or to reach out otherwise :)

[GrammAcc]

@seifertm Thank you for your detailed response! That makes perfect sense, and I had no idea upstream was deprecating {get|set}_event_loop.

I definitely agree that managed event loops are better than the overridable fixture in this case, but one thing I will suggest is that I think it's okay to let the user deal with race conditions in the test suite on their own. :)

I've written automated tests in several different environments (Python, NodeJS, Elixir, C++), and one thing that always comes up is that some tests simply need to access some kind of shared state (a test db for example). In a well-structured test suite, most tests will be completely isolated, but eventually, we have to actually test the real behavior of the application, and that usually involves shared state and side effects. How the programmer resolves that is usually application-dependent. For example, in one Node project I worked on, I wrote some setup code to essentially create dynamic per-thread postgres databases (yes an entire db for each thread lol) since it made sense for our environment. Different dev machines and CI VMs had different resource limitations, the ORM and web framework we were using made it difficult to isolate the db that different services used, and the full test suite needed to run in <30 seconds or so. That's not a great way to handle race conditions in db tests, but mocking wasn't an option in this case, and even though the test runner isolated tests into their own threads, the db is still a giant blob of shared mutable state, so I had to work around it somehow.

FWIW, isolating tests into their own event loop also doesn't solve these problems with race conditions accessing a test db, and it seems to make it harder for the user to implement an application-appropriate solution as well since the behavior of a shared fixture/resource (with locking or whatever you decide to use to synchronize it) gives unexpected results due to the event loop isolation.

I think the asyncio_default_fixture_loop_scope setting is probably sufficient to address these kinds of issues, but I just wanted to give my two cents on that part of it since I think test isolation is an extra burden that the pytest-asyncio maintainers can probably leave to the users. As long as the behavior of the async test cases matches the expectation of the users (e.g. test cases are executed sequentially, but any shared state between them needs to be synchronized by the user), then I think you're probably safe to leave that to us. :)

I guess what I'm trying to articulate is that it's probably okay to focus on making the library easy to maintain and refactor and not worry about test isolation as long as test cases don't run concurrently.

At the end of the day, most async applications already have to deal with the potential for race conditions, so the application developer will usually have an idea of what will cause concurrency problems in the test suite, and if they don't, that's something that they have to learn eventually. :)

Anyway, just my two cents on the topic.

I appreciate everything you do here, and thank you again for taking the time to give such a detailed and thoughtful response!

[UltimateLobster]

@seifertm Thank you for your detailed response! That makes perfect sense, and I had no idea upstream was deprecating {get|set}_event_loop.

I definitely agree that managed event loops are better than the overridable fixture in this case, but one thing I will suggest is that I think it's okay to let the user deal with race conditions in the test suite on their own. :)

I've written automated tests in several different environments (Python, NodeJS, Elixir, C++), and one thing that always comes up is that some tests simply need to access some kind of shared state (a test db for example). In a well-structured test suite, most tests will be completely isolated, but eventually, we have to actually test the real behavior of the application, and that usually involves shared state and side effects. How the programmer resolves that is usually application-dependent. For example, in one Node project I worked on, I wrote some setup code to essentially create dynamic per-thread postgres databases (yes an entire db for each thread lol) since it made sense for our environment. Different dev machines and CI VMs had different resource limitations, the ORM and web framework we were using made it difficult to isolate the db that different services used, and the full test suite needed to run in <30 seconds or so. That's not a great way to handle race conditions in db tests, but mocking wasn't an option in this case, and even though the test runner isolated tests into their own threads, the db is still a giant blob of shared mutable state, so I had to work around it somehow.

FWIW, isolating tests into their own event loop also doesn't solve these problems with race conditions accessing a test db, and it seems to make it harder for the user to implement an application-appropriate solution as well since the behavior of a shared fixture/resource (with locking or whatever you decide to use to synchronize it) gives unexpected results due to the event loop isolation.

I think the asyncio_default_fixture_loop_scope setting is probably sufficient to address these kinds of issues, but I just wanted to give my two cents on that part of it since I think test isolation is an extra burden that the pytest-asyncio maintainers can probably leave to the users. As long as the behavior of the async test cases matches the expectation of the users (e.g. test cases are executed sequentially, but any shared state between them needs to be synchronized by the user), then I think you're probably safe to leave that to us. :)

I guess what I'm trying to articulate is that it's probably okay to focus on making the library easy to maintain and refactor and not worry about test isolation as long as test cases don't run concurrently.

At the end of the day, most async applications already have to deal with the potential for race conditions, so the application developer will usually have an idea of what will cause concurrency problems in the test suite, and if they don't, that's something that they have to learn eventually. :)

Anyway, just my two cents on the topic.

I appreciate everything you do here, and thank you again for taking the time to give such a detailed and thoughtful response!

I agree 100% with this point. While it's important to give beginners a default, sane behavior out of the box, it's also important to remember that beginners will usually write simpler code.

As long as the simple use-cases will be taken care of, I think it's ok to give advanced users the responsibility for the rest. Especially if it gives you an easier time maintaining the code and adding features.

In my case, I'd really hope for an opt-in way to execute test cases concurrently sometime in the future (I can already think of tons of edge-cases and bugs it may cause to my tests but as said before, that's something I'm willing to have the responsibility to solve in my own code).

Thank you so much for the transparency and in general for the entire work being done here. My team and I have managed to save so much time by utilizing concurrency in our tests!

[seifertm]

@GrammAcc @UltimateLobster Your input is much appreciated. The same sentiment was already echoed previously by ffissore in https://github.com/pytest-dev/pytest-asyncio/issues/706#issuecomment-1859264437. Maybe pytest-asyncio is trying to do too much. I agree that software can be a pain if it tries to be smarter than the user.

I don't think this is a discussion we should have right now, though. The goal of the releases since v0.21 was to make it easier for pytest-asyncio to evolve by "internalizing" the _eventloop fixture. At the same time, pytest-asyncio wanted to provide a migration path for existing users. Admittedly, this introduced additional complexity, but the releases didn't try to add convenience features to outsmart the developer. Once the transition is complete, we should revisit this topic and see if and how we can simplify the functionality provided by the library.

That said, I hope that v0.24 resolves this long-standing issue. Thanks to all the participants in the discussion. GitHub is currently the only channel for pytest-asyncio maintainers to understand what users really want.

[GrammAcc]

Sorry for necrobumbing this one. I just wanted to let the maintainers know that I mentioned a possible bug in https://github.com/pytest-dev/pytest-asyncio/issues/706#issuecomment-2277043647, but there's nothing in pytest-asyncio. It was a bug with how I was connecting to different DBs when spawning servers in the test suite. I was just chasing a red herring with the event loop stuff. :)

I was busy preparing for a talk at my local user group, and I forgot to report back here.

Sorry for the confusion, and thanks again for such an awesome tool!

[greemo]

Hi @seifertm,

Unfortunately I don't think the changes in 0.24 fix my use case that has been broken since 0.21.1.

@pytest_asyncio.fixture(scope="session")
async def db_schema(db_engine):
    log.info("populating db schema")

    # create schema in database
    async with db_engine.begin() as conn:
        await conn.run_sync(__execute_upgrade)

    log.info("Schema created")
    return db_engine

@pytest_asyncio.fixture(scope="session")
async def db_connection(db_schema):
    # connect to the database
    connection = await db_schema.connect()

    # return connection to the Engine
    yield connection

    await connection.close()

@pytest_asyncio.fixture(scope="function")
async def db_session(db_connection):
    """Create a fixture for a DB Session
    The idea of this fixture is to allow tests to use the same DB connection, the transaction from which will get rolled back after each test, leaving the DB in the clean state. 
    This fixture should be function scope, runing for each test, but should use the event loop from the session fixture it depends on.

    Args:
        db_connection (_type_): _description_

    Returns:
        _type_: _description_
    """

    # begin a non-ORM transaction
    trans = db_connection.begin()
    await trans.start()

    # bind an individual Session to the connection
    session = AsyncSession(bind=db_connection, expire_on_commit=False)

    yield session

    await session.close()

    # rollback - everything that happens within the
    # Session below (including calls to commit())
    # is rolled back.
    await trans.rollback()

@pytest.mark.asyncio(loop_scope="session")
async def test_something(db_session):
    # I want this test to use the event loop from the session fixture
    pass

Running the test gives me this error:

raise MultipleEventLoopsRequestedError( E pytest_asyncio.plugin.MultipleEventLoopsRequestedError: Multiple asyncio event loops with different scopes have been requested E by test/test_me.py::test_something. The test explicitly requests the event_loop fixture, while E another event loop with session scope is provided by . E Remove "event_loop" from the requested fixture in your test to run the test E in a session-scoped event loop or remove the scope argument from the "asyncio" E mark to run the test in a function-scoped event loop.

[seifertm]

@greemo I cannot comment on the error message, but your use case should be supported.

Starting with v0.24 fixtures can specify different scopes for caching (scope) and for the eventloop (fixturescope). Try add loop_scope="session" to all fixture decorators in your example. @pytest_asyncio.fixture(loop_scope="session", scope="function") should give the desired behavior for _dbsession.

[commonism]

As I think communication/documentation will be key to overcome this breaking change and it's fallout,

Using the example from https://github.com/pytest-dev/pytest-asyncio/issues/706#issuecomment-1925604186

code for 0.24 is

import asyncio
import random
from typing import Optional, List
import sys
import inspect

import pytest
import pytest_asyncio

from hypercorn.asyncio import serve
from hypercorn.config import Config
import uvloop

from fastapi import FastAPI
import httpx

app = FastAPI(
    version="1.0.0", title="pytest-dev/pytest-asyncio#706",
    servers=[{"url": "/", "description": "Default, relative server"}]
)

@app.get("/random", operation_id="getRandom", response_model=List[int])
def getRandom(limit: Optional[int] = 3) -> List[int]:
    return [random.randrange(0, 6) for _ in range(limit)]

@pytest.fixture(scope="session")
def config(unused_tcp_port_factory):
    c = Config()
    c.bind = [f"localhost:{unused_tcp_port_factory()}"]
    return c

@pytest_asyncio.fixture(loop_scope="session")
async def server(config):
    event_loop = asyncio.get_event_loop()
    try:
        sd = asyncio.Event()
        task = event_loop.create_task(serve(app, config, shutdown_trigger=sd.wait))
        yield config
    finally:
        sd.set()
        await task

@pytest.fixture(scope="session")
def event_loop_policy():
    return uvloop.EventLoopPolicy()

class Client:
    def __init__(self, url):
        self.c = httpx.AsyncClient()
        self.url = url

    async def get(self, path):
        print(f"{__file__}:{inspect.currentframe().f_lineno} {id(asyncio.get_event_loop())=}")
        return await self.c.get(f"{self.url}/{path}")

@pytest_asyncio.fixture(loop_scope="session")
async def client(server):
    c = Client(f"http://{server.bind[0]}")
    dd = await c.get("openapi.json")
    return c

@pytest.mark.asyncio(loop_scope="session")
async def test_getRandom(client):
    r = await client.get("random")
    assert r.status_code == 200
    assert len(r.json()) == 3

@pytest.mark.asyncio(loop_scope="session")
@pytest.mark.skipif(sys.version_info < (3, 9), reason="requires asyncio.to_thread")
async def test_to_thread(client, server):
    r = await asyncio.to_thread(httpx.get, f"{client.url}/openapi.json")
    assert r.status_code == 200

delta

--- my_pytest_test.py   2024-09-18 16:07:19.630570449 +0200
+++ my_pytest_asyncio.py    2024-10-03 14:07:50.705652032 +0200
@@ -2,6 +2,7 @@
 import random
 from typing import Optional, List
 import sys
+import inspect

 import pytest
 import pytest_asyncio
@@ -19,17 +20,11 @@
 )

-@app.get("/random", operation_id="getRandom", response_model=list[int])
-def getRandom(limit: int | None = 3) -> list[int]:
+@app.get("/random", operation_id="getRandom", response_model=List[int])
+def getRandom(limit: Optional[int] = 3) -> List[int]:
     return [random.randrange(0, 6) for _ in range(limit)]

-@pytest.fixture(scope="session")
-def event_loop(request):
-    loop = asyncio.get_event_loop_policy().new_event_loop()
-    yield loop
-    loop.close()
-

 @pytest.fixture(scope="session")
 def config(unused_tcp_port_factory):
@@ -38,10 +33,9 @@
     return c

-@pytest_asyncio.fixture(scope="session")
-async def server(event_loop, config):
-    policy = asyncio.get_event_loop_policy()
-    asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())
+@pytest_asyncio.fixture(loop_scope="session")
+async def server(config):
+    event_loop = asyncio.get_event_loop()
     try:
         sd = asyncio.Event()
         task = event_loop.create_task(serve(app, config, shutdown_trigger=sd.wait))
@@ -49,34 +43,38 @@
     finally:
         sd.set()
         await task
-    asyncio.set_event_loop_policy(policy)

+@pytest.fixture(scope="session")
+def event_loop_policy():
+    return uvloop.EventLoopPolicy()
+
 class Client:
     def __init__(self, url):
         self.c = httpx.AsyncClient()
         self.url = url

     async def get(self, path):
+        print(f"{__file__}:{inspect.currentframe().f_lineno} {id(asyncio.get_event_loop())=}")
         return await self.c.get(f"{self.url}/{path}")

-@pytest_asyncio.fixture(scope="session")
-async def client(event_loop, server):
+@pytest_asyncio.fixture(loop_scope="session")
+async def client(server):
     c = Client(f"http://{server.bind[0]}")
     dd = await c.get("openapi.json")
     return c

-@pytest.mark.asyncio
+@pytest.mark.asyncio(loop_scope="session")
 async def test_getRandom(client):
     r = await client.get("random")
     assert r.status_code == 200
     assert len(r.json()) == 3

-@pytest.mark.asyncio
+@pytest.mark.asyncio(loop_scope="session")
 @pytest.mark.skipif(sys.version_info < (3, 9), reason="requires asyncio.to_thread")
-async def test_to_thread(client):
+async def test_to_thread(client, server):
     r = await asyncio.to_thread(httpx.get, f"{client.url}/openapi.json")
     assert r.status_code == 200

No reason to be afraid of this any longer.