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 99.62%. Comparing base (4f66fab) to head (8281d7c).

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3145    +/-   ##
========================================
  Coverage   99.62%   99.62%            
========================================
  Files         122      122            
  Lines       18407    18577   +170     
  Branches     1226     1250    +24     
========================================
+ Hits        18338    18508   +170     
  Misses         47       47            
  Partials       22       22            

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

[CoolCat467]

Wouldn't classes that use __slots__ define members before __init__ as well? It's just that they wouldn't have any values.

[jakkdl]

ye I suppose __slots__ can also make this test work. Pytest seems to only use it sparingly though so don't think I will bother.

[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

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

[Zac-HD]

Could say "inner ExceptionGroup(..." and Did you mean to use flatten_subgroups=True[ instead of allow_unwrapped=True]?" I guess?

[Zac-HD]

I think this is false and we don't need allow_unwrapped here?

[Zac-HD]

I think not-suggesting is correct in this case.

[Zac-HD]

this note is going to save someone hours of frustration 👍