Add reason for failed match to RaisesGroup #3145
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3145 +/- ##
================================================
Coverage 100.00000% 100.00000%
================================================
Files 124 124
Lines 18460 18733 +273
Branches 1216 1265 +49
================================================
+ Hits 18460 18733 +273
🚀 New features to boost your workflow:
|
….. though somewhat skeptical of their complexity cost vs benefit
… on check fail. Fix output issues when multiple copies of the same object were expected/raised
…. Also fix succesful->successful typo.
…raised-exception by adding another example
…and suggest using Matcher if match would match against a lone expected & raised exception
|
The only remaining TODO now is trio/src/trio/_tests/test_testing_raisesgroup.py Lines 103 to 110 in 9612d1e which I'm procrastinating on because |
|
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. |
…h no matches. Print if there's a possible covering that failed due to too-general expected.
|
Okay I like the new structure with an ABC, just a couple last things. @A5rocks I still can't get rid of the |
I ... moved the |
There was a problem hiding this comment.
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.
src/trio/testing/_raises_group.py
Outdated
| # "Optional[Callable[[BaseExceptionGroup[Union[ExcT_1, BaseExcT_1, BaseExceptionGroup[BaseExcT_2]]]], bool]]" | ||
| # which is kinda obviously off, but idk how to resolve that while also not | ||
| # getting any errors about overloads | ||
| super().__init__(match, check) # type: ignore[arg-type] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
...except that's only in mypy, not pyright, so might just be an unrelated mypy bug 🙃
There was a problem hiding this comment.
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
There was a problem hiding this comment.
- 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_kbito beBaseExceptionGroup[BaseException] - getting rid of the
type: ignoreis likely not worth the effort, if it's even theoretically possible.
There was a problem hiding this comment.
OK I tried several different things but I don't know how to best solve this. I totally agree that it's ultimately an issue with this specific type signature -- I suspect any specific overload signature would be fine w/r/t check's type, but unfortunately we need to satisfy all of them? I'm not totally sure though... (I remember making this __init__ signature through trial and error so it's not like it makes sense to begin with)
* Indent flatten_subgroups and re.escape suggestions. * Don't call expected `Exceptiongroup` "unexpected nested". * Use repr_callable so tests dont fail if hypothesis imported. * small polish to typing tests
repr_callable in __repr__s.
There was a problem hiding this comment.
The typing issues are a bit too much for me! To follow up on my last comment, I did actually end up thinking more about this and I think there's an algorithm we could follow that could have better properties (detailed in a drop down because a) I'm not sure it actually works, b) it's mostly just a "hey maybe consider this as a new PR"):
extra rambling that might not even work
I'll focus purely on mechanics cause like, I don't even know if it works let alone has better properties.
Firstly, sort the actual exceptions based on subtyping relationship w/ a topological sort: for instance if BaseException, Exception, and ValueError get raised, then [BaseException, Exception, ValueError].
Next, apply matchers in left to right order, on the actual exceptions.
Topological sort the expected types too. Then just do what we're already doing with using the first match.
Then you should have no actual and no expected.
Cases this fails on:
- matchers that are overly broad
- exceptions/types that are circular in terms of
issubclass/isinstance.
On the other hand I think it should always handle a case without any matchers and ordering does not matter -- matcher quality is all people need to care about. Error notes just becomes "here's what the matchers matched".
src/trio/testing/_raises_group.py
Outdated
| # "Optional[Callable[[BaseExceptionGroup[Union[ExcT_1, BaseExcT_1, BaseExceptionGroup[BaseExcT_2]]]], bool]]" | ||
| # which is kinda obviously off, but idk how to resolve that while also not | ||
| # getting any errors about overloads | ||
| super().__init__(match, check) # type: ignore[arg-type] |
There was a problem hiding this comment.
OK I tried several different things but I don't know how to best solve this. I totally agree that it's ultimately an issue with this specific type signature -- I suspect any specific overload signature would be fine w/r/t check's type, but unfortunately we need to satisfy all of them? I'm not totally sure though... (I remember making this __init__ signature through trial and error so it's not like it makes sense to begin with)
I don't think improving the "see-if-there's-a-full-match" algorithm is worth the effort/complexity. It currently only triggers on failing test cases, and unless people start expecting groups with like ... hundreds? thousands? of exceptions, I'm pretty sure the runtime cost of executing all the other logic will dwarf the O(n²) complexity of the "see-if-there's-a-full-match" check. It might be worth it if we switched from greedy matching to best-effort in the standard case, but:
|
|
Working on getting this into pytest, and they run with |
As another preparatory step for adding
RaisesGroupinto 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 usingRaisesGroupwith 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, likepytest.raises, would silently pass through the exception if it didn't match - but now it will instead fail with anAssertionError. This also has precedent upstream though,pytest.raiseswill fail with anAssertionErroriff you've specifiedmatch.You still see the original exception in the traceback, and in many ways I think always failing with an
AssertionErroris 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:
Another improvement would be making.matches()directly raise anAssertionError, instead of quietly setting.fail_reason. This would break any test currently doingif not RaisesGroup(...).matches(...):, or even more plausiblyassert not RaisesGroup(...).matches(...), soI think I should add a new method
.assert_matches()to bothRaisesGroupandMatcher, which either calls.matches()and asserts the return value withfail_reasonas the message - or do it the other way around and have.matches()call.assert_matches()in atry:.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.