-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Separate the various parts of the error report with newlines #11659
Separate the various parts of the error report with newlines #11659
Conversation
Previously the error report would have all sections glued together: - The assertion representation - The error explanation - The full diff This makes it hard to see at a glance where which one starts and ends. One of the representation (dataclasses, tuples, attrs) does display a newlines at the start already. Let's add a newlines before the error explanation and before the full diff, so we get an easier to read report. This has one disadvantage: we get one line less in the least verbose mode, where the output gets truncated.
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 like it! Just one question.
The non-verbose mode is now slightly worse (outside CI), with the output being truncated. Do we want to cater for that case and not add the newline in that case?
It seems OK to me, i.e. no need to special case.
This improvement is part of an issue that already has a changelog entry. Should I merge both or how should the file be named? (I currently added a .2 to it)
You can add it to the previous changelog entry, or you can add a new one. I plan to hand-edit the pytest 8 release notes anyway to make them more coherent so we can merge them then.
r" comparison failed. Mismatched elements: 20 / 20:", | ||
rf" Max absolute difference: {SOME_FLOAT}", | ||
rf" Max relative difference: {SOME_FLOAT}", | ||
r" Index \| Obtained\s+\| Expected", | ||
rf" \(0,\)\s+\| {SOME_FLOAT} \| {SOME_FLOAT} ± {SOME_FLOAT}", | ||
rf" \(1,\)\s+\| {SOME_FLOAT} \| {SOME_FLOAT} ± {SOME_FLOAT}", | ||
rf" \(2,\)\s+\| {SOME_FLOAT} \| {SOME_FLOAT} ± {SOME_FLOAT}...", |
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 assume the ...
here is added by pytest to indicate truncation. If so, shouldn't it be now on the previous line?
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.
Woops good catch. I've updated the test to add it back
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.
Just to make sure, these are literal dots, and the assert is a regex, right? If so, it would be nice to escape the .
s to make it clear and not overmatch: \.\.\.
. And maybe also add anchors ^
and $
.
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.
Good catch, I've made the whole test regexes more restrictive. Mind having another look at that part?
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.
The regex is good 👍
Thanks I added it to the previous one. |
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.
Thanks @BenjaminSchubert, LGTM.
(Note: when a PR is approved you're free to merge, but let's keep this open for a couple of days to let others review if they'd like)
r" comparison failed. Mismatched elements: 20 / 20:", | ||
rf" Max absolute difference: {SOME_FLOAT}", | ||
rf" Max relative difference: {SOME_FLOAT}", | ||
r" Index \| Obtained\s+\| Expected", | ||
rf" \(0,\)\s+\| {SOME_FLOAT} \| {SOME_FLOAT} ± {SOME_FLOAT}", | ||
rf" \(1,\)\s+\| {SOME_FLOAT} \| {SOME_FLOAT} ± {SOME_FLOAT}", | ||
rf" \(2,\)\s+\| {SOME_FLOAT} \| {SOME_FLOAT} ± {SOME_FLOAT}...", |
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.
Just to make sure, these are literal dots, and the assert is a regex, right? If so, it would be nice to escape the .
s to make it clear and not overmatch: \.\.\.
. And maybe also add anchors ^
and $
.
Perfect thanks :) I'll aim to merge this on wednesday unless I get more review by then |
Description
This is part of #11520.
Previously the error report would have all sections glued together:
This makes it hard to see at a glance where which one starts and ends.
One of the representation (dataclasses, tuples, attrs) does display a newlines at the start already.
Let's add a newlines before the error explanation and before the full diff, so we get an easier to read report.
This has one disadvantage: we get one line less in the least verbose mode, where the output gets truncated (outside CI).
Examples
Without pygments installed
Before
After
With pygments installed
Before
After
Questions
.2
to it)