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

Figure out how to stop fighting with pytest-asyncio over fixtures #10

Closed
njsmith opened this issue Dec 9, 2017 · 3 comments · Fixed by #50
Closed

Figure out how to stop fighting with pytest-asyncio over fixtures #10

njsmith opened this issue Dec 9, 2017 · 3 comments · Fixed by #50

Comments

@njsmith
Copy link
Member

njsmith commented Dec 9, 2017

As noted in python-trio/trio#27 (comment):

speaking of pytest-asyncio module, it considers any async fixture to be run with asyncio. So if it pytest pytest_fixture_setup is run before pytest-trio one (not sure how pytest decide which module execute first, but that's what happened during my tests), this cause a crash. So right now pytest-asyncio is not compatible with this pytest-trio

Here's my suggestion: since fixtures already have to be explicitly decorated, it's not too onerous to ask the user to also explicitly say which kind of async fixture they want. So instead of writing

from pytest import fixture

@fixture
async def whatever():
    ...

you'd write

from pytest_trio import trio_fixture

@trio_fixture
async def whatever():
   ...

We should probably also continue to install a fixture hook, but make it just detect when someone has accidentally written @fixture async def ... so we can give an error message saying to use @trio_fixture instead.

We should also try to convince the pytest-asyncio folks to switch to this system (CC @Tinche). Especially if we are going to install a fixture hook that will randomly cause their thing to break and tell their users to use @trio_fixture instead :-)

@njsmith
Copy link
Member Author

njsmith commented Dec 9, 2017

(accidentally posted this too soon; if you got an empty notification then click through to the issue to see my actual text.)

@njsmith
Copy link
Member Author

njsmith commented Dec 9, 2017

Oh heh, looking further into python-trio/trio#27 (comment) I see that you already proposed something similar. I guess my innovation above is that if people are using a @trio_fixture decorator anyway then we don't need to make them use @pytest.fixture too, we can make the @trio_fixture decorator imply that.

Small tweak we could do: make our special decorator accept non-async functions and silently treat them like regular @pytest.fixture, so you can do from pytest_trio import fixture at the top of your file and then use @fixture everywhere, and it magically works. I'm not sure whether this would be really handy, or way too magical.

@njsmith
Copy link
Member Author

njsmith commented Feb 9, 2018

Coming back around to this again, I'd like to make a more refined proposal:

  • A fixture is a "trio fixture" if any of the following are true:

  • It's an error to use a "trio fixture" in any situation except (a) test scope, (b) the test is marked @pytest.mark.trio

  • All "trio fixtures" get the special delayed handling when combined with the @pytest.mark.trio runner

I think this provides a good balance of functionality for both values of the "everything is trio" config flag, should provide useful error messages when misused, and has at least the potential to play nicely with packages like pytest-asyncio and pytest-curio.

Of course, right now, any system that allows for plain @pytest.fixture to work on regular async functions will be broken if pytest-asyncio is installed, because that's how it works, and that includes this proposal for when "everything async is trio" mode is enabled. But there would be path forward at least, since in this proposal that brokenness would be entirely on pytest-asyncio's side, and we could hopefully convince them to implement a similar setup. (@Tinche: sound plausible?)

njsmith added a commit to njsmith/pytest-trio that referenced this issue Jul 24, 2018
- Add @pytest_trio.trio_fixture for explicitly marking a fixture as
  being a trio fixture
- Make the nursery fixture a @trio_fixture
- Refactor Trio fixture classes into one class
- Check for trio marker instead of trio keyword (fixes python-triogh-43)
  - This also raises the minimum pytest version to 3.6
- Raise an error if a Trio fixture is used with a non-function
  scope (fixes python-triogh-18)
- Raise an error if a Trio fixture is used with a non-Trio test

I think this also closes python-triogh-10's discussion, though we still need to
convince pytest-asyncio to fix their side of things.
@njsmith njsmith mentioned this issue Jul 24, 2018
njsmith added a commit to njsmith/pytest-trio that referenced this issue Jul 25, 2018
- Add @pytest_trio.trio_fixture for explicitly marking a fixture as
  being a trio fixture
- Make the nursery fixture a @trio_fixture
- Refactor Trio fixture classes into one class
- Check for trio marker instead of trio keyword (fixes python-triogh-43)
  - This also raises the minimum pytest version to 3.6
- Raise an error if a Trio fixture is used with a non-function
  scope (fixes python-triogh-18)
- Raise an error if a Trio fixture is used with a non-Trio test

I think this also closes python-triogh-10's discussion, though we still need to
convince pytest-asyncio to fix their side of things.
njsmith added a commit to njsmith/pytest-trio that referenced this issue Jul 25, 2018
- Add @pytest_trio.trio_fixture for explicitly marking a fixture as
  being a trio fixture
- Make the nursery fixture a @trio_fixture
- Refactor Trio fixture classes into one class
- Check for trio marker instead of trio keyword (fixes python-triogh-43)
  - This also raises the minimum pytest version to 3.6
- Raise an error if a Trio fixture is used with a non-function
  scope (fixes python-triogh-18)
- Raise an error if a Trio fixture is used with a non-Trio test

I think this also closes python-triogh-10's discussion, though we still need to
convince pytest-asyncio to fix their side of things.
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 a pull request may close this issue.

1 participant