Run all sync/async fixture/test code under the same contextvars.Context

[jhominal]

So, here is my proposal for fixing #614

It follows the second approach outlined in #616 (comment), namely that it uses a session-scoped contextvars.Context object to run all fixtures and tests, whether asynchronous or synchronous.

It works better than I had hoped for, and it does not even require any modification of the TestRunner backend implementations.

I have created this PR as a draft because I suspect that reentrancy related to the usage of getfixturevalue does not work, but I will need to confirm it with a test case and I already have an idea to make reentrancy work (I actually had reentrancy working but I removed it because it was in the way of debugging all the ways that the tests were failing at the time)

[agronholm]

Why is there a .pyi file here?

[jhominal]

I wanted to write ContextLike.run's signature by using ParamSpec, and I did not think that I could do that in a way compatible with Python 3.9 without using a .pyi file (or taking a dependency on typing_extensions, which I am not keen to add).

Are you opposed to including .pyi files, beyond the incompatibility with mypy's pre-check? I can write a PR for that if that is the only issue.

[graingert]

I wanted to write ContextLike.run's signature by using ParamSpec, and I did not think that I could do that in a way compatible with Python 3.9 without using a .pyi file (or taking a dependency on typing_extensions, which I am not keen to add).

Are you opposed to including .pyi files, beyond the incompatibility with mypy's pre-check? I can write a PR for that if that is the only issue.

You can use

if TYPE_CHECKING:
    from typing_extensions import ParamSpec

    _P = ParamSpec("_P")

[jhominal]

I have added a bit of reentrancy management to the test context runner in order to support usage of getfixturevalue on synchronous fixtures.

Attempting to support usage of getfixturevalue on asynchronous fixtures unearthed the following issues:

• From a synchronous test, there was an error related to the fact that in some cases, the lazy initialization of the TestRunner object would happen on a different context from its disposal;
• From an asynchronous test, the current serialization of all asynchronous code units in a single background task means that we get in a deadlock - maybe this could be solved by using per-fixture tasks instead of running all fixtures in a single task, but I am not sure that it would be enough - as getfixturevalue is, by design, a regular callable, I do not see how the test being executed could yield to the parallel execution of another fixture (maybe the fact that we run the event loop in that callable might do it?);

I think that the implementation is mostly where I want it, though of course feedback and improvement suggestions are welcome.

The only question for me would be to think about what to document about the new context management behavior - here are my ideas:

• Explain the principle of the thing - that is:
  • tests and fixtures, whether sync or async, all run in the same contextvars.Context;
  • however, pytest hooks and plugin code all run in the implicit "main" context;
  • there is no attempt to isolate any test in its own context, whether sync or async - any context variable that is set without being reset afterward will leak to other tests;
• Given that anyio 3.x previously had per-function tasks (and thus, implicit per-function context isolation), but that with this patch all context isolation is removed:
  • we can highlight this change in behavior between AnyIO 3 and AnyIO 4;
  • I think it would be useful to document a sample recipe that would allow developers to mark a test as being run in its own contextvars.Context - I have already been able to write a decorator for that purpose that works with both functions and coroutines, and that I would be happy to share;

[jhominal]

I thought about this PR even more, and two things came to mind:

First, because of the way this PR modifies the way that non-async code runs, it poses a risk of new bugs (with other pytest plugins, with people's conftest.py files, or even built-in pytest features) - how do you want to manage that risk in the next release?

1. Implement "shared context" behavior only;
2. Activate "shared context" behavior by default, with an opt-out configuration switch;
3. Disable "shared context" behavior by default, with an opt-in configuration switch;

Second, it came to mind that, in AnyIO 3 (because of the way that the test running task was managed), it was implicit that every async def test was running in its own context. Given that this PR introduces context management features to anyio's pytest plugin, do you think it would be worth the cost to add a pytest mark to run test functions in "isolated contexts" (that is, a copy of the current context) so that clean-up can be handled automatically?

[jhominal]

@agronholm Have you had time to look at this PR and the associated comments?

[agronholm]

From your previous comments, I thought you had run into a dead end with this effort. The pre-commit failures need to be addressed at the minimum.

[jhominal]

@agronholm: I guess I must be too chatty, I was really waiting on a response from you in this PR, I really do not have any blocker (however, my other contemporaneous PR, #616, was at a dead end, so maybe I could have been clearer in communication).

That is, the question that I need you to answer is the one about how to manage the risk associated to the way that this PR changes the behavior of the anyio pytest plugin:

because of the way this PR modifies the way that non-async code runs, it poses a risk of new bugs (with other pytest plugins, with people's conftest.py files, or even built-in pytest features) - how do you want to manage that risk in the next release?

1. Implement "shared context" behavior only;
2. Activate "shared context" behavior by default, with an opt-out configuration switch;
3. Disable "shared context" behavior by default, with an opt-in configuration switch;

The pre-commit failure is due to having just updated to the last version of the main branch - I will get to it once I have an answer on the above question

[agronholm]

I'd rather not break anyone's test suite in a minor release. Now that I know this is just pending my review, I'll take a deeper look and try to think how I want to mitigate the issue.

[jhominal]

I also want to say, for the record, that if you agree with integrating the principle of this PR in anyio, I inteed to write documentation to explain exactly what is going on here and why.

[agronholm]

I'm currently busy on another project, but I will get to this. I need to take some time to understand it myself. It would be a good idea to write that documentation regardless (changing semantics w/o documentation updates is a no-no).