Add support to testing.RaisesGroup for catching unwrapped exceptions

[jakkdl]

• Deprecates the strict parameter, renaming it to flatten_subgroups.
• Adds the allow_unwrapped parameter, adding the possibility to more closely mirror except*. This is a feature requested in Allow pytest.raises cooperate with ExceptionGroups pytest-dev/pytest#11538 (comment) by @belm0
• Fixes a bug I noticed while reading the code, where e.g.

with RaisesGroup(ValueError, TypeError, strict=False):
  raise ExceptionGroup("", [ExceptionGroup("", [ValueError(), TypeError()])])

would fail due to it checking if the number of exceptions was correct before flattening the structure of the raised exceptiongroup.

I also found an unrelated issue where mypy&pyright are not able to deduce the type when passing multiple Matcher objects matching against different exceptions to RaisesGroup. (or Matcher+class, but two classes work). Fiddled around a little bit but wasn't able to quickly find a fix, and explicitly setting [Exception] isn't especially onerous.
EDIT: this was due to TypeVar lacking covariance.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.63%. Comparing base (ccd40e1) to head (6ef442b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2989   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files         120      120           
  Lines       17710    17798   +88     
  Branches     3179     3198   +19     
=======================================
+ Hits        17645    17733   +88     
  Misses         46       46           
  Partials       19       19           

Files	Coverage Δ
src/trio/_core/_run.py	99.34% <ø> (ø)
src/trio/_core/_unbounded_queue.py	100.00% <ø> (ø)
src/trio/_deprecate.py	100.00% <100.00%> (ø)
src/trio/_highlevel_open_tcp_listeners.py	100.00% <ø> (ø)
src/trio/_tests/test_deprecate.py	100.00% <100.00%> (ø)
src/trio/_tests/test_testing_raisesgroup.py	100.00% <100.00%> (ø)
src/trio/_threads.py	100.00% <ø> (ø)
src/trio/testing/_raises_group.py	100.00% <100.00%> (ø)

[jakkdl]

Oh, turns out the unrelated typing error does work with pyright even with the overloads, and it's solely a mypy thing. I'll try to get a minimal repro and/or see if it's a known mypy issue and/or report it.
EDIT: fixed by making TypeVar covariant

[jakkdl]

@TeamSpen210 after adding covariant=True to the TypeVar the docs/source/typevars.py fails to resolve it:

/home/h/Git/trio/looser_excgroups/src/trio/testing/__init__.py:docstring of trio.testing.RaisesGroup:1: WARNING: py:obj reference target not found: typing.Callable[[BaseExceptionGroup[+E]], bool] | None
/home/h/Git/trio/looser_excgroups/src/trio/testing/__init__.py:docstring of trio.testing.Matcher:1: WARNING: py:class reference target not found: type[+E] | None
/home/h/Git/trio/looser_excgroups/src/trio/testing/_raises_group.py:docstring of trio.testing._raises_group._ExceptionInfo.type:1: WARNING: py:class reference target not found: type[+E]

I'm a bit surprised as covariant typevars exist in a few other places in the codebase without issue. Unless you have any opinions I'll try do some ugly workaround where I either add a plussed [copy] to the dicts in identify_typevars, or strip +s from the target in lookup_reference

EDIT: nope, wasn't that easy to work around...

[jakkdl]

I can make a theoretical case for splitting strict=False into two separate flags - one that allows unwrapped exceptions, and one that flattens nested exceptiongroups. But I suspect the added burden of having to set both might outweigh the gain from somebody that actually wants them split:

# they want this to pass
with RaisesGroup(ValueError, strict=False):
    raise ExceptionGroup("outer", [ExceptionGroup("inner", [ValueError()])])
# but this to fail
with RaisesGroup(ValueError, strict=False):
    raise ValueError

Of course they can just do

with RaisesGroup(..., strict=False) as e:
    ...
assert isinstance(e, ExceptionGroup)

but that's easy to forget and could silently introduce changes in behavior.

One, probably better, case in favor of splitting out allow_unwrapped=True is that we can raise an error in RaisesGroup.__init__ if specifying several exceptions + allow_unwrapped=True. That's both easier to implement and faster to debug for end-users than trying to resolve that with a message on a failed catch #2989 (comment)

Since typing with RaisesGroup(ValueError, allow_unwrapped=True, flatten=True) is a mouthful we (or leave that for pytest) could add convenience aliases, or just rely on end-users to write their own thin wrappers that change the defaults.

[TeamSpen210]

Fixed the typevar-related issues, but now there's something else in history.rst. Not fully sure why the +E is only happening with this typevar, but I found a workaround. By using autodoc_process_signatures() I could insert the fully qualified name for the var, allowing Sphinx to find it.

[Zac-HD]

looks good to me, once the build passes let's merge!

[jakkdl]

Seems like nobody else had opinions on strict vs allow_unwrapped+flatten_subgroups. The ability to raise a ValueError on users doing RaisesGroup(SyntaxError, TypeError, allow_unwrapped=True) with a helpful message on what else to do convinced me that this is the way to go, even if it will be more verbose when you want to fully emulate except* (i.e. you expect a single exception, but don't care if it's unwrapped, or in any level of nesting).

[CoolCat467]

Looks good. I think I saw a few regexes reading through the changes that could still have end specifiers added, but I think it should be ok.

[jakkdl]

Looks good. I think I saw a few regexes reading through the changes that could still have end specifiers added, but I think it should be ok.

The missing end specifiers are intended because I didn't bother including the full error message.

I fixed it so RaisesGroup now supports it on ExceptionGroup - previously it failed since it included "1 sub-exception" in the message it matched against, since

>>> str(ExceptionGroup('foo', [ValueError()]))
'foo (1 sub-exception)'

[A5rocks]

Why aren't we using our deprecation utils in this case? Cause those would have the version this was deprecated in which is useful information.

[A5rocks]

(If it's because they don't warn with something derived from DeprecationWarning that makes sense. Maybe add a kwarg to warn_deprecated?)

[jakkdl]

mostly just me being confused about the existence of TrioDeprecationWarning (hence #2992). One weird part with warn_deprecated is you have to know what the next version bump will be, but I'm guessing I should mark it as 0.25.1.
But I also don't think this one is worthy of an official extended period of supporting it, it's a trivial change to use flatten_subgroups=True instead and this is explicitly a temporary helper until it's merged into pytest, so it's barely worth having a DeprecationWarning at all. It's not used at runtime either, so it can only break tests. But I guess TrioDeprecationWarning doesn't have an explicit promise of how long it will be kept deprecated anyhow

[A5rocks]

A few comments. Sorry for taking a while, I wasn't sure whether we should merge this yet in case we want a patch release w/ those embedded Python fixes, but I haven't made that release yet so... Oh well.

[jakkdl]

oh sorry for the re-request, didn't get notified about your review

[jakkdl]

oh sorry for the re-request, didn't get notified about your review

fixed everything now though, so re-review request is warranted

[jakkdl]

I encountered some issues with typing for functions passed to the check argument, and thought about handling that in a separate pull request - but it got so thorny that I'll just add the failing tests now and maaaybe address it in a separate PR once I hear from python/mypy#17251

[A5rocks]

Suggested change

	match: None = None,
	check: None = None,

I don't think it's necessary to provide these. a) we're adding the param now so no backcompat issues and b) I don't think we should care about backcompat for types because they're just hints.

[jakkdl]

Hmm, I think it doesn't hurt to have them, and allows users to do some automated stuff where they pass variables that they know to be None:

for allow_unwrapped,match in zip([True, None], [False, "hello"]):
  with RaisesGroup(Exception, allow_unwrapped=allow_unwrapped, match=match):
    ...

[A5rocks]

Also re: your mypy bug, my impression was that it just ignores __new__ in favor of __init__ if it's available.

[jakkdl]

Also re: your mypy bug, my impression was that it just ignores __new__ in favor of __init__ if it's available.

scrolling through https://github.com/python/mypy/blob/master/test-data/unit/check-classes.test I guess that is the current state of things with mypy. But after sleeping on it I think I have a better way of resolving it - but that's for a new PR. It's finally time to merge this one 🚀