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

Fix contextvars not propagated from fixture to test #161

Closed
wants to merge 2 commits into from

Conversation

linw1995
Copy link

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 #127

@linw1995
Copy link
Author

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

@Tinche
Copy link
Member

Tinche commented Jun 1, 2020

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

@jpic
Copy link

jpic commented Jan 13, 2021

Is this an alternative to #153 ?

@linw1995
Copy link
Author

linw1995 commented Jan 13, 2021

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?

@jpic
Copy link

jpic commented Feb 7, 2021

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

Thank you so much ❤️

🎩

@zbentley
Copy link

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!

@joeblackwaslike
Copy link

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.

@seifertm
Copy link
Contributor

seifertm commented Apr 5, 2022

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] python/cpython#31799

@joeblackwaslike
Copy link

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.

@seifertm
Copy link
Contributor

seifertm commented Sep 6, 2022

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
Copy link
Contributor

seifertm commented Sep 6, 2022

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.

@seifertm seifertm closed this Sep 6, 2022
@joeblackwaslike
Copy link

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!

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

Successfully merging this pull request may close these issues.

ContextVar not propagated from fixture to test
6 participants