add RaisesGroup & Matcher

[jakkdl]

Previous PR's: #11656 (ExpectedExceptionGroup), #11671 (closed to give this a fresh PR w/o stale comments)
fixes: #11538 #12504

This adds RaisesGroup (also exported as raises_group) and Matcher, to allow a robust way of expecting ExceptionGroup and the ability to specify the structure of nested groups, etc etc.

with RaisesGroups(ValueError):
    raise ExceptionGroup("", (ValueError(),))

This is the exact implementation available in trio.testing.RaisesGroup which has been in use since december 2023 and gone through a couple iterations since to settle on a good interface. The most recent addition was python-trio/trio#3145 which added error messages, where the formatting can probably still be fine-tuned.

Pytest currently has excinfo.group_contains for checking exceptiongroups... which is very problematic. I think it should be deprecated, either in the same release as this PR, or very soon after.

This currently does not touch the implementation of pytest.raises, but if we made Matcher a contextmanager we could easily support

with Matcher(ValueError):
    raise ValueError()

this would fix e.g. #12763 with Matcher having the check parameter. The only complication is the legacy form of pytest.raises but that should be solvable with a small helper function.
This can however be done in a followup PR.

This currently does not add an .assert_matches method, which previous iterations of the PR had. The reason to add that would be to improve the error message on failure, though it's possible to get the same result with

assert (m := Matcher(TypeError)).matches(e), m.fail_reason
# versus
Matcher(TypeError).assert_matches(e)

though note that .matches() is already a fairly niche feature that is entirely separate to the context-manager use (not to be confused with the match parameter).

TODO:

• bump the issue # on the newsfragment file
• documentation

[RonnyPfannschmidt]

We ought not to use such a generic name easily

There's a general wishlist plan to enable matchers as a whole that act similar to dirty equals

The matcher api that's being used here conflates multiple aspects of that in a manner that's problematic to partition later

Ideally the whole api around matchers is first evolved as a plugin to avoid early api lock in as it's hard to make good matchers

I'll elaborate further when the flu is over

[jakkdl]

sure, I can rename it... maybe something that's closer to RaisesGroup... RaisesExc/RaisesException? or something like ExpectedExc/ExpectedException

This has been available & in use for >1 year in trio, which has allowed the interface to mature. I don't think pytest needs to repeat that process by putting it in a plugin.

I know you've mentioned matcher objects in #11538 and I'm excited for what that might entail in the future, but I don't think we want to wait for that to add usable exceptiongroup support. If you have concrete ways of improving the RaisesGroup interface to harmonize with it I'm of course open to suggestions though.

[Zac-HD]

I've been heavily involved in the design and review for this feature as we've developed and shipped it in Trio. I'm excited to get it into Pytest (and ideally soonish), but I shouldn't be the only approving core dev for a substantial feature.

[jakkdl]

@Zac-HD please review the documentation :)

It feels a bit silly to add big warning boxes to ExceptionInfo.group_contains and not deprecate it, but it also feels silly to deprecate it one release after adding it.

[Zac-HD]

@Tusenka unfortunately pull requests are the wrong place to ask questions like this, unless you're either the author of the PR or a maintainer of the package. I suggest StackOverflow instead.

[jakkdl]

please bring basic questions like these to StackOverFlow, the python discord, ask an LLM of your choice, or otherwise. This is not relevant to the PR at hand and only wastes the time of maintainers without any benefit to pytest.

[jakkdl]

I think I'm mostly just waiting for a quick pass on docs changes from @Zac-HD (or others), and otherwise this can be merged and I can look at potential followup for pytest.raises and/or deprecating excinfo.group_contains

[nicoddemus]

Thanks a lot @jakkdl for the excellent work here!

I left mostly requests for doc changes, and a recommendation regarding pytest.raises_group, please take a look.

[nicoddemus]

Perhaps it would be best to not export RaisesGroup at all, keeping only pytest.raises_group as the official interface?

pytest.raises_group harmonizes well with the existing pytest.raises, plus I don't see any benefit of having two ways of doing the same thing. Another argument for that is that we do not expose the context manager used in pytest.raises either, so I don't think we should do the same here.

I would make the pytest.raises_group the first-class citizen in the API, we can keep RaisesGroup in the docs if we like, but I would make all examples use pytest.raises_group only.

@The-Compiler might want to comment on that given he does a lot of pytest training.

[jakkdl]

yeah only having a single name makes sense, but IMO it behaves so much like a class, and will get used a lot like a class, that the snake_case looks very weird. And surely RaisesGroup and RaisesExc should be the same? but raises_exc is then close-to-identically-named to raises which means it should be renamed, or they should be harmonized.

I think these examples look weird, but maybe that's just me?

with raises_group(raises_group(ValueError), raises_exc(match="foo")):
    ...

if raises_group(ValueError).matches(my_exc): # this one is especially bad imo
    ...

rg = raises_group(ValueError)
assert rg.matches(my_exc), rg.fail_reason

@pytest.mark.xfail(raises=raises_group(ValueError))
def foo():
    ...

I guess one might frame it as raises_group is a factory for creating RaisesGroup.. but then the user will assume there's a distinction and a reason for that distinction.

What I'm afraid might happen is people thinking that they need to do

with raises_group(RaisesGroup(ValueError)):
   ...

especially if the type hints specify RaisesGroup... which means that maybe the class itself should be named in snake_case??? Ew. Or people just not considering the possibility that it does a lot more than pytest.raises has historically supported.

In my very personal opinion the ideal would be having pytest.RaisesGroup & pytest.RaisesExc, and pytest.raises being a thin wrapper around pytest.RaisesExc to add support for the legacy calling mode. But if I'm in the minority here then /shrug

[jakkdl]

Another argument for that is that we do not expose the context manager used in pytest.raises either, so I don't think we should do the same here.

Wait sorry I only now properly parsed this. I don't think you should pattern-match this beast with pytest.raises, it's perhaps an overly engineered beast - but RaisesGroup is way more than just "the context manager" of a function call. It has public attributes in fail_reason & matches that are meant to be used - and it even supports modifying any of the parameters passed to __init__ after creation (though that bypasses the verification in __init__ so it is a bit dangerous).

[nicoddemus]

I guess one might frame it as raises_group is a factory for creating RaisesGroup.. but then the user will assume there's a distinction and a reason for that distinction.

Well at least this is consistent with pytest.raises, which is also just a factory. I personally find it fine to have pytest.raises_group a factory, and a separate RaisesExc as a class, given their usage is different anyway.

As an additional data point, we have pytest.param which is similar in purpose to RaisesExc, so perhaps this is another argument for pytest.raises_exc.

But I get your points, but I'm sure at least some users will wonder why there is pytest.raises and pytest.RaisesGroup.

But if we move forward with having pytest.RaisesGroup I would remove the pytest.raises_group alias, I don't see much benefit of having the alias in place.

But I'm not deadset on this either, just find the inconsistency a bit unnerving and might something to come up for new users. 👍

As I mentioned before, this is excellent work overall, I'm bringing this up because user facing API is important and is something we have overlooked in the past (regarding consistency I mean).

[jakkdl]

I don't think RaisesGroup and RaisesExc has significant differences in their use, the only difference is that RaisesExc does not support the context-manager use - but I kind of think it should and that RaisesContext should be removed. I've refrained from bundling that as yet another part of this PR, but I'm starting to think I should.

pytest.param is a good data point. And maybe the harmonization ends up with not needing a distinction between raises_exc and raises, in which case we get

with pytest.raises_group(pytest.raises(ValueError), pytest.raises_group(TypeError)):
    ...

I'll give it a go, and if the details on that becomes worthy of extensive review&discussion I'll split it off into another PR.

I'm very happy to get feedback! I've spent an ungodly amount of time on this feature, so I do have very strong personal opinions at this point, but hopefully I can soon let it go into the collective consciousness~

[nicoddemus]

but I kind of think it should and that RaisesContext should be removed.

Do you mean:

pytest/src/_pytest/python_api.py

Line 1030 in b0caf3d

class RaisesContext(AbstractContextManager[_pytest._code.ExceptionInfo[E]]):

Or is that a typo?

But before moving on, I would recommend to wait a few days for other opinions -- it is possible people will say "I agree with @jakkdl and @nicoddemus is being too cautious, this will not be a problem in practice" and we can just move forward, hehehe.

[jakkdl]

no, not a typo. The logic in RaisesContext is a ~strict subset of the logic in RaisesGroup, so if I move __enter__ and __exit__ to AbstractMatcher I can make pytest.raises return RaisesExc (and also offload some input validation from pytest.raises to RaisesExc).

That also adds support for e.g. pytest.raises(check=lambda e: isinstance(e.__cause__, ValueError)). #12763

[nicoddemus]

Can we elaborate on this a bit? Someone who does not understand the history behind this feature will have a hard time understanding this tip.

Probably also worth adding this under a .. note::?

[jakkdl]

Gave it a go, and removed it from RaisesExc to avoid repetition. (it's valid for RaisesExc as well, but you're unlikely to care if only using RaisesExc).

[jakkdl]

so, uh, sorry to everybody reviewing this, cause this last commit is a monster. I noticed a ton of small random things (though I probably shouldn't have bothered with backquotes tbh)

Anyway, this gets rid of RaisesContext and simplifies pytest.raises a lot. There were a couple niche things I hadn't incorporated into RaisesExc/RaisesGroup (tuples of exceptions, generics) and a bunch of differences in formatting.
One of the bigger changes is whether an unexpected exception is let through or not - when the matching is non-trivial I opted to change it so that RaisesExc/RaisesGroup always raised Failed, with the original exception as context, partly I think that's a superior interface, but also because I needed to output the reason why the match failed somewhere.

There's also a question of AssertionError vs Failed - currently RaisesExc will raise AssertionError on any failed match for the sake of minimizing the diff in tests, but I think Failed is better.

If any of this stuff is controversial I'll break stuff off into a separate PR, and I was hoping to do some of this in a follow-up PR, but it ended up becoming relevant to the discussion so I just did it in here for now.