nursery fixture is too error-prone

[belm0]

Even though I'm well aware of the nursery fixture's behavior of cancelling the nursery when the test function reaches end of scope, too many times I've written invalid tests that silently end without completing the test code. This is the worst kind of trap for test authors.

• example (my error): groove-x/trio-util@44ea836
• example (PR contributor's error): Catch ClosedResourceError. Fixes #134 trio-websocket#140 (comment)

In hindsight, it would have been better for the nursery fixture to not automatically cancel. The test writer would have to cancel explicitly, and the utility of the fixture is simply to avoid boilerplate and extra nesting. The big advantage is that the failure mode when the test author does the wrong thing is obvious: the test will hang.

At this point, changing the nursery semantics would break the world and cause confusion. So perhaps the best we can do is add a fixture under another name, like simple_nursery.

[gc-ss]

too many times I've written invalid tests that silently end without completing the test code

Some updates were made recently that now pass the cancel scope properly. This will allow your fixtures or dependent functions to detect and handle the cancel scope.

FYI: #120 (comment)

Can you revisit? Until it's merged in, it's: #121

[njsmith]

I think the way forward here is probably to:

• add some kind of "helper task" support to trio: Idea: service nurseries trio#1521
• deprecate the nursery fixture, since it's not needed if trio makes it easy to start this kind of auto-cancelling service on its own

[touilleMan]

I strongly agree about removing the nursery fixture altogether, see:

https://github.com/Scille/parsec-cloud/blob/92bd51bb9a69d9e3da2e10c9a3eb44e30326cab8/tests/conftest.py#L495-L510

Asynchronous yield fixture is much better (and allow user to implement it own nursery fixture if he really want to shoot hemself in the foot )

[gc-ss]

Asynchronous yield fixture is much better (and allow user to implement it own nursery fixture if he really want to shoot hemself in the foot )

I think people would appreciate a short example that shows why/when the nursery fixture is a bad idea and how to fix/address it (possibly using asynchronous yield fixtures)

If this requires a lot of code, can you explain some issues the current issues the nursery fixture can lead to?

I might be able to cook up an example but I am atleast a month away and would like to see feedback from others as well so I don't feel alone ! 🎉

[belm0]

I think the way forward here is probably to:

• add some kind of "helper task" support to trio: python-trio/trio#1521
• deprecate the nursery fixture, since it's not needed if trio makes it easy to start this kind of auto-cancelling service on its own

I'm not sure this addresses the point. It may not have been the intended use, but I suspect people mostly use the nursery fixture out of convenience: they don't want to write the open_nursery() block and have the extra level of indentation in their test body.

I think people would appreciate a short example that shows why/when the nursery fixture is a bad idea and how to fix/address it

async test_foo(nursery):
    connection = bar()

    @nursery.start_soon
    async _listener():
        assert await connection.receive() == 'baz'

    await connection.send('not-baz')

    # Whoops-- exiting test body will cancel the `_listener` task before it
    # evaluates its `assert`.  Most pytest-trio users have no idea that `nursery`
    # fixture behaves this way, and their test will incorrectly pass.

corrected usage 1: explicitly open_nursery() instead of using nursery fixture

corrected usage 2: use the fixture, but wait for important tasks to complete before exiting the test

async test_foo(nursery):
    done = trio.Event()
    connection = bar()

    @nursery.start_soon
    async _listener():
        assert await connection.receive() == 'baz'
        done.set()

    await connection.send('not-baz')
    await done.wait()

Per my original report:

In hindsight, it would have been better for the nursery fixture to not automatically cancel. The test writer would have to cancel explicitly, and the utility of the fixture is simply to avoid boilerplate and extra nesting. The big advantage is that the failure mode when the test author does the wrong thing is obvious: the test will hang.

At this point, changing the nursery semantics would break the world and cause confusion. So perhaps the best we can do is add a fixture under another name, like simple_nursery.

async test_foo(simple_nursery):
    connection = bar()

    @simple_nursery.start_soon
    async _listener():
        assert await connection.receive() == 'baz'

    await connection.send('not-baz')

    # OK:  test will block on completion of `_listener` task

[gc-ss]

So this simple_nursery would work by essentially monitoring these trio.Event?.

I mean, we could setup a trio.Event at the start of the @simple_nursery.start_soon and set it at the end. Then the simple_nursery blocks on .wait() but this is just me thinking out loud:

Why won't I make _listener an async fixture that takes care of it's own lifecycle?

The trio.Event monitoring will still be explicit but it will look like this:

@pytest.fixture
async done():
    return trio.Event()

@pytest.fixture
async _listener(done)
    # something with await connection.receive() …
    done.set()

async test_foo(nursery, _listener, done):
    connection = bar()

    await connection.send('not-baz')
    await done.wait()

    assert _listener.value  == 'baz'

[belm0]

The goal is not to refactor my simplistic test example-- rather to avoid surprises commonly encountered by pytest-trio users. (E.g. let's not assume the user is comfortable with writing fixtures.)

I don't think simple_nursery needs an explicit done event, since by definition a nursery will block on exit until its children tasks have exited.

@pytest.fixture
async simple_nursery()
    async with trio.open_nursery() as nursery:
        yield nursery