Add support to testing.RaisesGroup for catching unwrapped exceptions #2989
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2989 +/- ##
=======================================
Coverage 99.63% 99.63%
=======================================
Files 120 120
Lines 17710 17798 +88
Branches 3179 3198 +19
=======================================
+ Hits 17645 17733 +88
Misses 46 46
Partials 19 19
|
|
|
|
@TeamSpen210 after adding I'm a bit surprised as covariant typevars exist in a few other places in the codebase without issue. Unless you have any opinions I'll try do some ugly workaround where I either add a plussed [copy] to the dicts in EDIT: nope, wasn't that easy to work around... |
|
I can make a theoretical case for splitting # they want this to pass
with RaisesGroup(ValueError, strict=False):
raise ExceptionGroup("outer", [ExceptionGroup("inner", [ValueError()])])
# but this to fail
with RaisesGroup(ValueError, strict=False):
raise ValueErrorOf course they can just do with RaisesGroup(..., strict=False) as e:
...
assert isinstance(e, ExceptionGroup)but that's easy to forget and could silently introduce changes in behavior. One, probably better, case in favor of splitting out Since typing |
|
Fixed the typevar-related issues, but now there's something else in |
Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
…g where length check would fail incorrectly sometimes if using flatten_subgroups
|
Seems like nobody else had opinions on |
jakkdl
changed the title
The missing end specifiers are intended because I didn't bother including the full error message. I fixed it so >>> str(ExceptionGroup('foo', [ValueError()]))
'foo (1 sub-exception)' |
src/trio/testing/_raises_group.py
Outdated
| warnings.warn( | ||
| DeprecationWarning( | ||
| "`strict=False` has been replaced with `flatten_subgroups=True`" | ||
| " with the introduction of `allow_unwrapped` as a parameter." | ||
| ), | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Why aren't we using our deprecation utils in this case? Cause those would have the version this was deprecated in which is useful information.
There was a problem hiding this comment.
(If it's because they don't warn with something derived from DeprecationWarning that makes sense. Maybe add a kwarg to warn_deprecated?)
There was a problem hiding this comment.
mostly just me being confused about the existence of TrioDeprecationWarning (hence #2992). One weird part with warn_deprecated is you have to know what the next version bump will be, but I'm guessing I should mark it as 0.25.1.
But I also don't think this one is worthy of an official extended period of supporting it, it's a trivial change to use flatten_subgroups=True instead and this is explicitly a temporary helper until it's merged into pytest, so it's barely worth having a DeprecationWarning at all. It's not used at runtime either, so it can only break tests. But I guess TrioDeprecationWarning doesn't have an explicit promise of how long it will be kept deprecated anyhow
|
oh sorry for the re-request, didn't get notified about your review |
fixed everything now though, so re-review request is warranted |
|
I encountered some issues with typing for functions passed to the |
| match: None = None, | ||
| check: None = None, |
There was a problem hiding this comment.
| match: None = None, | |
| check: None = None, |
I don't think it's necessary to provide these. a) we're adding the param now so no backcompat issues and b) I don't think we should care about backcompat for types because they're just hints.
There was a problem hiding this comment.
Hmm, I think it doesn't hurt to have them, and allows users to do some automated stuff where they pass variables that they know to be None:
for allow_unwrapped,match in zip([True, None], [False, "hello"]):
with RaisesGroup(Exception, allow_unwrapped=allow_unwrapped, match=match):
...
|
Also re: your mypy bug, my impression was that it just ignores |
scrolling through https://github.com/python/mypy/blob/master/test-data/unit/check-classes.test I guess that is the current state of things with mypy. But after sleeping on it I think I have a better way of resolving it - but that's for a new PR. It's finally time to merge this one 🚀 |
strictparameter, renaming it toflatten_subgroups.allow_unwrappedparameter, adding the possibility to more closely mirrorexcept*. This is a feature requested in Allow pytest.raises cooperate with ExceptionGroups pytest-dev/pytest#11538 (comment) by @belm0would fail due to it checking if the number of exceptions was correct before flattening the structure of the raised exceptiongroup.
I also found an unrelated issue where mypy&pyright are not able to deduce the type when passing multipleMatcherobjects matching against different exceptions toRaisesGroup. (orMatcher+class, but two classes work). Fiddled around a little bit but wasn't able to quickly find a fix, and explicitly setting[Exception]isn't especially onerous.EDIT: this was due to
TypeVarlacking covariance.