Skip to content

Fix skip bug in which if exception group has all skips it reported error instead of skip#13741

Merged
Zac-HD merged 2 commits into
pytest-dev:mainfrom
gomri15:fix-13537
Oct 28, 2025
Merged

Fix skip bug in which if exception group has all skips it reported error instead of skip#13741
Zac-HD merged 2 commits into
pytest-dev:mainfrom
gomri15:fix-13537

Conversation

@gomri15

@gomri15 gomri15 commented Sep 21, 2025

Copy link
Copy Markdown
Contributor

Added handling for exception group with only skips inside of it to the reporter

fixes issue: #13537

@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Sep 21, 2025
@gomri15
gomri15 force-pushed the fix-13537 branch 2 times, most recently from 0380548 to 90fba0d Compare September 21, 2025 15:59
@nicoddemus

Copy link
Copy Markdown
Member

This PR also contains the commits from #13757, could you fix this please? Thanks!

Comment thread src/_pytest/reports.py Outdated
@gomri15

gomri15 commented Sep 27, 2025

Copy link
Copy Markdown
Contributor Author

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.

@gomri15
gomri15 requested a review from nicoddemus September 27, 2025 17:12

@bluetech bluetech left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/_pytest/reports.py Outdated
Comment thread src/_pytest/reports.py Outdated
Comment thread src/_pytest/reports.py Outdated
Comment thread src/_pytest/reports.py Outdated
Comment thread changelog/13537.bugfix.rst Outdated
@gomri15
gomri15 force-pushed the fix-13537 branch 3 times, most recently from 9adef4b to 2d37732 Compare October 16, 2025 05:44
@gomri15

gomri15 commented Oct 16, 2025

Copy link
Copy Markdown
Contributor Author

@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] instead Sequence[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

@gomri15
gomri15 requested a review from bluetech October 16, 2025 05:51
Comment thread src/_pytest/reports.py Outdated
@gomri15

gomri15 commented Oct 20, 2025

Copy link
Copy Markdown
Contributor Author

@Zac-HD could you have another look see if something else needs doing ?

@Zac-HD Zac-HD left a comment

Copy link
Copy Markdown
Member

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!

@Zac-HD
Zac-HD merged commit e26c0ae into pytest-dev:main Oct 28, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants