test_ctx.crashed() should always make the test end up with crash

[touilleMan]

I'm currently tracking a subtle bug were I get an exception in a coroutine spawn inside a fixture.
The test and other fixtures get cancelled by pytest-trio, but the original exception doesn't get re-raised during teardown so test_ctx.error_list end up beeing empty.

The worst part is:

pytest-trio/pytest_trio/plugin.py

Lines 327 to 330 in 5df0bde

	if test_ctx.error_list:
	raise trio.MultiError(test_ctx.error_list)

If test_ctx doesn't contains the exception nothing is done and the test is considered succeeded !

I think we should add a check here and raise an internal error if this occurs... and I should continue tracking the original bug to make this never occur in the first place ;-)

[vxgmichel]

In order to make sure failing tests do not end up silently succeeding, I used the following monkey-patching:

import pytest_trio

def fget(self):
    if self.crashed and not self._error_list:
        return [trio.TrioInternalError("See pytest-trio issue #75")]
    return self._error_list

def fset(self, value):
    self._error_list = value

pytest_trio.plugin.TrioTestContext.error_list = property(fget, fset)

But this doesn't tell us what the original exception was, which is still a huge problem for debugging those tests.

[njsmith]

This is still an open/important issues, and there's a ton more detail in the thread in #77.

The two ideas under consideration are:

• patch pytest-trio to accomplish basically the same thing as @vxgmichel's monkeypatch above. Specifically, if a fixture calls crash(None), but doesn't later follow it up with crash(<an actual exception>), we'd replace that with a new exception, which would at least include the name of the fixture that did this weird thing.

• Right now, the yield in pytest-trio fixtures never raises an exception, just like regular pytest fixtures. But maybe it should act like a checkpoint, and have the possibility of raising a Cancelled exception. This couldn't raise any other kinds of exceptions, and you'd only get this if some code inside this fixture triggered the cancellation by setting up some kind of cancel scope or nursery. (In particular, other fixtures or the main test couldn't trigger a Cancelled in this fixture.)

We might want to do either or both, I'm not sure.

One big open question is how compatible it would be to allow fixture yield to raise Cancelled. I guess we know it would have mostly fixed a bug in Parsec's test suite.

It looks like trio-websocket defines two fixtures. echo_server uses yield server as the last thing in the block, so it's equivalent to just return server and isn't really a yield fixture at all. If we changed the semantics of yield, it wouldn't be affected. The other one is:

@pytest.fixture
async def echo_conn(echo_server):
    async with open_websocket(HOST, echo_server.port, RESOURCE, use_ssl=False) as conn:
        yield conn

So.... it's effectively just a wrapper around an async context manager, which means it definitely won't be hurt by yield raising, and is also more evidence that the pattern of converting context managers into fixtures is common.

@belm0 you have a large code base and I guess lots of tests – if you have a moment to check it'd be nice to know if you have any pytest fixtures that would be affected by this change.

[belm0]

not sure I follow-- does this PR already implement the new semantic. Otherwise how do I check?

[njsmith]

@belm0 sorry, there's no PR yet. The new semantic would be if you define your own pytest fixture:

@pytext.fixture
async def my_fancy_fixture():
    async with trio.open_nursery() as nursery:
        nursery.start_soon(...)
        yield fixture_value
        print("whooo look at my custom fixture")

...then right now, that yield can never raise an exception, and the print is guaranteed to happen. In the possible future world, it will be possible for that yield to raise Cancelled in case the nursery's background task crashes, so the print might not happen.

So the question is whether you happen to have any custom fixtures that use yield, and if so, whether they depend on the code after the yield running even if some kind of background task inside the fixture crashed.

[belm0]

Thanks for clarifying. None of our fixtures have statements after yield, and they already use try/finally.

[njsmith]

OK, #83 implements both of the proposed changes.