Add RaisesGroup, a helper for catching ExceptionGroups in tests #2898
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2898 +/- ##
========================================
Coverage 99.67% 99.68%
========================================
Files 115 117 +2
Lines 17308 17546 +238
Branches 3109 3152 +43
========================================
+ Hits 17252 17490 +238
Misses 38 38
Partials 18 18
|
|
Failing tests are due to |
wait, I think this is the same thing as a lot of the other weird exceptions in the test - |
|
@TeamSpen210 with RaisesGroup(RaisesGroup(ValueError)) as excinfo:
...
print(type(excinfo.value)) # prints ExceptionGroupso what I kind of want is to be able to search&replace instances of |
|
Ahh, that is tricky, and probably not possible without a plugin or something. |
* Add longer explanation of what the problem with the types are, and expand type tests * Add __new__ to satisfy pyright * replace RaisesGroup.__repr__ with RaisesGroup.expected_type to not lie to repr(), and add support for printing *Base*ExceptionGroup when correct. * add tests for the above
|
huh, why is |
opened an issue in pyright after debugging this. I'm pretty sure we'll get warnings for lots of random symbols if that gets fixed 😅 |
|
Issue in pyright confirmed and will be fixed in the next release, so expect failing tests next time pyright is bumped @A5rocks |
|
The error was in the exceptiongroup backport, not pyright. So wrote a PR to fix it: agronholm/exceptiongroup#101 That's the only remaining failure in the checks, so unless we wanna wait for the docstrings I forgot at home this is ready to merge as soon as exceptiongroup does a new release with my PR. |
| # TODO: print/raise why a match fails, in a way that works properly in nested cases | ||
| # maybe have a list of strings logging failed matches, that __exit__ can | ||
| # recursively step through and print on a failing match. |
There was a problem hiding this comment.
Do we need to do this before merging?
There was a problem hiding this comment.
I'd say it depends on expected usage - if it's a helper that lots of people start using, and/or if it takes a long time until it's supplied by pytest, then it's probably worth chugging through now.
But I don't know how much of it would be applicable to any eventual solution in pytest, or if anybody cares to use this, so I'm hesitant to go all-in on polishing this.
| ExceptionGroup helpers | ||
| ---------------------- | ||
|
|
||
| .. autoclass:: RaisesGroup | ||
| :members: | ||
|
|
||
| .. autoclass:: Matcher | ||
| :members: | ||
|
|
||
| .. autoclass:: trio.testing._raises_group._ExceptionInfo | ||
| :members: |
There was a problem hiding this comment.
These classes should have docstrings aimed at explaining their use to end-users, if we're exposing them publicly.
I'd also prefer to mark this as a provisional interface, and note that we hope for it to be obsoleted by upstream work in pytest itself someday soon. (note: carefully noncommital about what happens or when)
There was a problem hiding this comment.
I wrote extensive docstrings... on a computer I'm currently 500km away from, and forgot to push. So I'll add them once back the 30th of december.
But good call to explicitly mark them as provisional
There was a problem hiding this comment.
Added them, and a note about them being provisional in the newsfragment.
| # this may bite users with type checkers not using pytest, but that should F438 | ||
| # be rare and give quite obvious errors in tests trying to do so. | ||
| # Ignoring "incorrect import of pytest", it makes sense in this situation. | ||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
If the class above is sufficient, why use the pytest version at all?
There was a problem hiding this comment.
pytest.ExceptionInfo has a couple helper methods I didn't bother reproducing - match, getrepr, exconly. And somebody writing tests might very well want to make use of those after catching an ExceptionGroup
There was a problem hiding this comment.
realized that anybody using this without pytest would have zero types on exceptioninfo, so reversed it to always pretend we return _ExceptionInfo and added stubs for the helper methods. But maybe better off to simply always use _ExceptionInfo and disregard the intersection of people that wants to use RaisesGroup + the weird helper methods until it's merged upstream.
| ExceptionGroup helpers | ||
| ---------------------- | ||
|
|
||
| .. autoclass:: RaisesGroup | ||
| :members: | ||
|
|
||
| .. autoclass:: Matcher | ||
| :members: | ||
|
|
||
| .. autoclass:: trio.testing._raises_group._ExceptionInfo | ||
| :members: |
| if TYPE_CHECKING: | ||
| SuperClass = BaseExceptionGroup | ||
| else: | ||
| SuperClass = Generic |
There was a problem hiding this comment.
Why not just always inherit from BaseExceptionGroup?
And I feel like inheriting from that's super fishy given we don't actually have the information BaseExceptionGroup would have. But TBH I don't really see another solution. The typing on this is really icky.
There was a problem hiding this comment.
I gave typing a go and ran across:
import contextlib
from typing import ContextManager, Generic, TypeVar, overload
E = TypeVar("E", bound=BaseException)
class FakeExcInfo(Generic[E]):
value: E
class RaisesGroup(ContextManager[FakeExcInfo[BaseExceptionGroup[E]]], Generic[E]):
@overload
def __init__(self, a: type[E], *b: type[E]):
...
@overload
def __init__(self: RaisesGroup[BaseExceptionGroup[E]], a: RaisesGroup[E],
CEB7
*b: RaisesGroup[E]):
...
def __init__(self: object, *args: object):
...
def __exit__(self, *args: object) -> bool:
...
with RaisesGroup(RaisesGroup(ValueError)) as excinfo:
pass
reveal_type(excinfo.value)it has a couple issues (not accepting both RaisesGroup and normal errors, plus whatever else kind of exceptions we allow) and also mypy doesn't like the overloads idk why but like. maybe that's cleaner?
There was a problem hiding this comment.
For what it's worth, pyright seems to dislike that (probably a bug) but there's an easy workaround for it:
from typing import ContextManager, Generic, TypeVar, overload
E = TypeVar("E", bound=BaseException)
E2 = TypeVar("E2", bound=BaseException)
class FakeExcInfo(Generic[E]):
value: E
class RaisesGroup(ContextManager[FakeExcInfo[BaseExceptionGroup[E]]], Generic[E]):
@overload
def __init__(self, a: type[E], *b: type[E]):
...
@overload
def __init__(self: "RaisesGroup[BaseExceptionGroup[E2]]", a: "RaisesGroup[E2]", *b: "RaisesGroup[E2]"):
...
def __init__(self: object, *args: object):
...
def __exit__(self, *args: object) -> bool:
...
with RaisesGroup(RaisesGroup(ValueError)) as excinfo:
pass
reveal_type(excinfo.value)There was a problem hiding this comment.
Why not just always inherit from
BaseExceptionGroup?
This gives errors due to the funky __new__ stuff that BaseExceptionGroup does.
> with RaisesGroup(ValueError, KeyError):
E TypeError: BaseExceptionGroup.__new__() argument 1 must be str, not type
implementing a __new__ that works like object.__new__:
> obj = object.__new__(cls)
E TypeError: object.__new__(RaisesGroup) is not safe, use BaseExceptionGroup.__new__()
and since we don't actually care for any of the functionality in BaseExceptionGroup, not inheriting at runtime seems clearly correct.
There was a problem hiding this comment.
And I feel like inheriting from that's super fishy given we don't actually have the information BaseExceptionGroup would have. But TBH I don't really see another solution. The typing on this is really icky.
Yeah this is clearly doing stuff that typing wasn't meant to handle.
The underlying problem is getting this to pass typing:
with RaisesGroup(RaisesGroup(ValueError)) as excinfo:
...
myexc: BaseExceptionGroup = excinfo.value.exceptions[0]and the only way I see to manage this is to lie to the type checker that it can treate RaisesGroup like a BaseExceptionGroup.
I'll update comments with reasons for some of the shady stuff. EDIT: this was mostly said in the comment above the lines you marked. Clarified it a bit but not sure I can make it much clearer.
There was a problem hiding this comment.
Ugh I poked a bit more and the E2 thing above that pyright accepts, mypy doesn't. I'm fine with this hacky inheritance thing I guess, hopefully inheritance doesn't fall under semver.
There was a problem hiding this comment.
I wouldn't think so for this particular case, because there would be no need to preform isinstance checks because you would not expect RaisesGroup to be in a list mixed with other objects.
There was a problem hiding this comment.
What did you think of my proposed workaround using overloads btw?
Rest makes sense though.
Oh I didn't trying to run it and failed to grok at a glance how it resolved the issue, sorry! I'll try and understand it and see if I can get it to work, otherwise I'll stick with the ugly inheritance thing.
There was a problem hiding this comment.
The E2 version + pyright doesn't work for me with a third level of nesting
with RaisesGroup(RaisesGroup(RaisesGroup(ValueError))) as excinfo3:
pass
reveal_type(excinfo3.value)
gives
information: Type of "excinfo3.value" is "BaseExceptionGroup[BaseExceptionGroup[Unknown]]"
There was a problem hiding this comment.
but yeah let's stick with the hacky inheritance for now, and we can revisit this if a version of this is accepted for upstream pyright.
Co-authored-by: EXPLOSION <git@helvetica.moe>
…ote about the new classes being provisional in the newsfragment
|
Thanks everybody for the help, reviews, and feedback! 🚀 🚀 🚀 |
pytest does not seem overly excited to add my proposed extensions to
pytest.raisesin a timely manner, although I did get good feedback on the design - see pytest-dev/pytest#11538So I think we might want to settle with having it in trio (or possible in a separate sub-project? catching exceptiongroups is not exclusive to trio users) for some time, so I thought I'd make a proper implementation+tests and rewrite any current tests that can make use of this. This PR is largely to prep for #2886, together with #2891, and splits out some code from it (except this uses my
RaisesGroupimplementation from pytest-dev/pytest#11671 instead ofExpectedExceptionGroupfrom pytest-dev/pytest#11656).TODO: