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

Maintain contextvars between fixtures and tests #1008

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

bcmills
Copy link
Contributor

@bcmills bcmills commented Dec 6, 2024

The approach I've taken here is to inspect the context after running the fixture setup task and copy any changed values out into the test's ambient context. When we run the finalizer to tear down the fixture, we then reset the modified variables.

Some limitations:

Fixes #127.

The approach I've taken here is to maintain a contextvars.Context
instance in a contextvars.ContextVar, copying it from the ambient
context whenever we create a new event loop. The fixture setup
and teardown run within that context, and each test function gets
a copy (as if it were created as a new asyncio.Task from within the
fixture task).

Fixes pytest-dev#127.
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.42%. Comparing base (a1cd861) to head (7e2da55).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1008      +/-   ##
==========================================
+ Coverage   90.83%   91.42%   +0.58%     
==========================================
  Files           2        2              
  Lines         513      548      +35     
  Branches       66       72       +6     
==========================================
+ Hits          466      501      +35     
  Misses         28       28              
  Partials       19       19              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 7, 2024

  • If a synchronous fixture runs after the corresponding event loop has already been created, and it modifies context vars in the thread's current context, those changes will not propagate into async fixtures and functions.

It occurs to me that we can get rid of this caveat by copying the modified values out to the ambient context after running the setup phase, and resetting them after the finalizer. I'll look into that approach.

Instead of storing a Context in a context variable, just copy out the
changes from the setup task's context into the ambient context, and
reset the changes after running the finalizer task.
@bcmills
Copy link
Contributor Author

bcmills commented Dec 7, 2024

It occurs to me that we can get rid of this caveat by copying the modified values out to the ambient context after running the setup phase, and resetting them after the finalizer.

Done, and I'm even happier with this approach!

@bcmills bcmills changed the title Maintain contextvars.Context in fixtures and tests Maintain contextvars between fixtures and tests Dec 7, 2024
pytest_asyncio/plugin.py Outdated Show resolved Hide resolved
Copy link
Contributor

@seifertm seifertm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You put a lot of effort into this, thanks! I find this a fairly elegant way to address the long-standing problem with context vars in pytest-asyncio. Your use of helper functions makes the code easy to follow and there are tests to support the change.

I'd appreciate if:

  1. You addressed my comment regarding the structure of the tests.
  2. You added a corresponding entry in docs/reference/changelog.rst that mentions the support for contextvars and the limitation to Python >=3.11.

pytest_asyncio/plugin.py Outdated Show resolved Hide resolved
tests/async_fixtures/test_async_fixtures_contextvars.py Outdated Show resolved Hide resolved
Also add type annotations for _create_task_in_context.
@bcmills
Copy link
Contributor Author

bcmills commented Dec 11, 2024

Thanks for the review! I think I've addressed all of your comments, but please let me know if I missed something.

Copy link
Contributor

@seifertm seifertm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what I had in mind, thanks!

@seifertm seifertm added this pull request to the merge queue Dec 12, 2024
@seifertm seifertm added this to the v0.25 milestone Dec 12, 2024
Merged via the queue into pytest-dev:main with commit f15b9c2 Dec 12, 2024
15 checks passed
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
3 participants