Rework fixtures #50
Rework fixtures #50
Conversation
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 96.75% 99.34% +2.59%
==========================================
Files 13 19 +6
Lines 308 457 +149
Branches 30 44 +14
==========================================
+ Hits 298 454 +156
+ Misses 6 2 -4
+ Partials 4 1 -3
Continue to review full report at Codecov.
|
|
Also renamed the |
njsmith
force-pushed
the
fixture-rework
branch
3 times, most recently
from
9b5c3e5
to
243fdf3
Compare
- 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.
Every nursery is called 'nursery'. This is less generic, and emphasizes that this nursery is special, because its lifetime is linked to the test. Kept 'nursery' around for now as a deprecated alias.
The main benefit of this is that it lets us catch more cases where these fixtures are accidentally misused. Closes python-triogh-18 Closes python-triogh-51
|
Just pushed another commit that refines the fixture handling logic a bit more: now async fixtures are treated as trio fixtures if (they're used by a trio test OR trio mode is enabled). I think this is the best of both worlds (see also #47 (comment)). |
See python-trio#55 for discussion. Main points: * Fixture setup/teardown is now performed concurrently whenever possible. * Each fixture is now run in an isolated task, and it's now safe to use nurseries inside fixtures (fixes python-triogh-55). * New magical fixture 'fixture_nursery' which gives a nursery that's cancelled immediately after *the requesting fixture finishes teardown*. If requested directly by the test itself, gives a nursery which is cancelled after the test finishes, before other fixtures are torn down.
|
I just rewrote the fixture/test code yet again, based on the discussion in #55. I'm really happy with how it came out. Main points:
|
|
Well that's bizarre. All of the Edit: 3.6.4 works. With 3.7.0... everything fails, I guess it's just a bad release. Some something that changed in 3.7.0 or 3.7.1. |
|
Ah-hah, it's this bug, which is already fixed in pytest master but hasn't been released yet: pytest-dev/pytest#3774 (Specifically the issue is that pytest is now putting a wrapper on fixture functions and then trying to unwrap it again... but it gets too eager with the "unwrapping", so if you combine This doesn't really have anything to do with this PR but I'll pin the pytest version here anyway so it doesn't block things. |
At this point this: @pytest.fixture async fix(nursery): ... is just a shorthand for this (they should be exactly equivalent): @pytest.fixture async fix(): async with trio.open_nursery() as nursery: try: ... finally: nursery.cancel_scope.cancel() And the latter is the obvious thing you would write by hand, there's really no other way to do it. And since it no longer has special subtle semantics, I'm no longer bothered by using the short name 'nursery'. Plus the longer I looked at 'fixture_nursery' the more annoying the name seemed. Plus I made that last minute change so you can use the magic nursery inside tests too, not just fixtures, so the name didn't even make much sense.
|
I want to add some more tests, but otherwise this is ready to review. Not sure the best way to approach this, since it's grown to become a major rewrite of pytest-trio's core... @touilleMan, do you think you'll be up to looking soon? Anyone else? I'd like to merge and release this soon, because it'll make a huge difference in usability, and also let us merge #47 to get proper docs. (It would even be helpful if anyone who's using pytest-trio could try out this branch and make sure that their test suite still works.) |
|
One small question I've been pondering: when we get a background crash, maybe it would make more sense to cancel all users, instead of just the test itself? |
|
@njsmith This seems like an incredible work, thanks a lot for this PR ! |
|
Added more tests. I'm sure we could add even more, but the tests do push at some of the nastier edge cases now (including background tasks crashing at different moments), and the coverage report is solid. Also, I added some code to use friendly names for all the fixture tasks, which could hopefully help with this issue (#47 (comment)):
In the course of this (and also while thinking about python-trio/trio#607), I have started to think that maybe the details of our new cancellation rules aren't quite right: I'm leaning towards saying that when a fixture crashes, we should cancel all "downstream" fixtures, but not do anything at all to independent fixtures. Right now, we only cancel the test, not other downstream fixtures... except that we also cancel all fixtures that haven't started yet, including independent ones. This means that if you have two independent fixtures that both crash during setup, you'll actually get different exceptions on different runs, randomly, depending on which one happens to run first. But these are some pretty obscure details that aren't going to really matter to 99.9% of users, and this PR is already out of control, so I'd rather merge this as is and then open an issue to keep track of that as a possible followup change. |
Also wrote some draft release notes
Also wrote some draft release notes
|
@touilleMan Did you get a chance to take a look this weekend? |
|
@njsmith Sorry for the delay, I've spent my weekend repairing my old car so not much time for computing :'-( Anyway, I've played with this PR and it seems pretty cool (work as a drop in replacement for my test suite). However I've found the fixtures doesn't play nice with trio_asyncio: import pytest
import trio_asyncio
import asyncio
@trio_asyncio.trio2aio
async def work_in_asyncio():
await asyncio.sleep(0)
@pytest.fixture()
async def asyncio_loop():
async with trio_asyncio.open_loop() as loop:
yield loop
@pytest.fixture()
async def asyncio_fiture_with_fixtured_loop(asyncio_loop):
await work_in_asyncio()
yield 42
@pytest.fixture()
async def asyncio_fixture_own_loop():
async with trio_asyncio.open_loop():
await work_in_asyncio()
yield 42
@pytest.mark.trio
async def test_no_fixture():
async with trio_asyncio.open_loop():
await work_in_asyncio()
@pytest.mark.trio
async def test_half_fixtured_asyncpg_conn(
asyncio_fixture_own_loop
):
pass
@pytest.mark.trio
async def test_fixtured_asyncpg_conn(
asyncio_fiture_with_fixtured_loop
):
passThis snippet works fine with current master of And with this PR: I even have the feeling this snippet could be added in |
|
Ughhhh of course it doesn't. Ughhhh. So... our two options are to either find some way to move back towards normality, so trio-asyncio automatically works because we're actually doing what it expects, or to double down on our elaborate house of mirrors. In general my instinct is to do the former, but in this case that would require going back to nesting fixtures inside each other, and I just can't see how to make that work. So... plan B. Here's my evil idea: what if we make it so that all the tasks we're creating for fixtures, actually share a contextvar context, so changes one fixture makes are visible to the others. Then when trio-asyncio registers stashes the loop in a contextvar, the subsequent fixtures and test will be able to find it. The main thing that gives me pause here is that if we literally use the same I guess we could literally do a three way merge. Or really go wild and do mark-merge. This is hilarious overkill and ridiculous but now that I've said it I'm kind of tempted. I guess it's not even the fully general case, since we would error out on conflicts (trying to depend on two fixtures that set the same contextvar to different values), and this means there would never be resolved conflicts in the history. |
|
We could also stop running fixtures in parallel. If we run them sequentially then there's no problem. I'm kind of dubious about whether running them in parallel really helps anything, actually – I mostly did it because once we were isolating fixtures into independent tasks, it seemed easier than doing a toposort to get a sequential order. Though now that I think about it, there's a great trick for picking the sequential order: just record what order pytest tries to set up the fixtures, and duplicate it. And this would reduce the chance of race conditions in fixtures, which sound like a real nightmare to deal with. |
This is sort of a gross hack, but it's simple and works. If it causes problems, then this comment discusses some other options: python-trio#50 (comment) Also added tests for using trio-asyncio in fixtures (which was the motivating use case for sharing contextvars).
|
Okay, I went with the simplest-possible-thing and enabled sharing of contextvars context across all the fixtures in a test. We may want to clean this up later, but it solves the immediate problem. I also added your trio-asyncio test to the test suite. It passes now. (At least on my laptop, we'll see what CI says.) |
|
Sorry for the python3.6 only snippet that broke the CI ^^ |
|
Heh, no worries :-) |
When importing trio-asyncio v0.8.0 on py35, we get a crash at import
time:
.../python3.5/site-packages/trio_asyncio/loop.py:171: in <module>
_orig_run_get = _aio_event._get_running_loop
E AttributeError: module 'asyncio.events' has no attribute '_get_running_loop'
See: https://travis-ci.org/python-trio/pytest-trio/jobs/419092404
So let's skip testing this for now...
|
Filed python-trio/trio-asyncio#33 for the trio-asyncio errors on py35. So assuming CI passes now, I think this is OK to merge, again :-). With follow-up issues to file:
|
|
I've retried your last version, it works fine and solve as expected issues I had with test never stopping when an exception occurs in the asyncio world (bad SQL syntax given to asyncpg in my case). |
Same thing here ! I would say sequential order is good enough and avoid painful concurrency debugging when multiple fixtures are stepping on each other toes ;-) But anyway this can be fixed in another PR as you said |
|
my project is using this PR so I'm in favor of a merge 👍
|
|
Okay, then let's do this :-) @touilleMan I believe #47 is up to date and ready to merge as soon as this goes in, do you (or anyone else) want to take a look? |
being a trio fixture
scope (fixes Verify scope for async fixtures #18)
I think this also closes gh-10's discussion, though we still need to
convince pytest-asyncio to fix their side of things.