-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
add RaisesGroup & Matcher #13192
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
Merged
Merged
add RaisesGroup & Matcher #13192
Changes from 6 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
c93130c
add RaisesGroup & Matcher
jakkdl 4737c8c
add AbstractMatcher support to xfail
jakkdl e1e1874
rename AbstractMatcher -> AbstractRaises, Matcher->RaisesExc. Add doc…
jakkdl e090517
Merge branch 'main' into raisesgroup
jakkdl e73c411
fix test on py<311
jakkdl c011e9b
fix test, fix references in docstrings
jakkdl 426fe19
Apply suggestions from code review
jakkdl 7f9966b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 0cdc5da
Update src/_pytest/_raises_group.py
jakkdl cb30674
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 9714dc0
doc improvements after review
jakkdl ad6542e
Merge remote-tracking branch 'origin/main' into raisesgroup
jakkdl 4d2c709
fix imports after file rename
jakkdl 9e38a9e
fix another import
jakkdl ff9dd38
sed s/RaisesGroups/RaisesGroup
jakkdl 2c8cd64
make pytest.raises use RaisesExc... which made me notice about a mill…
jakkdl 09d06fe
fix tests
jakkdl 753df94
harmonize stringify_exception, various comments
jakkdl 4f682c1
fix rtd
jakkdl edfcc86
Merge branch 'main' into raisesgroup
jakkdl 309030c
fix import loop
jakkdl 4e97652
remove raises_group alias, doc fixes after review
jakkdl 5186f36
Merge remote-tracking branch 'origin/main' into raisesgroup
jakkdl 9163167
Merge remote-tracking branch 'origin/main' into raisesgroup
jakkdl d37e6d6
Update 11538.feature.rst
jakkdl 4c6ded7
docs fixes after removing the raises_group alias.
jakkdl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Added :class:`pytest.RaisesGroup` (also export as ``pytest.raises_group``) and :class:`pytest.RaisesExc`, as an equivalent to :func:`pytest.raises` for expecting :exc:`ExceptionGroup`. It includes the ability to specify multiple different expected exceptions, the structure of nested exception groups, and flags for emulating :ref:`except* <except_star>`. See :ref:`assert-matching-exception-groups` and docstrings for more information. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| :func:`pytest.mark.xfail` now accepts :class:`pytest.RaisesGroup` for the ``raises`` parameter when you expect an exception group. You can also pass a :class:`pytest.RaisesExc` if you e.g. want to make use of the ``check`` parameter. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Perhaps it would be best to not export
RaisesGroupat all, keeping onlypytest.raises_groupas the official interface?pytest.raises_groupharmonizes well with the existingpytest.raises, plus I don't see any benefit of having two ways of doing the same thing. Another argument for that is that we do not expose the context manager used inpytest.raiseseither, so I don't think we should do the same here.I would make the
pytest.raises_groupthe first-class citizen in the API, we can keepRaisesGroupin the docs if we like, but I would make all examples usepytest.raises_grouponly.@The-Compiler might want to comment on that given he does a lot of pytest training.
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.
yeah only having a single name makes sense, but IMO it behaves so much like a class, and will get used a lot like a class, that the snake_case looks very weird. And surely
RaisesGroupandRaisesExcshould be the same? butraises_excis then close-to-identically-named toraiseswhich means it should be renamed, or they should be harmonized.I think these examples look weird, but maybe that's just me?
I guess one might frame it as
raises_groupis a factory for creatingRaisesGroup.. but then the user will assume there's a distinction and a reason for that distinction.What I'm afraid might happen is people thinking that they need to do
especially if the type hints specify
RaisesGroup... which means that maybe the class itself should be named in snake_case??? Ew. Or people just not considering the possibility that it does a lot more thanpytest.raiseshas historically supported.In my very personal opinion the ideal would be having
pytest.RaisesGroup&pytest.RaisesExc, andpytest.raisesbeing a thin wrapper aroundpytest.RaisesExcto add support for the legacy calling mode. But if I'm in the minority here then /shrugThere 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.
Wait sorry I only now properly parsed this. I don't think you should pattern-match this beast with
pytest.raises, it's perhaps an overly engineered beast - butRaisesGroupis way more than just "the context manager" of a function call. It has public attributes infail_reason&matchesthat are meant to be used - and it even supports modifying any of the parameters passed to__init__after creation (though that bypasses the verification in__init__so it is a bit dangerous).Uh oh!
There was an error while loading. Please reload this page.
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.
Well at least this is consistent with
pytest.raises, which is also just a factory. I personally find it fine to havepytest.raises_groupa factory, and a separateRaisesExcas a class, given their usage is different anyway.As an additional data point, we have
pytest.paramwhich is similar in purpose toRaisesExc, so perhaps this is another argument forpytest.raises_exc.But I get your points, but I'm sure at least some users will wonder why there is
pytest.raisesandpytest.RaisesGroup.But if we move forward with having
pytest.RaisesGroupI would remove thepytest.raises_groupalias, I don't see much benefit of having the alias in place.But I'm not deadset on this either, just find the inconsistency a bit unnerving and might something to come up for new users. 👍
As I mentioned before, this is excellent work overall, I'm bringing this up because user facing API is important and is something we have overlooked in the past (regarding consistency I mean).
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.
I don't think
RaisesGroupandRaisesExchas significant differences in their use, the only difference is thatRaisesExcdoes not support the context-manager use - but I kind of think it should and thatRaisesContextshould be removed. I've refrained from bundling that as yet another part of this PR, but I'm starting to think I should.pytest.paramis a good data point. And maybe the harmonization ends up with not needing a distinction betweenraises_excandraises, in which case we getI'll give it a go, and if the details on that becomes worthy of extensive review&discussion I'll split it off into another PR.
I'm very happy to get feedback! I've spent an ungodly amount of time on this feature, so I do have very strong personal opinions at this point, but hopefully I can soon let it go into the collective consciousness~
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.
Do you mean:
pytest/src/_pytest/python_api.py
Line 1030 in b0caf3d
Or is that a typo?
But before moving on, I would recommend to wait a few days for other opinions -- it is possible people will say "I agree with @jakkdl and @nicoddemus is being too cautious, this will not be a problem in practice" and we can just move forward, hehehe.
Uh oh!
There was an error while loading. Please reload this page.
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.
no, not a typo. The logic in
RaisesContextis a ~strict subset of the logic inRaisesGroup, so if I move__enter__and__exit__toAbstractMatcherI can makepytest.raisesreturnRaisesExc(and also offload some input validation frompytest.raisestoRaisesExc).That also adds support for e.g.
pytest.raises(check=lambda e: isinstance(e.__cause__, ValueError)). #12763