Add reason for failed match to RaisesGroup

[jakkdl]

As another preparatory step for adding RaisesGroup into pytest, I thought I'd finally get around to making it.. not a nightmare to debug why it fails to match. When I've been using RaisesGroup with a complicated structure I've sometimes found it very tricky to figure out why it's not matching - and I imagine that's 10x worse for somebody not intimately familiar with its innards.

This does introduce a major change in behavior, previously RaisesGroup, like pytest.raises, would silently pass through the exception if it didn't match - but now it will instead fail with an AssertionError. This also has precedent upstream though, pytest.raises will fail with an AssertionError iff you've specified match.

You still see the original exception in the traceback, and in many ways I think always failing with an AssertionError is more legible.
I don't think this will impact end user's test suites in a majority of cases, unless they're either testing RaisesGroup behavior itself, or doing some very weird nesting. But even if so, they can rewrite their code as:

with pytest.raises(AssertionError) as exc_info:
    with RaisesGroup(...):
        ...
assert isinstance(exc_info.type, ValueError)

Another improvement would be making .matches() directly raise an AssertionError, instead of quietly setting .fail_reason. This would break any test currently doing if not RaisesGroup(...).matches(...):, or even more plausibly assert not RaisesGroup(...).matches(...), so
I think I should add a new method .assert_matches() to both RaisesGroup and Matcher, which either calls .matches() and asserts the return value with fail_reason as the message - or do it the other way around and have .matches() call .assert_matches() in a try:.

There's lots of bikeshedding possible with the phrasing of each error message, and am not completely happy with nested cases, so would very much appreciate any suggestions.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (b41bb13) to head (9612d1e).

Additional details and impacted files

@@               Coverage Diff                @@
##                 main        #3145    +/-   ##
================================================
  Coverage   100.00000%   100.00000%            
================================================
  Files             124          124            
  Lines           18460        18637   +177     
  Branches         1216         1243    +27     
================================================
+ Hits            18460        18637   +177     

Files with missing lines	Coverage Δ
src/trio/_tests/test_exports.py	100.00000% <100.00000%> (ø)
src/trio/_tests/test_testing_raisesgroup.py	100.00000% <100.00000%> (ø)
src/trio/testing/_raises_group.py	100.00000% <100.00000%> (ø)

[Zac-HD]

I'm excited to see continued progress here! Really good support for ExceptionGroup in testing is one of those things that doesn't seem like a huge problem until you're living with it 😅

[Zac-HD]

Even after staring at this for a while, I'm having trouble matching up the corresponding lines to matchers to leaf exceptions. Can you add some comments laying out the intent?

I think that's mostly because RaisesGroup doesn't care about the order of matches, but I'm concerned that a greedy algorithm will fail to find a passing match when one exists (although exhaustive checks could be quite expensive in the worst case!).

I think our error messages are also unhelpful or even misleading here, in that the lines of output all seem to be describing the first leaf exception, without accounding for the fact that they might be (close) matches for other leaves or subgroups. Instead, I suggest trying to match each sub-exception.

• Set aside whatever matchers and subgroups have a 1:1 correspondence. This is probably the common case; i.e. one or more missing or unexpected exceptions. Report each missing exception and unused matcher.
• If it's more complicated than that - for example, two TypeErrors and only one matcher - report all of those as well. I don't think it's worth trying to work out which are "closer to matching", or which to set aside; the user can sort it out.

[jakkdl]

sorry yeah, it's failing to match any of the expected exceptions with the first exception in the group. But had to pass more exceptions in the group to make it pass the number-of-exceptions check. Will add comments :)

The algorithm is currently described in the class docstring:

trio/src/trio/testing/_raises_group.py

Lines 376 to 382 in 993a173

	The matching algorithm is greedy, which means cases such as this may fail::
	with RaisesGroups(ValueError, Matcher(ValueError, match="hello")):
	raise ExceptionGroup("", (ValueError("hello"), ValueError("goodbye")))
	even though it generally does not care about the order of the exceptions in the group.
	To avoid the above you should specify the first ValueError with a Matcher as well.

But I suppose I should at the very least also mention it in the docstring of matches().

I very much like your idea, will give it a shot!
This probably also means we should stop abandoning as soon as the number of exceptions is wrong, with RaisesGroup(ValueError, TypeError): raise ExceptionGroup("", [ValueError]) can point out that TypeError is the one unmatched exception.
This makes my current suggest-flattening logic impossible though, so I'll go ahead with run-it-again-but-with-flattening-if-it-fails.

[jakkdl]

Made an attempt at generating a more verbose error message, and while this is an intentionally complicated example it's also fairly reasonable that a user expects a complicated structure, but instead gets a very different exceptiongroup with very different errors.

So for this code:

with RaisesGroup(
    RaisesGroup(ValueError),
    RaisesGroup(RaisesGroup(ValueError)),
    RaisesGroup(Matcher(TypeError, match="foo")),
    RaisesGroup(TypeError, ValueError)
):
    raise ExceptionGroup(
        "", [TypeError(), ExceptionGroup("", [TypeError()]),
             ExceptionGroup("", [TypeError(), TypeError()])]
    )

my current WIP implementation outputs the following:

=========================================================================================== FAILURES ===========================================================================================
__________________________________________________________________________________ test_assert_message_nested __________________________________________________________________________________
  + Exception Group Traceback (most recent call last):
  |   File "/home/h/Git/trio/raisesgroup_fail_reason/src/trio/_tests/test_testing_raisesgroup.py", line 533, in test_assert_message_nested
  |     raise ExceptionGroup(
  | ExceptionGroup:  (3 sub-exceptions)
  +-+---------------- 1 ----------------
    | TypeError
    +---------------- 2 ----------------
    | ExceptionGroup:  (1 sub-exception)
    +-+---------------- 1 ----------------
      | TypeError
      +------------------------------------
    +---------------- 3 ----------------
    | ExceptionGroup:  (2 sub-exceptions)
    +-+---------------- 1 ----------------
      | TypeError
      +---------------- 2 ----------------
      | TypeError
      +------------------------------------

During handling of the above exception, another exception occurred:

    def test_assert_message_nested() -> None:
>       with RaisesGroup(
            RaisesGroup(ValueError),
            RaisesGroup(RaisesGroup(ValueError)),
            RaisesGroup(Matcher(TypeError, match="foo")),
            RaisesGroup(TypeError, ValueError)
        ):
E       AssertionError: Raised exception did not match: Failed to match expected and raised exceptions.
E       The following expected exceptions did not find a match: {RaisesGroup(Matcher(TypeError, match='foo')), RaisesGroup(ValueError), RaisesGroup(RaisesGroup(ValueError)), RaisesGroup(TypeError, ValueError)}
E       The following raised exceptions did not find a match
E         TypeError():
E           RaisesGroup(Matcher(TypeError, match='foo')): TypeError() is not an exception group
E           RaisesGroup(ValueError): TypeError() is not an exception group
E           RaisesGroup(RaisesGroup(ValueError)): TypeError() is not an exception group
E           RaisesGroup(TypeError, ValueError): TypeError() is not an exception group
E         ExceptionGroup('', [TypeError()]):
E           RaisesGroup(Matcher(TypeError, match='foo')): Failed to match: Matcher(TypeError, match='foo'): Regex pattern 'foo' did not match ''
E           RaisesGroup(ValueError): Failed to match: TypeError() is not of type 'ValueError'
E           RaisesGroup(RaisesGroup(ValueError)): Failed to match: RaisesGroup(ValueError): TypeError() is not an exception group
E           RaisesGroup(TypeError, ValueError): Too few exceptions raised! The following expected exception(s) found no match: {<class 'ValueError'>}
E         ExceptionGroup('', [TypeError(), TypeError()]):
E           RaisesGroup(Matcher(TypeError, match='foo')): Failed to match expected and raised exceptions.
E             The following expected exceptions did not find a match: {Matcher(TypeError, match='foo')}
E             The following raised exceptions did not find a match
E               TypeError():
E                 Matcher(TypeError, match='foo'): Regex pattern 'foo' did not match ''
E               TypeError():
E                 Matcher(TypeError, match='foo'): Regex pattern 'foo' did not match ''
E       
E           RaisesGroup(ValueError): Failed to match expected and raised exceptions.
E             The following expected exceptions did not find a match: {<class 'ValueError'>}
E             The following raised exceptions did not find a match
E               TypeError():
E                 TypeError() is not of type 'ValueError'
E               TypeError():
E                 TypeError() is not of type 'ValueError'
E       
E           RaisesGroup(RaisesGroup(ValueError)): Failed to match expected and raised exceptions.
E             The following expected exceptions did not find a match: {RaisesGroup(ValueError)}
E             The following raised exceptions did not find a match
E               TypeError():
E                 RaisesGroup(ValueError): TypeError() is not an exception group
E               TypeError():
E                 RaisesGroup(ValueError): TypeError() is not an exception group
E       
E           RaisesGroup(TypeError, ValueError): Failed to match expected and raised exceptions.
E             The following expected exceptions did not find a match: {<class 'ValueError'>}
E             The following raised exceptions did not find a match
E               TypeError():
E                 TypeError() is not of type 'ValueError'
E                 It matches <class 'TypeError'> which was paired with TypeError()

src/trio/_tests/test_testing_raisesgroup.py:527: AssertionError

There's about a million things to tweak, and this doesn't hit all logic (in particular when some-but-not-all exceptions match), but do you like this as the basic structure? It feels kind of insane to have an exception message that's 45 lines - and given the quadratic nature could quickly spiral even further - but maybe that's fine?

[Zac-HD]

Matcher(OSError, check={check_errno_is_5!r}): check did not return True for OSError(6, '') seems reasonable to me? (and see above for Hypothesis' pprinter idea...)

[jakkdl]

The problem with that is in the RaisesGroup case, if you don't want RaisesGroup(RaisesGroup(ValueError, check=my_fun)) to print the function twice - then RaisesGroup(ValueError, check=my_fun) will not print it at all.
The only way out of that is to not print it in the overview (maybe something like Raises(ValueError, check=...): check {check_errno_is_5!r} did not return True for OsError(6, '')), or to use logic to see if we're nested or not (I suppose we already have _depth)*

Never printing it in the un-nested case is probably fine a majority of the time, but if you're doing something like for f in ...: with RaisesGroup(..., check=f) then you'd need an extra debugging cycle.

* I'll go ahead with this latter one for now

[jakkdl]

fixed that case, but still printing it twice in nested cases:

trio/src/trio/_tests/test_testing_raisesgroup.py

Lines 718 to 726 in a8e263c

	# in nested cases you still get it multiple times though...
	with pytest.raises(
	AssertionError,
	match=wrap_escape(
	f"Raised exception group did not match: RaisesGroup(Matcher(OSError, check={check_errno_is_5!r})): Matcher(OSError, check={check_errno_is_5!r}): check did not return True for OSError(6, '')",
	),
	):
	with RaisesGroup(RaisesGroup(Matcher(OSError, check=check_errno_is_5))):
	raise ExceptionGroup("", [ExceptionGroup("", [OSError(6, "")])])

[Zac-HD]

we do this dance often enough that a helper function might be in order,

def fails_raises_group(msg):
    prefix = "Raised exception group did not match: "
    return pytest.raises(AssertionError, match=wrap_escape(prefix + msg))

and I'd also consider the parenthesized-with statement now that we've dropped Python 3.8

[jakkdl]

I'm somewhat skeptical of the parenthesized-with here because we're testing the context manager, rather than the innermost body being what the focus is on, but testing it just for the sake of exciting "new" feature ^^

[Zac-HD]

I'm fine with printing a table here, but we could just remove the repetition:

Suggested change

	" ExceptionGroup('', [ValueError(), TypeError()]):\n"
	" ExceptionGroup('', [ValueError(), TypeError()]) is not of type 'ValueError'\n"
	" ExceptionGroup('', [ValueError(), TypeError()]) is not of type 'TypeError'\n"
	" ExceptionGroup('', [ValueError(), TypeError()]):\n"
	" -> not of type 'ValueError'\n"
	" -> not of type 'TypeError'\n"

(particularly because I expect most reprs to be considerably longer than this)

[jakkdl]

This turned out to be very messy, so I instead ended up printing type(e).__name__ on a type mismatch, and not print the repr if check failed, and I think that ultimately resolved it cleanly. There's some non-nested simple cases that now doesn't print the repr at all, but I think they should all be fine.

[Zac-HD]

this note is going to save someone hours of frustration 👍

[jakkdl]

I think this is also the reason to get rid of

trio/src/trio/testing/_raises_group.py

Lines 765 to 768 in 8281d7c

	# TODO: I'm not 100% sure about printing the exact same as in the case above
	# And in all cases of len(failed_attemts_dict)==1 we probably don't need a full table
	if 1 == len(remaining_exceptions) == len(failed_attempts_dict):
	return f"{succesful_str}{failed_attempts_dict.popitem()[1][0]}"

When RaisesGroup is sensitive to ordering I think it can be quite important to print duplicate matches. I was thinking "if there's only exception that didn't get matched, and only one expected that had no raised, then we surely only care why those two didn't match" but while that may be the majority of cases it's not always the case.

(edited because I mixed up my scenarios)

edit2: wait no, I can just add another condition to see if there's any "would've-paired-but-ordering-made-them-not". But that's for another day

[jakkdl]

The only remaining TODO now is

trio/src/trio/_tests/test_testing_raisesgroup.py

Lines 103 to 110 in 9612d1e

	# TODO: this one should maybe say that it matched an already matched exception
	with (
	fails_raises_group(
	"1 matched exception. Too few exceptions raised, found no match for: [<class 'ValueError'>]"
	),
	RaisesGroup(ValueError, ValueError),
	):
	raise ExceptionGroup("", (ValueError(),))

which I'm procrastinating on because _check_exceptions is becoming an unwieldy beast.
I think I'm sticking to my guns on the matching being greedy, but _check_exceptions is so close to being fully exhaustive on fail already that we can probably have a message for "there is a pairing that would've worked, you should specify your requirements more stringently to allow the greedy matching to work"

[CoolCat467]

Run failure for pypi looks familiar, I think the issue last time was that package cache pypi runner was using had cached an outdated version of what uv versions were available, leading it to believe that constraint-specified version of uv does not exist when it actually does in reality, just using an old cache.

[jakkdl]

Code is perhaps a bit messy, and the tests definitely are (I think I'll restructure them when moving to pytest), but otherwise I'm happy with current functionality. So please review away!