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

TODO:

• also fix pytest.mark.xfail(raises=RaisesGroup(...)) ExceptionGroup ergonomics in pytest.param(..., marks=xfail(raises=...)) #12504

[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.

[Zac-HD]

Hi all - it's been quite a while since we started on this, and the Trio version has been working smoothly for most of that time now.

I'd like to propose that we adopt that into Pytest essentially unchanged, just using snake_case for the name, and merge in time for 9.0. Any objections? @jakkdl, can you rebase + add the remaining parts + ensure we get good test coverage?

[jakkdl]

@graingert no clue why I got a random fail on test_refcycle_unraisable_warning_filter_default https://github.com/pytest-dev/pytest/actions/runs/13034713512/job/36362225649?pr=11671

[graingert]

Bizarre, the match is clearly present in the output

[graingert]

Oh the output is on the stdout not the stderr, very strange. It looks like the gc was triggered even though it was disabled

[jakkdl]

Oh the output is on the stdout not the stderr, very strange. It looks like the gc was triggered even though it was disabled

oh it's not random https://github.com/pytest-dev/pytest/actions/runs/13035797919/job/36365825317?pr=11671
but why in this PR (seems to pass on main)... and why pluggy? :S

[graingert]

Oh I'm somewhat of an idiot, how's disable_gc supposed to work across into a subprocess

I have this failing locally. pipx run tox -e py39-pluggymain-pylib -- testing/test_unraisableexception.py::test_refcycle_unraisable_warning_filter_default fails but py39-pluggymain passes py39-pylib also passes, so it's an interaction.

[jakkdl]

I'd like to propose that we adopt that into Pytest essentially unchanged, just using snake_case for the name, and merge in time for 9.0. Any objections? @jakkdl, can you rebase + add the remaining parts + ensure we get good test coverage?

Done!

There's a couple decision points:

1. raises is now ~strictly inferior to Matcher, so there's a pretty decent case for making it use Matcher under the hood and get rid of some code duplication. The legacy calling convention of raises(DivisionByZeroError, 1/0) means we can't just make it a convenience alias (for now.. deprecation time?), but shouldn't be too bad
2. I .. should probably add back assert_matches? assert (m := Matcher(TypeError)).matches(e), m.fail_reason is not terribly ergonomic
3. idk I should reread the PR in its entirety there's probably something lurking somewhere

[graingert]

something like this:

From 76deeca5aa5cee73e973f5c667c3f73b68892b48 Mon Sep 17 00:00:00 2001
From: Thomas Grainger <[email protected]>
Date: Wed, 29 Jan 2025 17:02:09 +0000
Subject: [PATCH] disable gc at a process level for unraisablehook tests

---
 testing/test_unraisableexception.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/testing/test_unraisableexception.py b/testing/test_unraisableexception.py
index 3f191073e..df5894af5 100644
--- a/testing/test_unraisableexception.py
+++ b/testing/test_unraisableexception.py
@@ -321,6 +321,9 @@ def test_refcycle_unraisable_warning_filter_default(pytester: Pytester) -> None:
     # see: https://github.com/pytest-dev/pytest/pull/13057#discussion_r1888396126
     pytester.makepyfile(
         test_it="""
+        import gc
+        gc.disable()
+
         import pytest
 
         class BrokenDel:
@@ -335,8 +338,7 @@ def test_refcycle_unraisable_warning_filter_default(pytester: Pytester) -> None:
         """
     )
 
-    with _disable_gc():
-        result = pytester.runpytest_subprocess("-Wdefault")
+    result = pytester.runpytest_subprocess("-Wdefault")
 
     assert result.ret == pytest.ExitCode.OK
 
-- 
2.43.0

I can only guess that something in the new pluggy with py installed causes a threshold of memory allocated to be reached which runs the GC which wasn't correctly disabled.

do you want me to push to this branch? or open a new PR with just this patch?

[jakkdl]

do you want me to push to this branch? or open a new PR with just this patch?

It might take some time before this PR gets merged, so you probably want it in a separate one :)

[Zac-HD]

@jakkdl I suggest replacing this with a fresh PR, that can describe exactly what we're doing and reference the Trio implementation - I'm concerned that reviewing this with the stale comments above will get confusing.

Plausibly waiting until after the gc patch lands so that we can start from green CI.