TrioTestCase: Match other people's APIs for context stacks

[geofft]

One of the changes in #7 is that TrioTestCase now has a stack member of type contextlib.AsyncExitStack, which you can use via await self.stack.enter_async_context(...) or self.stack.push_async_callback(...).

As it happens, I think we're only internally using the latter.

I think it'd be a good idea for TrioTestCase to look like other test classes. Upstream unittest.TestCase and more interestingly unittest.IsolatedAsyncioTestCase solve this problem in a different way: they have self.addCleanup(...) and await self.addAsyncCleanup(...) methods.

So, in theory, we could adopt that approach pretty straightforwardly.

However, upstream unittest is considering adding support for ExitStack/AsyncExitStack - see bpo-45046 and python/cpython#28045. The API is slightly different from ours - instead of exposing an actual stack member or even having such a private member, they just implement enterContext etc. and enterAsyncContext themselves, by (async) calling the enter function and adding an (async) cleanup for the exit function.

I have a vague feeling that context managers are a better approach than addAsyncCleanup/push_async_callback, but I'm not really sure.

For what it's worth, pytest-trio just has async fixtures which can cleanup post-yield, and are somewhat geared towards using context managers with a literal with statement. Their tutorial has this example:

@pytest.fixture
async def echo_client(nursery):
    listeners = await nursery.start(
        partial(trio.serve_tcp, echo_server_handler, port=0)
    )
    echo_client = await open_stream_to_socket_listener(listeners[0])
    async with echo_client:
        yield echo_client

I don't think there's any other way in pytest-trio to do cleanup.

(It's maybe also worth noting that pytest-trio's nursery fixture cancels the nursery's cancel scope when the test finishes, which is at least a little bit of what we're doing in our cleanups, but not all of it. So there may also be an argument for TrioTestCase to automatically provide a nursery, maybe....)

[catern]

I think it'd be a good idea for TrioTestCase to look like other test classes. Upstream unittest.TestCase and more interestingly unittest.IsolatedAsyncioTestCase solve this problem in a different way: they have self.addCleanup(...) and await self.addAsyncCleanup(...) methods.

Yes, that seems reasonable. Although I think we'd want both spellings (push_async_callback and addAsyncCleanup) so that we're compatible with both AsyncExitStack and TestCase. Possibly we should even inherit from AsyncExitStack, so that the typing matches up without needing to use an interface...

Although I still don't remember the whole philosophy I was going for with passing around an AsyncExitStack, other than that I think Nursery should support AsyncExitStack methods... because, if you want to run some code when you get cancelled, you can do that currently today, but it's a hassle that requires a fair bit of boilerplate.

Possibly a better API than this ExitStack business would be to just have a helper function which takes a Nursery and a callback and starts up a task to run that callback when the Nursery is cancelled? I don't remember if that would work with the concrete use case. Maybe I left some commit messages behind when I added the AsyncExitStack, you might check.

However, upstream unittest is considering adding support for ExitStack/AsyncExitStack - see bpo-45046 and https://github.com/python/cpython/pull/28045. The API is slightly different from ours - instead of exposing an actual stack member or even having such a private member, they just implement enterContext etc. and enterAsyncContext themselves, by (async) calling the enter function and adding an (async) cleanup for the exit function.

That would be reasonable to support too, sure.

So there may also be an argument for TrioTestCase to automatically provide a nursery, maybe....)

Don't we already automatically provide a nursery?

[serhiy-storchaka]

The reason of doing so in unittest.TestCase is that we now have separate reports for errors in every cleanup function instead of a single traceback with chained exceptions. Note that it was designed before implementing exceptions chaining.