pytest-asyncio icon indicating copy to clipboard operation
pytest-asyncio copied to clipboard

Fix contextvars not propagated from fixture to test

Open linw1995 opened this issue 4 years ago • 9 comments

I manage to use a global value current_context, that stores the current context after entering the async generator fixture. Apply it before running fixtures set up and test functions. Clean it up after the test functions finished.

Fix https://github.com/pytest-dev/pytest-asyncio/issues/127

linw1995 avatar May 16 '20 14:05 linw1995

There is no one who maintains this repository? If my code has something wrong, please tell me.

linw1995 avatar May 28 '20 11:05 linw1995

I'm just very busy, thank you for your patience :)

Tinche avatar Jun 01 '20 13:06 Tinche

Is this an alternative to #153 ?

jpic avatar Jan 13 '21 13:01 jpic

Is this an alternative to #153 ?

Well yes but actually no. It uses copy_context() to store the context after the yield fixture setup, and then applies this context to run the coroutine tests. It will work fine in some cases.

I use it for a long time, and I got a case that will cause an exception.


var_db = ContextVar("db")


@pytest.fixture
@pytest.mark.asyncio
def ensure_db():
    db = ...
    token = var_db.set(db)
    try:
        yield
    finally:
        var_db.reset(token)

var_db.reset(token) will raise an exception due to applying a new context on its finalizer.

To avoid applying a new context instead of its original context object, the new solution needs to implement two extra functions.

  1. able to access and write the original context object, instead of duplicating it via copy_context.
  2. able to manage all original context objects, apply them on their right coroutines.

There may are two ways to implement the first function. But I haven't enough time to do that.

  1. Change the CPython code to expose APIs that allow us to modify the current context variables.
  2. Set the asyncio.tasks._PyTask as the default task factory, so we can modify the context by accessing the _PyTask._context attribute.

Update

This is a general problem not causing by this pull request but by the pytest-asyncio. See this link for more details about why it is a general problem. https://gist.github.com/linw1995/a50928ed90d5e8c8c1b9575d7d7fe70c pytest-asyncio runs the __anext__ method of the same async_generator in different coroutines.

so, we can say this pull request resolves this #153?

linw1995 avatar Jan 13 '21 14:01 linw1995

Confirmed this is even better than #153 because it shares contextvars from fixtures to test indeed.

Thank you so much :heart:

:tophat:

jpic avatar Feb 07 '21 11:02 jpic

What are the chances of this or #153 making it into the next release of pytest-asyncio?

No problem either way; if it's unlikely we'll publish a wheel of one of those branches temporarily to our internal pypi mirror, but if it's going to ship soon there's no need to spend that time. Just curious!

zbentley avatar Aug 22 '21 19:08 zbentley

Hey guys, first off this is really excellent work in this PR! I noticed however that it's a bit out of date (2020). I was able to apply the same approach to the current branch on master with a little bit of finess (https://github.com/joeblackwaslike/pytest-asyncio/blob/master/pytest_asyncio/plugin.py).

Since this PR is kind of stale and contains some nontrivial conflicts preventing it from being approved/merged, I'm curious what would best help get the ball rolling? I don't believe I can push to the linw1995 repo but could maybe open my own PR from mine if that helps?

Let me know the best approach forward here. I'd personally love to see this merged because without it, it requires a lot of app/request context boilerplate to write integration tests for quart apps. For that reason we're currently vendoring this dependency, which is less than ideal.

joeblackwaslike avatar Apr 01 '22 19:04 joeblackwaslike

It's really unfortunate that this PR has been lying around for such a long time, especially since it seems to be very well tested. It's very nice to hear about a specific use case (Quart tests), though.

At the moment, I'm not a huge supporter of the PR. Pytest-asyncio already tends to break in unexpected ways between releases. I fear that introducing functionality for transplanting the contextvars will open a can of worms. I'd much prefer an approach that reduces complexity, instead.

Python 3.11 introduces asyncio.Runner, a context manager which allows multiple functions to run in the same asyncio.Context. [0][1] There is a proposal to use this in pytest-asyncio, because it would make various things much easier. The condition for asyncio.Runner to be usable is that there's a reasonable way to backport the functionality to earlier Python versions.

@joeblackwaslike To get the ball rolling from my POV: We should look into how asyncio.Runner can be used to run tests and their fixtures in the same asyncio context. Any feedback is appreciated! This is probably not the answer you wanted to hear, sorry :)

[0] https://bugs.python.org/issue47062 [1] https://github.com/python/cpython/pull/31799

seifertm avatar Apr 05 '22 15:04 seifertm

Python 3.11 introduces asyncio.Runner, a context manager which allows multiple functions to run in the same asyncio.Context. [0][1] There is a proposal to use this in pytest-asyncio, because it would make various things much easier. The condition for asyncio.Runner to be usable is that there's a reasonable way to backport the functionality to earlier Python versions.

@joeblackwaslike To get the ball rolling from my POV: We should look into how asyncio.Runner can be used to run tests and their fixtures in the same asyncio context. Any feedback is appreciated! This is probably not the answer you wanted to hear, sorry :)

[0] https://bugs.python.org/issue47062 [1] python/cpython#31799

I'm currently unaware of any attempts to backport asyncio functionality from newer versions of python to older versions so I'm really not sure where to start. If you do that would be useful information. Can you link me to the current proposal to use asyncio.Runner?

When I have some additional free time, I may try to backport the asyncio.Runner class to python 3.7 to see if it's even compatible with an older event loop.

joeblackwaslike avatar Apr 05 '22 16:04 joeblackwaslike

I'm currently unaware of any attempts to backport asyncio functionality from newer versions of python to older versions so I'm really not sure where to start. If you do that would be useful information. Can you link me to the current proposal to use asyncio.Runner?

When I have some additional free time, I may try to backport the asyncio.Runner class to python 3.7 to see if it's even compatible with an older event loop.

#312 is a draft that (among other things) introduces the concept of a Runner to pytest-asnycio. If you (or anyone else) are still willing to give it a go, I'd definitely support it.

seifertm avatar Sep 06 '22 18:09 seifertm

I really hate to do this, but I'm closing this PR in favour of the solution proposed in https://github.com/pytest-dev/pytest-asyncio/pull/312.

I do like the extensive amount of testing you put into the PR! The approach of storing the context in a global variable and transplanting it into each test seems to be very effective as well.

However, Python 3.11 introduces asyncio.Runner, which essentially allows running multiple async functions in the same context. #312 introduces the concept of a Runner in pytest-asyncio. I think this is the approach we should take.

seifertm avatar Sep 06 '22 18:09 seifertm

I really hate to do this, but I'm closing this PR in favour of the solution proposed in #312.

I do like the extensive amount of testing you put into the PR! The approach of storing the context in a global variable and transplanting it into each test seems to be very effective as well.

However, Python 3.11 introduces asyncio.Runner, which essentially allows running multiple async functions in the same context. #312 introduces the concept of a Runner in pytest-asyncio. I think this is the approach we should take.

I was just looking at aioloop-proxy and the Runner Draft PR that uses it and agree this is the ideal direction. Looking forward to seeing this released!

joeblackwaslike avatar Dec 19 '22 11:12 joeblackwaslike