-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix skip bug in which if exception group has all skips it reported error instead of skip #13741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0380548 to
90fba0d
Compare
This PR also contains the commits from #13757, could you fix this please? Thanks!
c8a8a33 to
959a89f
Compare
This PR also contains the commits from #13757, could you fix this please? Thanks!
Done.
Thank you so much for the review, i can only imagine how busy you are with all these reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't think too much about this, just some comments from reading the PR.
9adef4b to
2d37732
Compare
@bluetech Thanks for the review I fixed all the comments
Summary of changes:
- Change test to be more readable by using parametrization with better names and more explicit parameters
- Casting to
Sequence[BaseExceptions]insteadSequence[Skip]which was wrong - Removed redundant comments and changed some comments to ready like sentences
Note: checking if exceptions exist and its insides feels a bit hacky to me using getattr and literal check if excinfo.value.exceptions but I couldn't find a better way of doing this.
open to suggestions, wasn't sure how much more energy should be spent on this since it's such a edge case scenario.
i remember @bluetech mentioned flattening the exception group in #13650 maybe that is the way to go for better clarity and not having to explicitly check the insides of excinfo.value so literally
also so saw a suggestion for TypeGuard but because haven't seen this logic before i the project i hesitated to add it just for this use case
7c9cba0 to
11ad3fb
Compare
...d exceptions in teardown
11ad3fb to
ff07ff6
Compare
@Zac-HD could you have another look see if something else needs doing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - thanks again for the contribution @gomri15, and for your patience!
Added handling for exception group with only skips inside of it to the reporter
fixes issue: #13537