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

Implemented contextvars isolation in test runner using distinct tasks for each scope #616

Closed
wants to merge 2 commits into from

Conversation

jhominal
Copy link
Contributor

This is an implementation of contextvars isolation between tests, using distinct tasks and queues for each pytest scope.

I am sure that there is a lot to review (e.g. the _TaskManager name is not very inspired ;)), but it seems that the basic functionality works. (The documentation should also be updated to document this new behaviour of the test runner)

I am going to implement the solution to issue #614 on top of the implementation in this branch, when I have a bit more time, but in the meantime I welcome any and all feedback.

@agronholm
Copy link
Owner

Nice! I'll try to find time for a review in the coming week.

@agronholm
Copy link
Owner

Looking at this, I have just one concern: do contextvars still propagate from higher scoped fixtures to lower scoped fixtures (e.g. module -> function)?

@jhominal
Copy link
Contributor Author

Yes, higher scoped fixtures still propagate to lower scoped fixtures - that is thanks to pytest ordering guarantees, that higher scoped fixtures run before lower scoped ones. However, this propagation is implemented by making lower scoped fixtures run on a copy of the higher level context, meaning that lower scoped fixtures cannot modify the context of the higher level context. I guess I will add another test to confirm that.

However, trying to fix issue #614 is something of a pain with this system of queuing coroutines to background tasks, because suddenly I have to try and run even synchronous fixtures and tests in these background tasks and I have a hard time identifying when that is possible/desirable. I also suspect that reentrancy related to calling getfixturevalue would be difficult. Ideally I would like to run asynchronous fixtures in the "current context" without spinning a context copy (meaning, without spawning a new task), but I do not know if that is even possible.

@agronholm
Copy link
Owner

Hm, I suspected this might not be so easy. I've only taken a fairly cursory look, but I'll delve deeper when I get the chance. Maybe I can think of something then.

@jhominal
Copy link
Contributor Author

So, after thinking about the issue during the night:

  • I am beginning to think that context isolation is too much of a deviation from "regular pytest" to be something that should be introduced as a matter of course in the anyio test runner;
    • Isolation between tests can actually be handled via a decorator on the test function, same as what people would be able to do with a sync or async test;
  • I think I have identified two possible, not too invasive, ways of running all fixtures and tests, including the synchronous ones, in the same context;
    • One which already promises to be extremely hacky if I can make it work at all but would allow me to avoid touching to the synchronous fixtures and tests at all;
    • One which would be slightly more invasive but could probably work by using an explicit, pytest session-scoped contextvars.Context object;

As a result of that, I am going to mark this PR as a draft, and I will submit PRs related to these two options in the coming days.

@jhominal
Copy link
Contributor Author

Given how #618 has turned out, I am going to close this PR - I do not think anymore that this PR should be integrated in anyio.

@jhominal jhominal closed this Sep 29, 2023
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.

2 participants