-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
test_ctx.crashed() should always make the test end up with crash #75
Comments
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. |
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:
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 It looks like trio-websocket defines two fixtures. @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 @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. |
not sure I follow-- does this PR already implement the new semantic. Otherwise how do I check? |
@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 So the question is whether you happen to have any custom fixtures that use |
Thanks for clarifying. None of our fixtures have statements after yield, and they already use try/finally. |
OK, #83 implements both of the proposed changes. |
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 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 ;-)
The text was updated successfully, but these errors were encountered: