Skip to content
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

add RaisesGroup & Matcher #13192

Merged
merged 26 commits into from
Mar 5, 2025
Merged

add RaisesGroup & Matcher #13192

merged 26 commits into from
Mar 5, 2025

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Feb 4, 2025

Previous PR's: #11656 (ExpectedExceptionGroup), #11671 (closed to give this a fresh PR w/o stale comments)
fixes: #11538 #12504

This adds RaisesGroup (also exported as raises_group) and Matcher, to allow a robust way of expecting ExceptionGroup and the ability to specify the structure of nested groups, etc etc.

with RaisesGroups(ValueError):
    raise ExceptionGroup("", (ValueError(),))

This is the exact implementation available in trio.testing.RaisesGroup which has been in use since december 2023 and gone through a couple iterations since to settle on a good interface. The most recent addition was python-trio/trio#3145 which added error messages, where the formatting can probably still be fine-tuned.

Pytest currently has excinfo.group_contains for checking exceptiongroups... which is very problematic. I think it should be deprecated, either in the same release as this PR, or very soon after.

This currently does not touch the implementation of pytest.raises, but if we made Matcher a contextmanager we could easily support

with Matcher(ValueError):
    raise ValueError()

this would fix e.g. #12763 with Matcher having the check parameter. The only complication is the legacy form of pytest.raises but that should be solvable with a small helper function.
This can however be done in a followup PR.

This currently does not add an .assert_matches method, which previous iterations of the PR had. The reason to add that would be to improve the error message on failure, though it's possible to get the same result with

assert (m := Matcher(TypeError)).matches(e), m.fail_reason
# versus
Matcher(TypeError).assert_matches(e)

though note that .matches() is already a fairly niche feature that is entirely separate to the context-manager use (not to be confused with the match parameter).

TODO:

  • bump the issue # on the newsfragment file
  • documentation

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Feb 4, 2025


@final
class Matcher(AbstractMatcher[MatchE]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We ought not to use such a generic name easily

There's a general wishlist plan to enable matchers as a whole that act similar to dirty equals

The matcher api that's being used here conflates multiple aspects of that in a manner that's problematic to partition later

Ideally the whole api around matchers is first evolved as a plugin to avoid early api lock in as it's hard to make good matchers

I'll elaborate further when the flu is over

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I can rename it... maybe something that's closer to RaisesGroup... RaisesExc/RaisesException? or something like ExpectedExc/ExpectedException

This has been available & in use for >1 year in trio, which has allowed the interface to mature. I don't think pytest needs to repeat that process by putting it in a plugin.

I know you've mentioned matcher objects in #11538 and I'm excited for what that might entail in the future, but I don't think we want to wait for that to add usable exceptiongroup support. If you have concrete ways of improving the RaisesGroup interface to harmonize with it I'm of course open to suggestions though.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been heavily involved in the design and review for this feature as we've developed and shipped it in Trio. I'm excited to get it into Pytest (and ideally soonish), but I shouldn't be the only approving core dev for a substantial feature.

…s on RaisesGroup&RaisesExc. Add warnings to group_contains. Remove group_contains example from getting-started page
@jakkdl jakkdl linked an issue Feb 6, 2025 that may be closed by this pull request
@jakkdl
Copy link
Member Author

jakkdl commented Feb 6, 2025

@Zac-HD please review the documentation :)

It feels a bit silly to add big warning boxes to ExceptionInfo.group_contains and not deprecate it, but it also feels silly to deprecate it one release after adding it.

@jakkdl jakkdl requested a review from Zac-HD February 6, 2025 14:42
@Zac-HD
Copy link
Member

Zac-HD commented Feb 10, 2025

@Tusenka unfortunately pull requests are the wrong place to ask questions like this, unless you're either the author of the PR or a maintainer of the package. I suggest StackOverflow instead.



@final
class RaisesExc(AbstractRaises[BaseExcT_co_default]):
Copy link
Member Author

@jakkdl jakkdl Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please bring basic questions like these to StackOverFlow, the python discord, ask an LLM of your choice, or otherwise. This is not relevant to the PR at hand and only wastes the time of maintainers without any benefit to pytest.



@final
class RaisesExc(AbstractRaises[BaseExcT_co_default]):

This comment was marked as off-topic.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 17, 2025

I think I'm mostly just waiting for a quick pass on docs changes from @Zac-HD (or others), and otherwise this can be merged and I can look at potential followup for pytest.raises and/or deprecating excinfo.group_contains

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @jakkdl for the great work here!

I'm OK with us having pytest.RaisesGroup and it being different than pytest.raises, you have made your points and since nobody else has given their opinions, I guess it is no big deal in the end.

However I think we should reconsider also exposing pytest.raises_group, as having both ways to do the exact same thing might not be a net positive? So I propose just going ahead and having only pytest.RaisesGroup, leaving out the pytest.raises_group alias.

@nicoddemus
Copy link
Member

nicoddemus commented Mar 5, 2025

Hi @jakkdl, thanks for sticking with it so far!

Can you just fix the conflict please? Then I will be happy to merge. 👍

@jakkdl
Copy link
Member Author

jakkdl commented Mar 5, 2025

lel, amusing how big a conflict you can get from a simple be2600e

I'll merge before it happens again! 🎉 🚀 🎉 🚀

@jakkdl jakkdl enabled auto-merge March 5, 2025 15:37
@jakkdl jakkdl linked an issue Mar 5, 2025 that may be closed by this pull request
@jakkdl jakkdl merged commit 150cac8 into pytest-dev:main Mar 5, 2025
28 checks passed
@jakkdl
Copy link
Member Author

jakkdl commented Mar 5, 2025

ugh I just noticed this didn't quite have 100% patch coverage. We should enforce that in config

@nicoddemus
Copy link
Member

nicoddemus commented Mar 5, 2025

Thanks @jakkdl.

In the future, please consider squashing the commits before merging, as this PR introduced a lot of commits which don't make sense separately.

See https://github.com/pytest-dev/pytest/blob/main/CONTRIBUTING.rst#mergesquash-guidelines. 👍

@jakkdl
Copy link
Member Author

jakkdl commented Mar 5, 2025

oh shit, I'm way too used to squashing being the default

do you want me to squash them on master (if I can?), or will you do that?

@RonnyPfannschmidt
Copy link
Member

It's already on main and main is protected

@nicoddemus
Copy link
Member

Yeah, unfortunately now it has landed on main so we cannot change it, as a Git good practice.

Not the end of the world, but something to take care in the future. Thanks again!

@jakkdl jakkdl deleted the raisesgroup branch March 6, 2025 12:16
jakkdl added a commit to jakkdl/pytest that referenced this pull request Mar 7, 2025
* renames src/_pytest/raises_group.py to src/_pytest/raises.py
* moves pytest.raises from src/_pytest/python_api.py to src/_pytest/raises.py
* adds several newsfragments that should've been bundlen with
* add RaisesGroup & Matcher pytest-dev#13192
* add more detailed error message if you try to do RaisesGroup((ValueError, TypeError))
* mess around with ValueError vs TypeError on invalid expected exception
jakkdl added a commit to jakkdl/pytest that referenced this pull request Mar 10, 2025
* renames src/_pytest/raises_group.py to src/_pytest/raises.py
* moves pytest.raises from src/_pytest/python_api.py to src/_pytest/raises.py
* adds several newsfragments that should've been bundlen with
* add RaisesGroup & Matcher pytest-dev#13192
* add more detailed error message if you try to do RaisesGroup((ValueError, TypeError))
* mess around with ValueError vs TypeError on invalid expected exception
expected_exception: (
type[BaseExcT_co_default] | tuple[type[BaseExcT_co_default], ...]
),
match: str | Pattern[str] | None = ...,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make match and check kw-only arguments (also in the real __init__ below)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in af1c472 in #13279

func = args[0]
if not callable(func):
raise TypeError(f"{func!r} object (type: {type(func)}) must be callable")
with RaisesExc(expected_exception) as excinfo:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just noted that we have a important dissonance - the raises contextmanager is reentreant, the RaisesExc one isnt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to a failed match now raising AssertionError instead of letting the original exception through unchanged (reverted in #13279 (comment)), or something else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does fail:

    r = pytest.raises(ValueError)
    with r:
        with r:
            raise ValueError
        raise ValueError

but that was an error previously as well.

For reuse this does work:

    r = pytest.raises(ValueError)
    with r:
        raise ValueError
    with pytest.raises(Failed):
        with r:
            ...
    with r:
        raise ValueError

probably worth adding tests for those

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to the state of the contextmanager instances

Copy link
Member Author

@jakkdl jakkdl Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes there's state - but afaik there's no relevant state that's not reset upon doing a new __enter__ or matches() call. Do you have an example that demonstrates the change in behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jakkdl added a commit that referenced this pull request Mar 29, 2025
* raisesgroup followups
* renames src/_pytest/raises_group.py to src/_pytest/raises.py
* moves pytest.raises from src/_pytest/python_api.py to src/_pytest/raises.py
* adds several newsfragments that should've been bundled with #13192
* add more detailed error message if you try to do RaisesGroup((ValueError, TypeError))
* mess around with ValueError vs TypeError on invalid expected exception
* revert change in behaviour if raises has a type mismatch
* add check example to raises, fix test after behaviour revert
* made args to AbstractMatcher, RaisesExc and RaisesGroup pos/kw-only

---------

Co-authored-by: Ran Benita <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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
6 participants