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

Refactor everything and use the same logic as pytest-anyio #147

Open
jakkdl opened this issue Sep 17, 2024 · 5 comments
Open

Refactor everything and use the same logic as pytest-anyio #147

jakkdl opened this issue Sep 17, 2024 · 5 comments

Comments

@jakkdl
Copy link
Member

jakkdl commented Sep 17, 2024

https://anyio.readthedocs.io/en/stable/testing.html has a much cleaner implementation at a fraction of the code size: https://github.com/agronholm/anyio/blob/master/src/anyio/pytest_plugin.py

Discussions in Gitter also suggests people prefer running pytest-anyio in trio mode over directly running pytest-trio.

This would also resolve #137, #124, probably #123, #89, #57, and surely a whole host of other issues.

EDIT: it would also resolve the problem of yield raising trio.Cancelled and making teardown not get executed unless it's inside a finally: https://pytest-trio.readthedocs.io/en/stable/reference.html#an-important-note-about-yield-fixtures

@jakkdl
Copy link
Member Author

jakkdl commented Oct 2, 2024

I'm making solid progress on this, but pytest-anyio does not support clocks/instruments (agronholm/anyio#260), so will have to add that functionality

@jakkdl
Copy link
Member Author

jakkdl commented Oct 3, 2024

I'm making solid progress on this, but pytest-anyio does not support clocks/instruments (agronholm/anyio#260), so will have to add that functionality

okay this was pretty trivial.

  1. But Parallel fixture setup/teardown violates contextvar stack discipline #137 needs to be "resolved" (which means removing functionality and their corresponding tests) as the pytest-anyio test runner does not support it, and adding it would be messy (and perhaps bad).
  2. pytest-anyio currently does not run sync fixtures in the same context https://anyio.readthedocs.io/en/stable/testing.html#context-variable-propagation and there may be issues when doing that + differently scoped fixtures. I'll probably blaze ahead and run all fixtures, async or not, in the same context and see if issues do pop up later on - any current test suites using pytest-trio will only be using function-scoped fixtures anyway.

@bradleyharden
Copy link

@jakkdl, I took a brief look, and it seems that anyio uses a trio guest loop. I'm sure that's a perfectly reasonable architecture if you're running pytest from the command line, but you might also consider the use case of running pytest.main with trio.to_thread.run_sync from within an existing trio runtime. That is my use case, and the workaround I came up with in #89 here handles both approaches (although the code as-posted has at least one bug).

I looked into what it would take to convert the existing pytest-trio plugin to use my approach, but I also ran into the clock problem. I don't immediately see an easy solution, but perhaps it could be solved by implementing my threading approach as a session-scoped fixutre that can take a clock as a dependency, or something like that.

In any event, those are problems I don't need to solve for my use case, so I think I will end my investigation here. I'll continue with my own workaround unless/until this plugin is updated. Good luck with your proposed refactor.

@jakkdl
Copy link
Member Author

jakkdl commented Oct 20, 2024

yeah the biggest hurdle to any redesign is keeping feature parity with current pytest-trio (other than removal of concurrent fixture execution). But your approach looks very interesting, and I think it would be great if it was available Somewhere ™️ for others with the same requirement. If you wanna host a repo for it we could at least link it from https://trio.readthedocs.io/en/stable/awesome-trio-libraries.html

@jakkdl
Copy link
Member Author

jakkdl commented Nov 13, 2024

progress on WIP PR is currently blocked by agronholm/anyio#805

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

No branches or pull requests

2 participants