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

Breaking change in 0.23.* #706

Open
tkukushkin opened this issue Dec 4, 2023 · 36 comments
Open

Breaking change in 0.23.* #706

tkukushkin opened this issue Dec 4, 2023 · 36 comments
Labels
Milestone

Comments

@tkukushkin
Copy link

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.

@redtomato74
Copy link

redtomato74 commented Dec 4, 2023

I confirm, we use similar setup with 2 session level fixtures (one to redefine event loop, another for our own purposes), tests don't work anymore, complain either about "The future belongs to a different loop than the one specified as the loop argument" or "Event loop is closed".

@albertferras-vrf
Copy link

The version 0.23.0 changelog is already mentioning the breaking change: https://github.com/pytest-dev/pytest-asyncio/releases/tag/v0.23.0

I went through the same, the new way to do it can be seen in this PR https://github.com/pytest-dev/pytest-asyncio/pull/662/files
Would be nice to have some documentation on to how to migrate to the new way.

@redtomato74
Copy link

It says "This release is backwards-compatible with v0.21. Changes are non-breaking, unless you upgrade from v0.22."

@tkukushkin
Copy link
Author

@albertferras-vrf the changelog mentions asyncio_event_loop mark removal, I think it is only about upgrading from 0.22.

@albertferras-vrf
Copy link

You are right, I forgot about that part

@seifertm
Copy link
Contributor

seifertm commented Dec 4, 2023

It says "This release is backwards-compatible with v0.21. Changes are non-breaking, unless you upgrade from v0.22."

That was the original intention, yes. I can reproduce the difference between v0.21.1 and v0.23 and I agree that this is a breaking change (by accident).

With regards to the migration and as a workaround: The fundamental idea of v0.23 is that each pytest scope (session, package, module, class, and function) provides a separate event loop. You can decide for each test in which loop they run via the new scope kwarg to the asyncio mark.

@tkukushkin In your specific example you want to run the test in the same loop as the fixture. The some_async_fixture fixture has session scope. In order to achieve your goal, you should mark test_something accordingly:

@pytest.mark.asyncio(scope="session")
async def test_something(some_async_fixture):
    assert asyncio.get_running_loop() is some_async_fixture

See also the part about asyncio event loops in the Concepts section of the docs.

@redtomato74 I'd like to hear more about your use case for two different event loops. I suggest you open a separate issue for this.

@seifertm seifertm added the bug label Dec 4, 2023
@seifertm seifertm added this to the v0.23.3 milestone Dec 4, 2023
@tkukushkin
Copy link
Author

@seifertm I'd like to have only one loop for all fixtures and tests, without additional decorators to all tests and fixtures. We have thousands of tests in hundreds of services where all tests and fixtures share one loop and it is crucial for them. Is there any workaround to emulate the old behaviour?

This way https://pytest-asyncio.readthedocs.io/en/v0.23.2/how-to-guides/run_session_tests_in_same_loop.html does not consider fixtures at all(

@seifertm
Copy link
Contributor

seifertm commented Dec 4, 2023

I'd like to have only one loop for all fixtures and tests, without additional decorators to all tests and fixtures.

The linked how-to is supposed to make it easy to add the asyncio mark to all of your tests, specifically for large test suites where marking all packages or modules is cumbersome. Unfortunately, there seem to be problems with it, too. Please refer to #705

We have thousands of tests in hundreds of services where all tests and fixtures share one loop and it is crucial for them.

I don't think this use case was considered during the development of v0.22 and v0.23. Can you explain why you need all tests and fixtures to run the same loop? Why

This way https://pytest-asyncio.readthedocs.io/en/v0.23.2/how-to-guides/run_session_tests_in_same_loop.html does not consider fixtures at all(
That's a fair point. The docs should be updated to explain how fixtures are run.

Fixtures in v0.23 generally behave like tests and choose the event loop of the fixture scope. That means if a fixture has session scope it will run in the session-wide loop.

I cannot think of a workaround to switch to the old behaviour at the moment. I suggest pinning pytest-asyncio to <0.23 until this issue is fixed.

@tkukushkin
Copy link
Author

Fixtures in v0.23 generally behave like tests and choose the event loop of the fixture scope. That means if a fixture has session scope it will run in the session-wide loop.

Sorry, but I don't understand, I have not only session scoped fixtures but also module scoped fixtures, and they should use the same event loop as session scoped fixtures. Could you please describe how to achieve it?

We have thousands of tests in hundreds of services where all tests and fixtures share one loop and it is crucial for them.

I don't think this use case was considered during the development of v0.22 and v0.23. Can you explain why you need all tests and fixtures to run the same loop?

We write blackbox tests for microservices using pytest and pytest-asyncio. Some session scoped fixtures for example create database connection pool, which all tests can use to check database state. Another session scoped fixture in background monitors logs of subprocesses (instances of application, that we test) and captures these logs to some list, which tests can check. These subprocesses can be started by any fixture and test as async context manager. And obviously, subprocess (asyncio.subprocess) should be started with the same loop as fixture that captures logs from it. And we have way more such examples.

@seifertm
Copy link
Contributor

seifertm commented Dec 4, 2023

Thanks for the explanation!

Fixtures in v0.23 generally behave like tests and choose the event loop of the fixture scope. That means if a fixture has session scope it will run in the session-wide loop.

Sorry, but I don't understand, I have not only session scoped fixtures but also module scoped fixtures, and they should use the same event loop as session scoped fixtures. Could you please describe how to achieve it?

This is what I meant when I said your use case hasn't been considered in the development. There's currently no way to control the event loop used by a fixture independently from the fixture scope.

The v0.23 release will not work for your test suite. I suggest that you downgrade to v0.21.1.

I'll think of a way to control the fixture scope independently of the event loop scope.

@tkukushkin
Copy link
Author

Yes, we have already downgraded to 0.21.1. I don't think it's gonna be a problem for us for a long time (at least until Python 3.13).

I'll think of a way to control the fixture scope independently of the event loop scope.

Thank you! Looking forward to the news.

@tonal
Copy link

tonal commented Dec 6, 2023

After upgrade to v 0.23 error in teardown:

@pytest.fixture
def server():
  from main import _create_fastapy_server
  app = _create_fastapy_server()
  return app

@pytest_asyncio.fixture
async def client_async(server):
    app = server
    async with (
      app.router.lifespan_context(app),
      AsyncClient(app=app, base_url="http://testserver") as client
    ):
      yield client

@pytest.mark.asyncio
async def test_server(client_async):
  """Start - Stop"""

Output:

tests/test_server.py:63 (test_server)
def finalizer() -> None:
        """Yield again, to finalize."""
    
        async def async_finalizer() -> None:
            try:
                await gen_obj.__anext__()
            except StopAsyncIteration:
                pass
            else:
                msg = "Async generator fixture didn't stop."
                msg += "Yield only once."
                raise ValueError(msg)
    
>       event_loop.run_until_complete(async_finalizer())

/home/tonal/.pyenv/versions/epool-smsserver/lib/python3.11/site-packages/pytest_asyncio/plugin.py:336: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/tonal/.pyenv/versions/3.11.6/lib/python3.11/asyncio/base_events.py:628: in run_until_complete
    self._check_closed()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <_UnixSelectorEventLoop running=False closed=True debug=True>

    def _check_closed(self):
        if self._closed:
>           raise RuntimeError('Event loop is closed')
E           RuntimeError: Event loop is closed

/home/tonal/.pyenv/versions/3.11.6/lib/python3.11/asyncio/base_events.py:519: RuntimeError
sys:1: RuntimeWarning: coroutine '_wrap_asyncgen_fixture.<locals>._asyncgen_fixture_wrapper.<locals>.finalizer.<locals>.async_finalizer' was never awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
/home/tonal/.pyenv/versions/epool-smsserver/lib/python3.11/site-packages/aiohttp/client.py:357: ResourceWarning: Unclosed client session <aiohttp.client.ClientSession object at 0x7fbfa55246f0>
  _warnings.warn(
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/home/tonal/.pyenv/versions/epool-smsserver/lib/python3.11/site-packages/aiohttp/connector.py:277: ResourceWarning: Unclosed connector <aiohttp.connector.TCPConnector object at 0x7fbfa5b67ad0>
  _warnings.warn(f"Unclosed connector {self!r}", ResourceWarning, **kwargs)
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/home/tonal/.pyenv/versions/3.11.6/lib/python3.11/asyncio/sslproto.py:119: ResourceWarning: unclosed transport <asyncio._SSLProtocolTransport object>
  _warnings.warn(
/home/tonal/.pyenv/versions/3.11.6/lib/python3.11/asyncio/selector_events.py:864: ResourceWarning: unclosed transport <_SelectorSocketTransport fd=17>
  _warn(f"unclosed transport {self!r}", ResourceWarning, source=self)
ResourceWarning: Enable tracemalloc to get the object allocation traceback

@ffissore
Copy link

@seifertm while I deeply appreciate your work, which is crucial for all python and pytest users dealing with async tests, I don't understand why you had to change the way the event loop is set up: the previous way worked just fine.

Please consider restoring the previous behaviour.

As for use cases, a few people provided their setups and needs in #657 (and this was my comment)

@seifertm
Copy link
Contributor

@ffissore Thanks for the kind words and for being so upfront. I'm generally open to restoring the previous behavior, if the existing problems with it can be solved in another way. Before I give a more extensive answer:

Do you take an issue with the bugs and the incompatibilities that were (involuntatrily) introduced in v0.23? Or do you think the new approach is generally flawed?

@oroulet
Copy link

oroulet commented Dec 17, 2023

I did not follow exactly the issue, but I wanted to mention that 0.23 broke all our pipelines at work with some strange errors and the same with one of the open source project I maintain: https://github.com/FreeOpcUa/opcua-asyncio . The solution so far has been to revert to 0.21 everywhere

@ffissore
Copy link

I'm afraid I'm not in a position to judge the approach: I don't know enough about the previous design and the desired long-term design.
I can see how switching from event loop to even loop policy still allows folks to switch to alternative event loop implementations (we, for example, use uvloop) but also how it makes it harder to define custom scopes, or even different scopes depending on the test or test package.

IMHO the best solution is the one that makes the end developer write as little code as possible, and only code that is strictly related to what the dev wants to do.
With this in mind, overriding the event_loop fixture was good enough: it's 5 lines of code written in the proper conftest file. Markers instead are not good, as it's lots of boilerplate for specifying the same setting all over a test suite: makers should better be used to specify exceptions, not rules.

@ramnes
Copy link

ramnes commented Dec 18, 2023

It was good enough but a terrible DX honestly. It took me quite a while to figure out the first time I've seen this problem (and thought it was a bug or very bad design). If the goal here is that we don't need these five lines anymore, I'm all in and will just pin my dependency until we update our code. Let's not revert to something ugly if the new approach is better.

Also, the semantic versioning meaning of 0.* versions is that they can introduce breaking changes anytime. If you don't want to hit this kind of breaking changes in the first place, just pin your dependencies...

@ramnes
Copy link

ramnes commented Dec 18, 2023

I think the real problem is that the migration process is not clear. I would have expected that removing the event_loop fixture would be enough with asyncio_mode = auto, but it's not.

Here is a minimal reproducible example (with GitHub CI!) of a real world application that I can't migrate to 0.23 so far: https://github.com/ramnes/pytest-asyncio-706

@seifertm Any hint on how something like this should be migrated? (PR welcome on that repository.) If it's not possible to migrate, then this would be the real issue: it wouldn't be a breaking change but a loss of functionality. Otherwise we'll probably have a few bits to add to the documentation here. :)

commonism added a commit to commonism/aiopenapi3 that referenced this issue Feb 15, 2024
commonism added a commit to commonism/aiopenapi3 that referenced this issue Feb 15, 2024
dotlambda added a commit to dotlambda/nixpkgs that referenced this issue Feb 23, 2024
e-gulakhmet added a commit to e-gulakhmet/HomeFinancier that referenced this issue Mar 9, 2024
@nerlijman
Copy link

We also needed to downgrade to 0.21.1 to make our project work. Waiting for a new update. Thanks!

Cynerd added a commit to silicon-heaven/pyshv that referenced this issue Apr 9, 2024
Prevents issue reported here and revert is the suggested way:
pytest-dev/pytest-asyncio#706

Tests in this repository are not affected at the moment because event
loops are used only in function scope fixtures but this prevents
possible confusion in the future when that would not be the case.
rgildein added a commit to rgildein/charm-apt-mirror that referenced this issue Apr 11, 2024
The functional tests requiered some changes, they were done strangely
and I'm not sure why they worked before. I think it's maybe due [1],
but I did not have time to investigate more.

---
[1]: pytest-dev/pytest-asyncio#706
rgildein added a commit to canonical/charm-apt-mirror that referenced this issue Apr 16, 2024
* Set controller channel to 3.4 in CI

The functional tests requiered some changes, they were done strangely
and I'm not sure why they worked before. I think it's maybe due [1],
but I did not have time to investigate more.

---
[1]: pytest-dev/pytest-asyncio#706

* tmp fix until [1] is not resulved

---
[1]: canonical/charmcraft#1640

* drop bootstack-actions for func tests

* define concurrency in CI

* dropping tmp usage of 5.20/stable for LXD

* Switch back to bootstack-actions
ffissore added a commit to ffissore/pytest-asyncio that referenced this issue Apr 29, 2024
This unblocks all users stuck with 0.21.1 due to pytest-dev#706
@ffissore
Copy link

ffissore commented Apr 29, 2024

With the latest release of pytest, 8.2.0, tests fail with AttributeError: 'FixtureDef' object has no attribute 'unittest' when run with pytest-asyncio==0.21.1.
This has been fixed in #800 and 0.23.6 but we can't use that version.

I opened #823 that backports the fix done in #800 to 0.21.1

cc @bluetech and @seifertm

@houmie
Copy link

houmie commented Apr 29, 2024

I was afraid this day would come. Thanks for keeping 0.21.1 alive.

wxtim added a commit to wxtim/cylc that referenced this issue Apr 29, 2024
We cannot use pytest-async 0.23 because of pytest-dev/pytest-asyncio#706
We need to use pytest-async 0.23 if we use pytest 8.2 because of pytest-dev/pytest#12269
This may be fixed bt this backport of the fix in pytest-async 0.23: pytest-dev/pytest-asyncio#823
seifertm pushed a commit that referenced this issue Apr 29, 2024
This unblocks all users stuck with 0.21.1 due to #706
@ffissore
Copy link

pytest-asyncio==0.21.2 is now available and works well with pytest==8.2.0

@wxtim
Copy link

wxtim commented Apr 30, 2024

pytest-asyncio==0.21.2 is now available and works well with pytest==8.2.0

Epic-quick fix. Many thanks @ffissore .

wxtim added a commit to wxtim/cylc that referenced this issue Apr 30, 2024
We cannot use pytest-async 0.23 because of pytest-dev/pytest-asyncio#706
We need to use pytest-async 0.23 if we use pytest 8.2 because of pytest-dev/pytest#12269
This may be fixed bt this backport of the fix in pytest-async 0.23: pytest-dev/pytest-asyncio#823

Test MacOs using Python 3.8 becuase GHA no longer providing 3.7 on that platform
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests