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 (3e0058b) to head (561660b).

Additional details and impacted files

@@               Coverage Diff                @@
##                 main        #3145    +/-   ##
================================================
  Coverage   100.00000%   100.00000%            
================================================
  Files             124          124            
  Lines           18460        18723   +263     
  Branches         1216         1265    +49     
================================================
+ Hits            18460        18723   +263     

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 😅

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

[Zac-HD]

I suggest we check the two tweaks to messages below, and then come back for the hypothesis-plugin-monkeypatch in a followup PR. Otherwise, looks great and I can't wait to use it!

[Zac-HD]

Could we just put " " at the start of the message to get this effect?

[jakkdl]

..yes! And can actually resolve the flatten_subgroups problem as well by only doing so when the old fail_reason isn't multi-line.

[Zac-HD]

ah... if we do the monkeypatch from Trio's hypothesis plugin as I suggest above, we'll then also want to use the maybe-monkeypatched _check_repr function to construct the expected string; and maybe also assert that that string is one of two known possibilities. Which is kinda ugly, but imo showing the lambda source here is nice enough to be worth it.

[jakkdl]

oh.. I just realized if hypothesis start always monkeypatching it as soon as it's installed then the test suite would fail on install as well. Or possible even in some scenarios of hypothesis-pytest-plugin thingie thingies.

I'll split out a test case showing how ugly the lambda repr is and make it skip if hypothesis is in sys.modules, and convert the rest to use _check_repr, to avoid future headaches.

[A5rocks]

I haven't looked that hard at the logic but this looks good! (note to self: restart review at _check_exceptions)

This is mostly wording/nitpicks/minor changes. A bunch of them, but it's a few concerns duplicated:

• I don't like _depth and proposed some changes to get rid of it
• __str__ defaults to __repr__
• honestly I can't remember, probably just suggestions about errors/docs

[A5rocks]

You could try tossing attrs on them and seeing! But I think so too.

Maybe even just try putting __slots__ on them and using that to double check, then removing this (checked) comment.

[jakkdl]

ugh, they're passing locally (but a bunch of other classes aren't 🙃)
and this is running through a minimal tox with -r test-requirements.txt as the only dep so I have no clue what's going on. That's for another day

[jakkdl]

Okay I like the new structure with an ABC, just a couple last things.
A couple methods should probably be renamed to disambiguate _check_xxx methods that set fail_reason and those that don't.

@A5rocks I still can't get rid of the type: ignore, if you want to take a look feel free. But even so I'm not sure it's worth having a bunch of @overloads (if they're necessary) and sending a dummy variable to _check_exceptions just to get rid of a type: ignore/cast.

[jakkdl]

Okay I like the new structure with an ABC, just a couple last things. A couple methods should probably be renamed to disambiguate _check_xxx methods that set fail_reason and those that don't.

@A5rocks I still can't get rid of the type: ignore, if you want to take a look feel free. But even so I'm not sure it's worth having a bunch of @overloads (if they're necessary) and sending a dummy variable to _check_exceptions just to get rid of a type: ignore/cast.

I ... moved the type: ignore to RaisesGroup.__init__ 🙃

[A5rocks]

Looks fine.

I feel like there should be a (polynomial-time) algorithm that tells us whether there's a solution that works for non-greedy matching, but I couldn't think of anything after maybe 30 minutes of staring at examples/counterexamples... Oh well.

(that is, it feels weird that there's no way to go from {a: [1, 2, 3], b: [1], c: [1]} to "there is no way of assigning those" in polynomial time in general)

---

I thought even more about it and actually it makes sense that it's not possible.

[A5rocks]

Yeah it's potentially error worthy given this is a test construct. But even if it is, this error message won't make sense.

[A5rocks]

Maybe this could be fixed by updating the abstract class's __init__. (I'm less and less sure about whether it's a good idea as I come across downsides like this, but oh well. It isn't that bad.)

Namely I think the issue is that AbstractMatcher expects a callable that can take BaseExceptionGroup[BaseExceptionGroup[BaseExcT_2]]...? That's weird.

[jakkdl]

Nah that's not weird, BaseExceptionGroup[BaseExceptionGroup[BaseExcT_2]] comes from the nested case of self: RaisesGroup[..., BaseExceptionGroup[BaseExcT_2]], exception: ... | RaisesGroup[BaseExcT_2]
i.e.

RaisesGroup(
    RaisesGroup(ValueError),
    check=<function that should take BaseExceptionGroup[BaseExceptionGroup[ValueError]]>)

and staring at that I found an issue in the current typing that might be related:

    def handle_value_or_kbi(e: BaseExceptionGroup[ValueError | KeyboardInterrupt]) -> bool:
        return True
    RaisesGroup(ValueError, KeyboardInterrupt, check=handle_value_or_kbi)

error: Value of type variable "ExcT_1" of "RaisesGroup" cannot be "BaseException"
error: Argument "check" to "RaisesGroup" has incompatible type "Callable[[BaseExceptionGroup[Union[ValueError, KeyboardInterrupt]]], bool]"; expected "Optional[Callable[[ExceptionGroup[BaseException]], bool]]"

expecting ExceptionGroup[BaseException] is obviously bonkers, but I think that's downstream of the first error.

[jakkdl]

...except that's only in mypy, not pyright, so might just be an unrelated mypy bug 🙃

[jakkdl]

The error itself is fairly straightforward: (the non-overloaded) RaisesGroup.__init__ defines self as RaisesGroup[ExcT_1 | BaseExcT_1 | BaseExceptionGroup[BaseExcT_2]], and inherits from AbstractMatcher[BaseExceptionGroup[BaseExcT_co]], so AbstractMatcher.__init__(..., check: Callable[[BaseExcT_co], bool] means it expects check to be

Callable[[BaseExceptionGroup[ExcT_1 | BaseExcT_1 | BaseExceptionGroup[BaseExcT_2]], bool]

but then that doesn't match up with the typing of check in RaisesGroup.__init__

        check: (
            Callable[[BaseExceptionGroup[BaseExcT_1]], bool]
            | Callable[[ExceptionGroup[ExcT_1]], bool]
            | None
        ) = None,

(tbh I'm kinda surprised that the check type hint works, given that there's no case for BaseExceptionGroup[BaseExceptionGroup[BaseExcT_2]] - which is why I found the mypy error)

but I suspect the problem ultimately stems from me setting the inheritance as class RaisesGroup[BaseExceptionGroup[BaseExcT_co]] which locks in the BaseExceptionGroup.

Feel free to message me on Gitter if you wanna delve into this further

[jakkdl]

1. The mypy error appears to stem from union-vs-join, so not something we can fix. (The end user can work around it by e.g. changing the type hint of handle_value_or_kbi to be BaseExceptionGroup[BaseException]
2. getting rid of the type: ignore is likely not worth the effort, if it's even theoretically possible.

[A5rocks]

This self._nested is nicer than threading a parameter before, but I think a slight change to the output format would simplify this. From the tests above, I assume this is the effect when nested:

                r"    Matcher\(check=<function test_assert_message_nested.<locals>.<lambda> at .*>\): check did not return True\n"

I assume it would be simpler to support this:

                r"    Matcher\(check=...\): check <function test_assert_message_nested.<locals>.<lambda> at .*> did not return True\n"

In my mind at least, Matcher.__repr__ calls Matcher._repr(show_check=True) and then when printing this nicer reason, we call Matcher._repr(show_check=False).

[jakkdl]

One problem with that one is you would need logic to suppress the printing of check in Matcher.__repr__ (and RaisesGroup.__repr__) only when failing due to the check failing (otherwise you might get issues telling Matcher/RaisesGroups apart if other parameters are the same); and then you'd end up with inconsistent __repr__ outputs depending on the fail reason.

Not saying I love the current logic though. Another option would be to never print the check_repr in _check_check and always add repr(self) to the fail_reason a la

trio/src/trio/_tests/test_testing_raisesgroup.py

Lines 632 to 634 in 561660b

	# TODO: this line is not great, should maybe follow the same format as the other and say
	# 'ValueError': Unexpected nested 'ExceptionGroup' (?)
	" Unexpected nested 'ExceptionGroup', expected bare 'ValueError'\n"

[A5rocks]

I was thinking that instead of doing {whatever!r} you could do {whatever._repr(show_check=False)}