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

synchronous fixture setup and teardown #138

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

agnesnatasya
Copy link

Mainly fixes #137, by making fixtures setup and teardown synchronous
Related issues and discussions:

Implementation details

  • I created a topological sort of all fixture dependencies for each test, and set them up synchronously, and then tear them down synchronously in a reverse order, by waiting for the 'previous' or 'next' fixture in the topology
    • njsmith mentioned here that we can record the order of fixture construction from pytest - but I'm not sure if this is necessary? I was thinking we can create our own order, as long as it respects the dependency order, but I might be missing something, so let me know if this ordering is not enough to cover all cases!
  • I added a test that tests for the contextvar not intertwining with each other
    • I also verified that this fails flakily without this diff.

Concerns

  • This change might impact user experience, especially users that sets up i/o heavy fixtures.

This is my first PR, want to double check if I've done the complete checklist

  • I installed pre-commit with config file in the trio repo since there doesn't seem to be one in this repo.
    • I ended up getting warning for asend from codespell, so I just disabled codespell and committed without the codespell check
  • I ran pytest with PYTHONPATH="pytest-trio" pytest pytest-trio and I received 2 xfailed, and 1 warning.
    • I think the 2 xfailed are expected, since these 2 are indeed marked as xfail
    • I can also see the warning when running pytest without setting the PYTHONPATH

Let me know if I'm missing anything, would appreciate any feedback :D

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.

Parallel fixture setup/teardown violates contextvar stack discipline
1 participant