-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
jest-snapshot: Display change counts in annotation lines #8982
jest-snapshot: Display change counts in annotation lines #8982
Conversation
In the following 5 pictures, the only difference is the change counts The changes are insertions in the received value: The changes are replacements: This comparison will improve in future pull request to ignore indentation: Some changed lines have substring differences: Unlike other assertions, snapshots do not “know” the expected type: |
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.
Code looks solid to me. Not 100% sold on the way it's displayed and want opinions.
+ Received 1 + | ||
|
||
- bar | ||
+ foo |
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.
For this case, where it's a string that was 1 line and will again be 1 line, this information is pretty useless. Can we not display it?
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.
Yes, I will deal with comparison of two one-line strings as a special case for the concise labeled format, because it seems likely for toThrowErrorMatchingSnapshot
assertions.
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.
Maybe with a clearer labeling (other comment thread) it's actually fine and worth keeping for consistency?
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.
Tim, good point about prioritizing consistency higher than a more concise special case.
If most reports have differences when snapshot assertions fail, and if next pull request changes the colors to be less confusing, some people might learn to scan quickly which console output is from snapshots versus other assertions, and more intuitively choose which decision path.
For example, the e2e
tests for this pull request had 3 snapshots to update and 1 toMatch
assertion which I needed to rewrite as 4 assertions.
To test this theory: think fast, what do you need to do for each of the following:
Expected: "bar"
Received: "foo"
Snapshot: "bar"
Received: "foo"
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 think I know what you mean, if there's lots of (correct) test failures I usually try to change non-snapshot tests first and then update snapshots once they're the only things still failing. Not sure how that's related to this comment thread but I'd definitely appreciate making it easier to tell them apart.
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.
See pictures in #8982 (comment) of concise label format if neither string is multiline
packages/jest-snapshot/src/__tests__/__snapshots__/printDiffOrStringified.test.ts.snap
Outdated
Show resolved
Hide resolved
For default serialization in concise format, improved does not unescape string differences: See if you can interpret the meaning For default string serialization, keep the concise label format: The improved picture at the right is possible with custom raw string serialization: Because the received is multiline string, display as comparison lines: For default string serialization, keep the concise label format: The improved picture at the right is possible with custom raw string serialization: |
In the following 3 pictures of edge cases, improved does not unescape quoted strings: baseline code never unescaped backslashes and improved does not in object serialization: baseline (above) always unescaped double quotes but improved (below) doesn’t in object serialization: do not display differences for a few received types like number the expected snapshot string |
Codecov Report
@@ Coverage Diff @@
## master #8982 +/- ##
==========================================
+ Coverage 63.9% 64.05% +0.15%
==========================================
Files 277 277
Lines 11652 11676 +24
Branches 2860 2863 +3
==========================================
+ Hits 7446 7479 +33
+ Misses 3576 3572 -4
+ Partials 630 625 -5
Continue to review full report at Codecov.
|
If neither quoted string is multiline, then restore a previous improvement to display with labels: Small change suggests either update after clean up message or fragile test (for example, v8 versus Gecko) so replace Either regression or change if received has no common substring with expected: Reasons why not to display as unescaped strings without double quote marks:
Possible regression if received message is empty string: Rare edge case for expected snapshot to be empty string: However, for custom raw string serialization, always display as diff comparison lines. |
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.
@scotthovestadt wanna take a look again to see if you like it in the current state?
And what do y’all think about a follow-up pull request to display change counts for other assertions? |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
The number of change lines helps to interpret differences when a snapshot assertion fails:
Similar to benefit of summary report following a test run, except these precede the comparison.
In addition to the
includeChangeCounts
option from #8881 here are supporting changes:Call functions directly from
jest-diff
instead of indirectly viajest-matcher-utils
as prerequisite for color options in a future pull request, and also swap where two snapshot transformations are called:removeExtraLineBreaks
from_toMatchSnapshot
function tomatch
method, because it is a private detail of snapshot formattingunescape
function frommatch
method toprintDiffOrStringified
function, because its use depends on how the report displays the snapshot (see 2)Adjust changes in Unescape serialized snapshots for diffing #2852 to unescape double quote marks and also backslashes but only when the report displays string differences, and if both of the following:
With change counts as a clue to interpret edge cases when the snapshot or received is empty string and its counterpart is multiline string, display as comparison lines instead of labeled lines; which reverses a previous decision:
Next steps
What do y’all think about a follow-up pull request to display change counts in differences when
toBe
,toEqual
,toStrictEqual
,toHaveProperty
,toMatchObject
assertions fail?Change confusing colors in snapshot differences, but not exactly as requested in Diff colors should show green for insert, red for delete #5430
Ignore indentation in serialization of received objects and by heuristic in snapshots to remove distracting differences, see Optionally hide whitespace changes in diff #8626
Test plan
e2e
failures
updated 2 snapshotse2e
toMatchSnapshot
toMatch
assertion with 2 newe2e
watchModeUpdateSnapshot
updated 1 snapshotjest-matcher-utils
printDiffOrStringify
jest-snapshot
printDiffOrStringified
2317 and added 4 snapshots; added36 testsSee also pictures in 4 following comments: baseline is at left and improved is at right