Skip to content
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

Other event loop scopes? #657

Closed
2e0byo opened this issue Oct 31, 2023 · 20 comments · Fixed by #667
Closed

Other event loop scopes? #657

2e0byo opened this issue Oct 31, 2023 · 20 comments · Fixed by #667
Milestone

Comments

@2e0byo
Copy link

2e0byo commented Oct 31, 2023

The recent changes deprecated redefining the event_loop fixture, but provided no replacement for scope=package. Presumably the move away from the fixture was taken for good reasons, but I'd like to suggest support for (at least) package scope is worth having.

Package scope is really handy: you can have a directory structure like this:

a/
├─ test_foo.py
├─ test_bar.py
b/
├─ test_baz.py
├─ test_blah.py

with expensive fixtures per-directory, spin them up once per dir and then clean them up (freeing ram) before moving to the next dir.

I know this will work just fine with a session scoped event loop (which is also no longer possible?), but matching the event loop scope to the context has advantages:

  • every new testing context (new package) starts in a clean loop
  • failing fixtures or leftover state (locks, unawaited coros) throw at the package level, so you get debugging quickly
  • certain exotic failure modes where stale state on the loop is inadvertently cleaned up elsewhere (ahem) only show if the loop is explicitly torn down on exiting the context, effectively asserting nothing is running on it

Thus I'd like to suggest support for package scope is added to asyncio_event_loop. For completeness, the scopes are:

  • function: default
  • class: already available
  • module: already available
  • package: missing
  • session: missing

I also think it is worth implementing session scoping, although I think package is normally the right scope. It's possible I'm missing something here and this is already possible.

@seifertm
Copy link
Contributor

seifertm commented Oct 31, 2023

That's a fair point.

I deliberately omitted session scope, but I wasn't aware of package scope.

Do you have suggestion how package-scoped (and session-scoped) loops should be defined?

@val-melev
Copy link

what's the rationale for omitting session scope?

@seifertm
Copy link
Contributor

what's the rationale for omitting session scope?

There were essentially two ideas for replacing event loop fixture overrides. The first was to have a config option (similar to asyncio_mode) that defines the scope of the event loop. However, this would force all tests in a project to use the same event loop scope. Since a single project can have a mix of small "unit" tests and larger tests (e.g. involving starting a web server), this approach seemed too limiting.

The other option was to tie the loop scope to a pytest mark. I'm not aware of a way for users to add a mark to the whole session, unless they know about pytest hooks. So in a way, it was an accepted drawback of the current solution.

@val-melev Do you have a use case that requires session-scoped loops? If so, can you share it?

@2e0byo
Copy link
Author

2e0byo commented Oct 31, 2023

I deliberately omitted session scope, but I wasn't aware of package scope.

I think if you add package, you probably end up adding session as well: package scope in the top level package looks like session to me, although I don't know if pytest will create/tear down in a slightly different place.

Do you have suggestion how package-scoped (and session-scoped) loops should be defined?

Do you mean behaviour? In which case I think the old (fixture) behaviour is good: the loop gets created when the first test in the package requests it, which happens when you run the first async test, and is destroyed when the last test in the package releases it.

If you mean implementation I don't know. I need to look at the new code and its motivation before I can suggest something.

As an aside, I wonder if the current behaviour is more breaking than it might appear. It's quite common in the db/backend world to have things like engines or session factories which are bound to an event loop when first accessed and throw if that loop goes away. Whilst you can handle this all in fixtures, it makes sense in integration testing (in my opinion) to let the code do it it's normal way and just override the configuration. I agree that long-lived sessions of any kind don't normally belong in unit testing.

gpauloski added a commit to proxystore/proxystore that referenced this issue Oct 31, 2023
pytest-asyncio v0.22.0 deprecates overriding the event_loop fixture,
and replacing the event_loop fixture with the asyncio_event_loop mark
breaks our session scoped asyncio fixtures (e.g., the relay_server
fixture).

This commit doesn't fix this problem, but at least defers the problems
for some time while I figure out how to correctly instrument our shared
asyncio fixtures.

Related:
pytest-dev/pytest-asyncio#657
pytest-dev/pytest-asyncio#658
@seifertm
Copy link
Contributor

Do you have suggestion how package-scoped (and session-scoped) loops should be defined?

Do you mean behaviour? In which case I think the old (fixture) behaviour is good: the loop gets created when the first test in the package requests it, which happens when you run the first async test, and is destroyed when the last test in the package releases it.

I was rather wondering how you'd write your test code so it gives you a package or session scoped loop.
Given that event loop fixture overrides are going to be deprecated and you cannot @pytest.mark packages or sessions, I'm looking for ideas :)

@2e0byo
Copy link
Author

2e0byo commented Oct 31, 2023

hmm, I can think of a few options, though I'm just brainstorming:

  1. don't deprecate fixtures, at least in the package/session case (I really need to go and read the thread to understand if this is a stupid suggestion, but if it's just about stopping users cutting themselves this could be a way to go: overlapping fixtures would go away, and exactly one of session_event_loop or package_event_loop could be overridden, implemented by default as yield None. I don't know if this would work with the new code.)

2. use a fixture with the right scope to set a mark on the test, via pytestconfig. Off the top of my head I don't know whether that would work in terms of lifetimes, but if it would I'd personally consider it an acceptable workaround. I don't mind using hooks, I just don't want to reimplement collection. Edit sorry, this is nonsense isn't it, as the fixture will only get evaluated once, so it only has access to one of the function names.

  1. use a hook and do it at collection by inspecting the module. I'd really like to avoid this. Most python devs don't use pytest hooks but do use marks/fixtures. If I do hook magic inside a fixture called register_custom_event_loop anyone can grok what it does from the name and where from the fixture scope.

If you have it handy, could we link this issue to the discussion which motivated this change?

@kusaku
Copy link

kusaku commented Oct 31, 2023

@val-melev Do you have a use case that requires session-scoped loops? If so, can you share it?

For example, the presence of other async fixtures that have a scope wider than function will result in You tried to access the function scoped fixture event_loop with a module scoped request object, involved factories: ...

This is why we "extend" the standard scope of the event_loop fixture.

@mthpower
Copy link

Hi, I hope it's helpful to this discussion if I can share my use case for an async fixture which we scope to the session.

# conftest.py

@pytest.fixture(scope="session")
async def test_database(database_url: URL) -> URL:
    await create_database(database_url)
    return database_url

We use a fixture like this to create a database to test a web application. We want the database to be created at the point when we start the test session, and reuse that test database for the whole session. This fixture requests the event loop fixture, with session scope.

So far, we've been using a fixture like so:

# conftest.py

@pytest.fixture(scope="session")
def event_loop() -> Iterator[asyncio.AbstractEventLoop]:
    loop = asyncio.new_event_loop()
    yield loop
    loop.close()

From what I've read so far, I don't see a way to make this work with version 0.22.0.

@2e0byo
Copy link
Author

2e0byo commented Nov 1, 2023

@seifertm (and everyone else) how would you feel about this interface?

@pytest.fixture(scope="package", autouse=True)
def custom_event_loop_scope() -> str:
    return "package"

Or, IMHO clearer:

from pytest_asyncio import SCOPE

@pytest.fixture(scope="package", autouse=True)
def custom_event_loop_scope() -> SCOPE:
    return SCOPE.package

? This could be inspected in the collection code just like a mark and appropriate action taken.

Arguments in favour:

  • other packages use 'config fixtures' a lot, e.g. pytest-recording, pytest itself (arguably, with e.g. pytest_addoption)
  • fixtures are trivial to broadcast over scopes, but marks are neater in the function/class/module case, so we can have both
  • from a glance through the code now I don't think this would be very hard to implement, and it doesn't really add two logic flows like having both the mark and overriding the loop fixture does.

Alternatively the fixture could supply an eventloop policy directly. That might be a fix for #646, since fixture parametrisation would push the problem out of pytest-asyncio's flow?

@pytest.fixture(scope="package")
def event_loop_policy() -> AbstractEventLoopPolicy:
    return DefaultEventLoopPolicy()

This form I think is more powerful, but it would make contradictions less obvious between e.g. a function marked for module scope event loop but requesting a package level event loop policy. We'd still have the scope, so we could just raise at collection.

If this is something you'd be interested in I can try to prototype it at the weekend.

@lindycoder
Copy link

lindycoder commented Nov 1, 2023

Sharing use case from the other issue

We have session scoped async fixtures so we have this central fixture

@pytest.fixture(scope="session")
def event_loop() -> Generator[AbstractEventLoop, None, None]:
    """Make the loop session scope to use session async fixtures."""
    policy = asyncio.get_event_loop_policy()
    loop = policy.new_event_loop()
    yield loop
    loop.close()

And every single AsyncGenerator fixtures we have actually require event_loop to make sure the teardown is done before the loop is closed.

The main use case for us is we run black box tests using containers we launch part of session scoped fixtures and we return an httpx client configured to talk to the container which we async with AsyncClient(...) . Of course it would be possible to split the client in a lesser scoped fixture... but keeping it there is handy. We probably have other use-cases that i don't think of.

@2e0byo
Copy link
Author

2e0byo commented Nov 3, 2023

Incidentally I've found a few cases in my code where I deliberately requested the event loop fixture in a sync test (either because I needed to interact with a running event loop to test e.g. inspecting/getting loops, but all the methods were sync, or because something deep under the hood would need an event loop but the interface was sync). I don't think it's a problem to deprecate this pattern as you can get exactly the same effect by declaring the test as async def, but it might be worth mentioning in the release notes. I've no idea how common a usage it is.

@seifertm
Copy link
Contributor

seifertm commented Nov 3, 2023

It's become clear that larger loop scopes need to be supported by pytest-asyncio, so this issue is definitely going to be implemented. Thanks for sharing your use cases and API proposals.

The current solution with the asyncio_event_loop mark has two problems:

  1. Usually, applying a pytest mark to a class, for example, applies that mark to all tests in the class and that mark affects each test. asyncio_event_loop works slightly differently. It changes the event loop scope of tests inside the marked class or module. I think this could be surprising for users.
  2. Packages and sessions cannot be easily marked.

other packages use 'config fixtures' a lot, e.g. pytest-recording, pytest itself (arguably, with e.g. pytest_addoption)

@2e0byo Thanks for establishing precedent for configuration fixtures. It means that users are more likely to be familiar with them. Technically, the event_loop fixture is also a configuration fixture. The problem is that people write all kind of code there. This results in a large number of things that can go wrong which—in turn—makes it difficult to change pytest-asyncio without breaking someone's code. However, I can imagine that fixtures returning a plain string or an enum value, such as in your example, are less problematic.

from a glance through the code now I don't think this would be very hard to implement, and it doesn't really add two logic flows like having both the mark and overriding the loop fixture does.

Looking at my average "lines of code per hour spent" I'd say nothing in pytest-asyncio is easy to implement :)

I don't see a way around the split logic in the next release. Users need to be made aware about deprecated functionality (i.e. event loop fixture overrides) and about its replacement functionality. Therefore, both ways must coexist in at least one release. Given the use of pytest-asyncio, I don't see a breaking change without any heads-up information to be an option.

Let's try the approach with a dedicated fixture for the loop policy as proposed here. Note that the event loop policy system is going to be deprecated in CPython 3.13 in favour of the loop_factory arguments to asyncio.run and asyncio.Runner. Therefore, I'd prefer we stick to the new naming and call the fixture asyncio_loop_factory, instead.

Scoped event loops in v0.22 require the presence of an asyncio_event_loop mark to determine the loop policy. Once the asyncio_loop_factory fixture is in place, we can unconditionally attach an event loop to each pytest collector (Class, Module, Package, Session) and the underlying loop policy is resolved dynamically.

Once the loop factory fixture is in place, we can extend the existing asyncio mark with an optional scope kwarg. scope defaults to "function", when missing, to keep the code backwards compatible. Remember that each collector has an event loop. Depending on the chosen scope, each async test can decide in which scope (i.e. in which loop) it runs. The following example runs the two tests in the same even loop:

class TestClassScopedLoop:
    @pytest.mark.asyncio(scope="class")
    async def test_a(self):
        ...
    @pytest.mark.asyncio(scope="class")
    async def test_b(self):
        ...

This would also mean that we can keep the standard pytest mark behaviour. The following example is equivalent to the former:

@pytest.mark.asyncio(scope="class")
class TestClassScopedLoop:
    async def test_a(self):
        ...
    async def test_b(self):
        ...

Trivially, the scope can also be function (the default), module, package, or session.

The scope kwarg to the asyncio mark and the asyncio_loop_factory fixture would replace the asyncio_event_loop mark introduced in v0.22. Some playing around with the current code on main suggests this solution is feasible.

What's your opinion on this approach?

@seifertm seifertm added this to the v0.23 milestone Nov 3, 2023
@seifertm
Copy link
Contributor

seifertm commented Nov 3, 2023

Let's try the approach with a dedicated fixture for the loop policy as proposed #657 (comment). Note that the event loop policy system is going to be python/cpython#94597 in favour of the loop_factory arguments to asyncio.run and asyncio.Runner. Therefore, I'd prefer we stick to the new naming and call the fixture asyncio_loop_factory, instead.

On second thought, it would be fine to have a fixture for an event loop policy. We don't have to be faster with deprecating things than upstream.

@seifertm
Copy link
Contributor

seifertm commented Nov 3, 2023

Incidentally I've found a few cases in my code where I deliberately requested the event loop fixture in a sync test (either because I needed to interact with a running event loop to test e.g. inspecting/getting loops, but all the methods were sync, or because something deep under the hood would need an event loop but the interface was sync). I don't think it's a problem to deprecate this pattern as you can get exactly the same effect by declaring the test as async def, but it might be worth mentioning in the release notes. I've no idea how common a usage it is.

Thanks for mentioning that. There are no plans to deprecate the default event_loop fixture for sync tests in the upcoming release.

We have to keep in mind that other projects depend on event_loop for compatibility with pytest-asyncio, for example pytest-aiohttp.

@2e0byo
Copy link
Author

2e0byo commented Nov 3, 2023

On second thought, it would be fine to have a fixture for an event loop policy. We don't have to be faster with deprecating things than upstream.

I didn't realise this was coming, but I'm glad (a policy kind of is a factory anyway). On the other hand it looks like a new type will be used, so (ignoring duck typing) it might be better to keep the new name available.

The scope kwarg to the asyncio mark and the asyncio_loop_factory fixture would replace the asyncio_event_loop mark introduced in v0.22. Some playing around with the current code on main suggests this solution is feasible.

What's your opinion on this approach?

I much prefer it. The kwarg to the asyncio mark is much cleaner IMHO, especially when @pytest.mark.asyncio is used manually and v0.22 would have required two marks.

I'll have a readthrough the WIP and follow that PR.

Thanks for being extremely quick to respond to suggestions, and for an excellent library!

@ffissore
Copy link

ffissore commented Nov 7, 2023

Hi all, and thx for your work. I don't know how helpful this comment will be but I'd like to share the way we use pytest-asyncio

  • we override the event_loop fixture, making it session-scoped
  • we run tests in auto mode, so we don't need to write boilerplate decorators
  • we write plain test_* functions (we don't use test classes)

A session-scoped event loop is crucial for us, as all our services share some heavy-weight async fixtures that must start at the very beginning

@seifertm
Copy link
Contributor

Thanks to everyone who participated in this issue. Sharing your use cases and ideas is the only way pytest-asyncio can make good design decisions and remain useful.

pytest-asyncio v0.23.0a0 is available on PyPI and adds an optional scope keyword argument to the asyncio mark to control the scope used for each test. I'd appreciate your feedback on the pre-release version.

@ArtemIsmagilov
Copy link

Thanks to everyone who participated in this issue. Sharing your use cases and ideas is the only way pytest-asyncio can make good design decisions and remain useful.

pytest-asyncio v0.23.0a0 is available on PyPI and adds an optional scope keyword argument to the asyncio mark to control the scope used for each test. I'd appreciate your feedback on the pre-release version.

Guys, are working for me. I suffered all day with the problem of inconsistency of asyncpg, sqlalchemy and asyncio. I add myself to requirements.txt pytest-asyncio==0.23.0a0

  • test_app.py
from app.sql_app.crud import User
from app.sql_app.database import get_conn
from tests.conftest import Conf, mm_user_id
import pytest


@pytest.mark.asyncio(scope="class")
class TestHelp:
    endpoint = "/help_info"

    async def test_help_info(self, client):
        data = {'context': {'acting_user': {'id': mm_user_id, 'username': Conf.test_client_username}}}

        response = await client.post(self.endpoint, json=data)

        assert response.status_code == 200


@pytest.mark.asyncio(scope="class")
class TestConnections:
    endpoint = "/connections"

    @pytest.mark.parametrize('login,token,timezone', (
            ('', '', ''),
            ("fake_login", 'fake_token', 'UTC'),
            ("invalid_data", 'invalid_data', 'UTC'),
    ))
    async def test_fail_connect_account(self, client, login, token, timezone):
        data = {'context': {'acting_user': {'id': mm_user_id, 'username': Conf.test_client_username}},
                # mattermost form data
                'values': {'login': login,
                           'token': token,
                           'timezone': {'value': timezone},
                           }
                }
        response = await client.post(self.endpoint + "/connect_account", json=data)
        json_resp = await response.json
        assert json_resp['type'] == 'error'

    async def test_fail_connect_for_exist_account(self, client):
        data = {'context': {'acting_user': {'id': mm_user_id, 'username': Conf.test_client_username}},
                # mattermost form data
                'values': {'login': Conf.test_client_ya_login,
                           'token': Conf.test_client_ya_token,
                           'timezone': {'value': Conf.test_client_ya_timezone},
                           }
                }
        async with get_conn() as conn:
            # create test_user connection
            await User.add_user(
                conn,
                mm_user_id,
                Conf.test_client_ya_login,
                Conf.test_client_ya_token,
                Conf.test_client_ya_timezone,
                e_c=False,
                ch_stat=False
            )

        response = await client.post(self.endpoint + "/connect_account", json=data)

        async with get_conn() as conn:
            # delete testuser after connection
            await User.remove_user(conn, mm_user_id)

        json_response = await response.json
        assert response.status_code == 200
        assert json_response['type'] == 'error'
  • conftest.py
import asyncio, pytest
from httpx import HTTPError

from app import create_app
import settings
from app.bots.bot_commands import get_user_by_username, create_user

# init testing environments
settings.Conf = settings.Testing
from settings import Conf

# create test user, if it doesn't exist
test_user = asyncio.get_event_loop().run_until_complete(
    get_user_by_username(username=Conf.test_client_username))

if isinstance(test_user, HTTPError):
    test_user = asyncio.get_event_loop().run_until_complete(create_user({
        'email': Conf.test_client_email,
        'username': Conf.test_client_username,
        'first_name': Conf.test_client_first_name,
        'last_name': Conf.test_client_last_name,
        'nickname': Conf.test_client_nickname,
        'password': Conf.test_client_password,
    }))

mm_user_id = test_user['id']


@pytest.fixture  # (scope="session")
def app():
    app = create_app()

    yield app


@pytest.fixture  # (scope="session")
def client(app):
    return app.test_client()


@pytest.fixture  # (scope="session")
def runner(app):
    return app.test_cli_runner()

@seifertm
Copy link
Contributor

@ArtemIsmagilov Thanks for testing the alpha release. My understanding is that you're running into problems with pytest-asyncio v0.23.0a0 and that you have provided the code examples for us to look into the problems.

Unfortunately, your example is incomplete, so there's no way for me to run it. It also involves setting up configurations and databases that I have no knowledge of. What's more, I don't know the error message you're getting.

I'm happy to look into your issue if you can provide a minimal, reproducible example. Otherwise, my hands are tied.

@2e0byo
Copy link
Author

2e0byo commented Nov 13, 2023

Thanks for the very quick turnaround on this! I've opened a separate issue with a new bug, but I do appreciate how responsive you've been over this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants