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

Have pytest.raises match against exception __notes__ #11227

Merged
merged 12 commits into from Jul 18, 2023

Conversation

ivirshup
Copy link
Contributor

@ivirshup ivirshup commented Jul 17, 2023

Closes #11223

Adds support for matching against __notes__ added to an exception within pytest.raises. See issue for discussion/ design.

Notes

TODO

  • Add yourself to AUTHORS in alphabetical
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst.

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.

Looks good to me, thanks again Isaac! Once those todos are done I'd be happy to merge.

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.

LGTM, left some simple suggestions.

@@ -843,6 +843,13 @@ def raises( # noqa: F811
>>> with pytest.raises(ValueError, match=r'must be \d+$'):
... raise ValueError("value must be 42")

The ``match`` argument searches the formatted exception string, which includes any ``__notes__``:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would be nice to add a link to the relevant PEP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

testing/code/test_excinfo.py Outdated Show resolved Hide resolved
testing/code/test_excinfo.py Outdated Show resolved Hide resolved
@ivirshup ivirshup marked this pull request as ready for review July 18, 2023 09:49
@ivirshup
Copy link
Contributor Author

Thanks for the quick review! Addressed comments, finished up, and added a couple more test cases.

@@ -704,7 +704,12 @@ def match(self, regexp: Union[str, Pattern[str]]) -> "Literal[True]":
If it matches `True` is returned, otherwise an `AssertionError` is raised.
"""
__tracebackhide__ = True
value = str(self.value)
value = "\n".join(
Copy link
Member

Choose a reason for hiding this comment

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

We might want to work with @agronholm to ensure note helpers in the backport as well as shared formatting as follow up

Choose a reason for hiding this comment

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

The tests here don't seem to involve exception groups though.

Copy link
Member

Choose a reason for hiding this comment

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

@agronholm my impression is that exception notes are closely related and my understanding is that integration with the backport may be helpful as per agronholm/exceptiongroup#31 and since formatting may also be affected my impression is that the most benefit for library users would come from having one integration place instead of multiple

Copy link
Contributor Author

@ivirshup ivirshup Jul 18, 2023

Choose a reason for hiding this comment

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

About formatting: I don't think there is a function in traceback that provides the exact rendering pytest expects. I think all the methods that traceback provides will include the exception type.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Im happy now that the design decision is done and we will use match all

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.

Nice, thanks @ivirshup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support __notes__ in pytest.raises
5 participants