Initial implementation of RaisesGroup

[jakkdl]

This is an alternative implementation to #11656, to fix #11538

Instead of doing

pytest.raises(ExpectedExceptionGroup(ValueError)):
  ...

this condenses it to

pytest.RaisesGroup(ValueError):
  ...

which also means it doesn't do any modifications to pytest.raises or RaisesContext

[nicoddemus]

Hey @jakkdl thanks for this PR!

I like the idea of going with pytest.raises_group, for the reasons I stated in #11538 (comment):

• We might decide to go with a simpler implementation initially, for instance only matching a single exception from the group, without worrying about hierarchy, and decide how to expand on that later.
• We can postpone to decide on more complicated aspects such as how to also match the exceptions against their messages.

Trying to shoehorn exception group unpacking into pytest.raises seems risky, as we have several concerns to solve (how match should be handled?), plus any future extension to pytest.raises will have to take exception groups into account, and for some extensions it might not even make sense to also support exception groups.

I would vote to go with just pytest.raises_group matching one exception, and postpone hierarchy and message matching for later: I think it would be nice to get this into 8.0, so we should keep it simple and not rush into major commitments -- introducing a simple pytest.raises_group seems low risk and opens up expansion later -- in fact we can see how https://github.com/python-trio/trio/releases/tag/v0.24.0 and python-trio/trio#2898 will fare in the wild and learn from it.

What do you think @bluetech @RonnyPfannschmidt ?

[jakkdl]

(I think you meant python-trio/trio#2898 and not python-trio/trio#2891)
that seems like a reasonable start, I can pare this down to a minimal implementation. We could even refer people to the RaisesGroups in trio in the docs if they need the extra functionality.

[nicoddemus]

(I think you meant python-trio/trio#2898 and not python-trio/trio#2891)

Thanks, edited my post.

I can pare this down to a minimal implementation

[jakkdl]

Some polishing of docstrings and the like left, but this should be a decent MVP. With the logic being simpler I also added an assert_matches that gets used by the context manager, so the user is informed what part of the matching failed. (This is not in trio.RaisesGroup)

I didn't rename RaisesGroup to raises_group for now - since there's no longer a method that constructs the cm as with raises it feels weird to do lower case, and went with RaisesGroup in trio, but if you want it for consistency with raises I'm not entirely opposed to renaming it.

[nicoddemus]

I didn't rename RaisesGroup to raises_group for now

I think it is important for us to be consistent with pytest.raises, so I think we should definitely have pytest.raises_group.

However no need to rename the class, let us create a raises_group function, which returns the RaisesGroup instance, and expose that in the public API, leaving the RaisesGroup class an implementation detail.

@bluetech this is a new feature, but given it is isolated I think we can release this in 8.0, do you agree?

[bluetech]

@bluetech this is a new feature, but given it is isolated I think we can release this in 8.0, do you agree?

At this point I think it's better not to add new features to 8.0. But we can have a 8.1 not too long after.

[jakkdl]

I didn't rename RaisesGroup to raises_group for now

I think it is important for us to be consistent with pytest.raises, so I think we should definitely have pytest.raises_group.

However no need to rename the class, let us create a raises_group function, which returns the RaisesGroup instance, and expose that in the public API, leaving the RaisesGroup class an implementation detail.

What about RaisesGroup().matches? It looks quite weird to do raises_group(TypeError).matches() imo. Primary use case for me has been checking the __cause__ on an exception, from the docstring in trio:

       """Check if an exception matches the requirements of this RaisesGroup.
        Example::
            with pytest.raises(TypeError) as excinfo:
                ...
            assert RaisesGroups(ValueError).matches(excinfo.value.__cause__)
            # the above line is equivalent to
            myexc = excinfo.value.__cause
            assert isinstance(myexc, BaseExceptionGroup)
            assert len(myexc.exceptions) == 1
            assert isinstance(myexc.exceptions[0], ValueError)
        """

[nicoddemus]

t looks quite weird to do raises_group(TypeError).matches() imo

Definitely, but I believe the idea is to eventually have a matches= parameter to pytest.raises_group, like with pytest.raises_group(TypeError, matches=), for consistency with pytest.raises.

[jakkdl]

It looks quite weird to do raises_group(TypeError).matches() imo

Definitely, but I believe the idea is to eventually have a matches= parameter to pytest.raises_group, like with pytest.raises_group(TypeError, matches=), for consistency with pytest.raises.

No that's a different thing, this is for checking a constructed exception that hasn't been raised - something that pytest.raises doesn't do. Due to the ambiguity of a match parameter to raises_group (should it match the string of the group? or the exception within it? Or either?) I instead added a check parameter that takes a callable and is passed the exceptiongroup.

with pytest.raises_group(TypeError, check=lambda x: "foo" in getattr(x, "__notes__", ())):
  e = ExceptionGroup("", (TypeError(),))
  e.add_note("foo")
  raise e

and in the full implementation with Matcher as it is in trio, you can wrap exceptions in Matcher to do matches or checks on them. And now there's much less ambiguity on what the match parameter does so I also support it on the RaisesGroup object.

with trio.RaisesGroup(trio.Matcher(TypeError, match="bar"), match="foo")):
  raise ExceptionGroup("foo", (TypeError("bar"),))

but .matches() is an entirely separate thing, which I came up with when there was complicated logic checking the __cause__ on exceptions. So what previously required this code

with pytest.raises(TypeError) as exc_info:
  ...
myexc = excinfo.value.__cause__
assert isinstance(myexc, BaseExceptionGroup)
assert len(myexc.exceptions) == 1
assert isinstance(myexc.exceptions[0], ValueError)

could now be written

with pytest.raises(TypeError) as exc_info:
  ...
assert pytest.raises_group(ValueError).matches(excinfo.value.__cause__)

or if using RaisesGroup/raises_group (which has check=) we can even do

expected_cause = pytest.raises_group(ValueError)
with pytest.raises_group(TypeError, check=lambda x: expected_cause.matches(x.__cause__)):
  ...

Exposing that functionality was trivial, since that's already what was used internally, and cleaned up several instances in the test suite.

The equivalent in non-exceptiongroup cases would be:

with pytest.raises(TypeError) as exc_info:
  ...
assert isinstance(exc_info.value.__cause__, ValueError)

But I guess if you want to continue with the super minimal public API, I can simply make matches/assert_matches hidden methods and don't document them at all (or direct to trio.RaisesGroup).

[nicoddemus]

Initially I wanted to keep the API really simple, to avoid committing to something more complex right now, because I was hoping to get this released in 8.0.

But given this will no longer land in 8.0x, I think we can have more detailed discussion to include a more complete API regarding matches.

Another option is to actually hold deciding on a "matches" API/solution, taking advantage that trio has already released its own API, and see how it fares on the wild.

[nicoddemus]

@bluetech @RonnyPfannschmidt @The-Compiler @Zac-HD thoughts?

[Zac-HD]

• I agree that spelling it with pytest.raises_group(...) as _: is the right interface.
  • I'd expect a matches= parameter to match against the message of the group itself. Weak preference to decide what we're doing and then ship that in the first release to include raises_group()
• Having reviewed the Trio PRs, having a .matches(: BaseException) -> bool method is definitely useful. assert_matches(...) might let us give better messages on failure though.
  • We may therefore want RaisesGroup itself to also be public API?
  • I don't expect to learn much more from public uses of Trio's RaisesGroup; there's just not much open source code which handles ExceptionGroups! I don't expect much exotic use at work either, but we could wait ~months and check again I suppose.

[jakkdl]

• I agree that spelling it with pytest.raises_group(...) as _: is the right interface.

  • I'd expect a matches= parameter to match against the message of the group itself. Weak preference to decide what we're doing and then ship that in the first release to include raises_group()

minor note that the parameter is named match= and not matches=. But yeah that's the behavior I'd lean towards as well.

• Having reviewed the Trio PRs, having a .matches(: BaseException) -> bool method is definitely useful. assert_matches(...) might let us give better messages on failure though.

I might play around with the "full" implementation in the trio repository to see how to add better messages on failure for nested groups and the like, to see if both assert & assert_matches are useful to keep around in that case.

• We may therefore want RaisesGroup itself to also be public API?

It seems reasonable to expose RaisesGroup and also provide raises_group as a convenience alias.

I do think it's possibly a bit problematic that

with pytest.raises(ValueError):
  ...

and

with pytest.raises_group(ValueError):
  ...

are so similar, esp when there's tons of parameters and other busy stuff around it, whereas RaisesGroup is quite distinct. But differentiating wrapped- vs non-wrapped exceptions is probably not [going to be] a huge deal for most devs, given that except* accepts either.