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

Enhance errors for exception/warnings matching #8508

Merged
merged 4 commits into from Mar 21, 2022

Conversation

RonnyPfannschmidt
Copy link
Member

No description provided.

@RonnyPfannschmidt RonnyPfannschmidt changed the title wip: enhance errors of some internal utilities enhance errors of some internal utilities Jun 7, 2021
Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Might want to use f-strings instead of str.format everywhere?

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Oh, also, why the extra (unneeded) assert before excinfo.match?

@RonnyPfannschmidt
Copy link
Member Author

@The-Compiler interesting, pycharm seems to have added those i didn't even note, going to take them off

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, thanks!

We might want to add a small changelog entry 8508.improvement.rst:

Improve :func:`pytest.raises` and :func:`pytest.warns` error messages.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

LGTM, just a (preexisting) grammar fix and a suggestion.

src/_pytest/recwarn.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
msg += " Did you mean to `re.escape()` the regex?"
assert re.search(regexp, str(self.value)), msg.format(regexp, str(self.value))
value = str(self.value)
msg = f"Regex pattern did not match.\n Regex: {regexp!r}\n Input: {value!r}"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it wouldn't be better to use a straight str here instead of repr. With the new line format the quoting doesn't seem needed, perhaps the opposite.

(Either way is fine with me)

Copy link
Member Author

Choose a reason for hiding this comment

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

i believe for the regex it helps to get the escapes printed more nicely readable, my personal opinion is that repr reads slightly better than just the str or the escape

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, this will show as

>>> regexp = r'py\.test'
>>> value = 'pytest'
>>> msg = f"Regex pattern did not match.\n Regex: {regexp!r}\n Input: {value!r}"
>>> print(msg)
Regex pattern did not match.
 Regex: 'py\\.test'
 Input: 'pytest'

I would have preferred py\.test over py\\.test.

Copy link
Member Author

Choose a reason for hiding this comment

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

i just experimented with this - if there is a simple way to print "raw" strings, i'll happily use that - however even in the pytest testsuite there is a number of cases that would turn confusing without

Copy link
Member

Choose a reason for hiding this comment

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

if there is a simple way to print "raw" strings, i'll happily use that

That would be {regexp} instead of {regexp!r}, unless I'm misunderstanding.

however even in the pytest testsuite there is a number of cases that would turn confusing without

Can you give an example?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm somewhat undecided here. I agree the double backslashes are a bit confusing, but at the same time I think it's good to print the repr for the input (considering that it can contain whitespace and such, and seeing that whitespace might be essential to find out why the pattern doesn't match).

But then if we use the repr for the input, it might be more consistent and thus less confusing if we use the repr for the pattern as well.

changelog/8508.improvement.rst Outdated Show resolved Hide resolved
changelog/8508.improvement.rst Outdated Show resolved Hide resolved
@RonnyPfannschmidt
Copy link
Member Author

@The-Compiler @nicoddemus i believe this is complete niw, please have another look

@RonnyPfannschmidt RonnyPfannschmidt changed the title enhance errors of some internal utilities enhance errors for exception/warnings matching Oct 11, 2021
@nicoddemus nicoddemus self-requested a review October 11, 2021 12:27
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.

Looks good, thanks!

src/_pytest/recwarn.py Show resolved Hide resolved
changelog/8508.improvement.rst Show resolved Hide resolved
msg += " Did you mean to `re.escape()` the regex?"
assert re.search(regexp, str(self.value)), msg.format(regexp, str(self.value))
value = str(self.value)
msg = f"Regex pattern did not match.\n Regex: {regexp!r}\n Input: {value!r}"
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, this will show as

>>> regexp = r'py\.test'
>>> value = 'pytest'
>>> msg = f"Regex pattern did not match.\n Regex: {regexp!r}\n Input: {value!r}"
>>> print(msg)
Regex pattern did not match.
 Regex: 'py\\.test'
 Input: 'pytest'

I would have preferred py\.test over py\\.test.

src/_pytest/recwarn.py Outdated Show resolved Hide resolved
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Couple things I noticed about the warns error message. They are preexisting so not meant to block this PR, just if you feel like it.

testing/test_recwarn.py Outdated Show resolved Hide resolved
testing/test_recwarn.py Show resolved Hide resolved
* use f-strings more
* use pformat to ensure multi line print for longer-warning lists
  while keeping short ones small
* now multiline and compares regexes
* warns on forgotten escaping
Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Some nice cleanups and output improvements! I added my $0.02 to a couple of things. Nothing blocking, though.

msg += " Did you mean to `re.escape()` the regex?"
assert re.search(regexp, str(self.value)), msg.format(regexp, str(self.value))
value = str(self.value)
msg = f"Regex pattern did not match.\n Regex: {regexp!r}\n Input: {value!r}"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm somewhat undecided here. I agree the double backslashes are a bit confusing, but at the same time I think it's good to print the repr for the input (considering that it can contain whitespace and such, and seeing that whitespace might be essential to find out why the pattern doesn't match).

But then if we use the repr for the input, it might be more consistent and thus less confusing if we use the repr for the pattern as well.

src/_pytest/recwarn.py Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
Co-authored-by: Florian Bruhin <me@the-compiler.org>
@nicoddemus nicoddemus changed the title enhance errors for exception/warnings matching Enhance errors for exception/warnings matching Mar 21, 2022
@nicoddemus nicoddemus merged commit e9dd3df into pytest-dev:main Mar 21, 2022
25 checks passed
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.

None yet

4 participants