Upgrade `trio.testing.RaisesGroup` with a `must_reach_point` argument to test context-manager exits

[Zac-HD]

At work we have an internal AssertRaises class which I'd like to kill it off in favor of the newer trio.testing.RaisesGroup (added in https://github.com/python-trio/trio/pull/2898). However, it has one neat feature which we use fairly regularly to test exceptions arising specifically when exiting some (potentially async) context manager, often one which wraps a nursery:

with RaisesGroup(..., must_reach_point=True) as assert_raises:
    async with some_async_context_manager:
        ...
        assert_raises.must_reach_this_point()
        # passes iff we raise the error on exit

Obviously it's an error to call .must_reach_this_point() without specifying must_reach_point=True or vice-versa, or to call it twice. If must_reach_point=True then the RaisesGroup should not catch or otherwise handle any exceptions raised before .must_reach_this_point() is called.

You can currently implement something similar inline, with:

reached_point = False
with RaisesGroup(...) as excinfo:
    async with some_async_context_manager:
        ...
        reached_point = True
if not reached_point:
    raise AssertionError("Raised expected exception, but never executed CM body") from excinfo.value

but a mere three lines of code each time adds up pretty quickly. I think the main effect of this is to change from checking the exception was raised at all to checking it was raised during exit, which seems pretty valuable to me.

A small downside is it may encourage too-widely-scoped with RaisesGroup; in principle we could check at runtime that the .must_reach_this_point() is inside a with block which is itself inside the RaisesGroup , but I don't think it's worth the trouble.

[jakkdl]

I'm not a fan of this parameter, in large part because the end goal is to move RaisesGroup into upstream pytest, and it's hard for me to make a strong case for adding it there. It also brings up a bunch of follow-up questions: why should RaisesGroup have this functionality, but not pytest.raises? Should it be possible to unset .must_reach_this_point? Can one require setting it multiple times? Or what about multiple different .must_reach_this_points?

I could even see pytest newbies creating really broad with RaisesGroup blocks, or even going as far as manually raising ExceptionGroups just to use the must-reach-this-point functionality.

I think making sure a specific line of code runs is a super common problem that comes up in tons of other situations, not just when we expect ExceptionGroups to be raised. So adding custom logic for this one specific case needs a strong reason imo - which I'm not sure this scenario when there are fairly painless ways of achieving it in other ways.

Another option for rewriting it would be to unroll the async with statement:

my_cm = some_async_context_manager()
await my_cm.__aenter__()
...
with RaisesGroup(...):
    await my_cm.__aexit__()

(which would make PT012 very happy), making it very explicit that you're only testing the exit. though ofc it is somewhat clunky.

Yet another option, keep your AssertRaises - but make it a subclass of RaisesGroup, delegating all the exception logic to it and only adding on your .must_reach_this_point(). Something like

class AssertRaises(trio.testing.RaisesGroup):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.has_reached=False

    def must_reach_this_point(self):
        self.has_reached=True

    def __exit__(self, *args):
        assert self.has_reached
        super().__exit__(*args)

[Zac-HD]

Fair enough, the subclass trick does seem like a better solution than upstreaming everything.

[A5rocks]

Alternatively, I think it would be possible to just have another context manager (not a subclass) and use that! You could probably even then use a pytest fixture and allow it for non-exception group tests. To be specific, something like this (not sure if this works, just copying from the subclass above + edits):

class GetsTo():
    def __init__(self):
        self.has_reached=False

    def must_reach_this_point(self):
        self.has_reached=True

    # some definition of __enter__, I think this is the right definition?:
    def __enter__(self, *args):
        return None

    def __exit__(self, *args):
        assert self.has_reached

[jakkdl]

Hm, I considered that one as well but thought it wouldn't be able to perfectly replicate the behaviour with printing the stacktrace if the point wasn't reached. But thinking about it again now I don't see why it wouldn't work. Just gotta make sure to place the GetsTo() CM inside the RaisesGroup(), and to always return False (unless the assert fails) in __exit__ so as not to suppress the expected exception[group].