Ensure async fixture setup and teardown run in the same task

[provinzkraut]

An attempt to fix #1191.

Inspired by anyio's pytest plugin: https://github.com/agronholm/anyio/blob/b8e91a5cfe35c3b28f91ea8251674a871a7f4e26/src/anyio/_backends/_asyncio.py#L2222-L2239.

Unfortunately, this requires some hacky hacks abusing backports.asyncio.runner to ensure task context is properly propagated on Python < 3.11, in order not to break things.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.28%. Comparing base (3ab1fea) to head (7581198).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
pytest_asyncio/plugin.py	93.18%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1193      +/-   ##
==========================================
+ Coverage   88.91%   89.28%   +0.36%     
==========================================
  Files           2        2              
  Lines         406      448      +42     
  Branches       44       48       +4     
==========================================
+ Hits          361      400      +39     
- Misses         35       37       +2     
- Partials       10       11       +1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[seifertm]

Thanks for sharing this! In general, I think we can try something like this.

It expect this to be a very intrusive change that has the potential to break a lot. Therefore, the queue-based task runner needs to be behind a feature toggle, such as a configuration value and/or a command line option.

I'm not sure what the best approach is codewise. My very early guess it that it would help to create some kind of abstraction on top of asyncio.Runner, and move some existing log, such as handling of contextvars, there. In the next step, we can create our new queue-based implementation and eventually we can use the feature toggle to switch between them. This is just an idea, though. I have no clue how it plays out.

[provinzkraut]

I'm not sure what the best approach is codewise. My very early guess it that it would help to create some kind of abstraction on top of asyncio.Runner, and move some existing log, such as handling of contextvars, there. In the next step, we can create our new queue-based implementation and eventually we can use the feature toggle to switch between them. This is just an idea, though. I have no clue how it plays out.

Would you be okay with taking an iterative approach to this?

A first iteration could be basically what I've got here, with a feature toggle added.

My issue at the moment is, that this feature not existing means I, as a library author, cannot use anyio.CancelScope within code that users of my library might use in a pytest fixture. Especially since the code in question is a testing utility, and using it within a fixture is basically the intended use case, it means that I would break the test suites of all my users who also use pytest-asyncio.

I'm definitely willing to put in the work on this side of the fence, if we could come up with some discrete steps, ideally one that includes an MVP :)

[seifertm]

The test file you found uses the "old" way of writing tests directly in a module. In the past, these kinds of tests caused side effects (e.g. failing to clean up resources resulting in a ResourceWarning) that only appeared much later in the test suite. Therefore, we moved to using Pytester for all pytest-asyncio tests.

I'd appreciate, if you rewrote the test to use Pytester, even though it's mostly boilerplate. You can check the the file test_function_scope.py for an example.

Although it's not strictly necessary, I suggest to use strict mode in those tests (or both, if there's different behavior depending on the mode).

[seifertm]

Would you be okay with taking an iterative approach to this?

A first iteration could be basically what I've got here, with a feature toggle added.

That makes sense. Code that lies around in a branch doesn't help anyone. The change you're proposing would not just benefit Litestar, but also other users. If we manage to put it behind a feature flag and all existing tests are green, it should be fine.

The feature flag documentation needs to communicate clearly, that this is an experimental feature that may bring breaking changes without notice. That is, until we stabilize it.
Ideally, we will eventually be able switch to the queue-based approach, but I don't fully understand the consequences, yet.

[provinzkraut]

Would you be okay with taking an iterative approach to this?
A first iteration could be basically what I've got here, with a feature toggle added.

That makes sense. Code that lies around in a branch doesn't help anyone. The change you're proposing would not just benefit Litestar, but also other users. If we manage to put it behind a feature flag and all existing tests are green, it should be fine.

The feature flag documentation needs to communicate clearly, that this is an experimental feature that may bring breaking changes without notice. That is, until we stabilize it. Ideally, we will eventually be able switch to the queue-based approach, but I don't fully understand the consequences, yet.

Sounds like a good approach! I probably won't have much time in the next 2 weeks, but I'm planning to pick this back up after!