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

jest-snapshot: Highlight substring differences when matcher fails, part 3 #8569

Merged
merged 17 commits into from Jun 18, 2019

Conversation

@pedrottimark
Copy link
Collaborator

pedrottimark commented Jun 14, 2019

Summary

When expected and received are both string, snapshot matchers call printDiffOrStringify

If neither received nor expected (snapshot) is multiline, or either is empty:

  • display following Snapshot: and Received: labels

If either received or expected (snapshot) is multiline, and neither is empty:

  • if semantic cleanup leaves a common substring, highlight substring differences
  • otherwise display line diff, however without enclosing double quote marks

Test plan

The change in report for single-line strings affects a few tests:

  • updated 1 toMatch assertion in e2e/__tests__/toMatchSnapshot.test.ts
  • updated 2 snapshots in e2e/__tests__/failures.test.ts
  • updated 1 snapshot in watchModeUpdateSnapshot.test.ts

Added printDiffOrStringified.test.ts in jest-snapshot package

See pictures of reports from local tests in following comments

Example pictures baseline at left and improved at right

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 14, 2019

Display single line strings following Snapshot: and Received: labels

toThrowErrorMatchingInlineSnapshot

Display multiple line strings with highlight for substring differences

toThrowErrorMatchingSnapshot

toMatchSnapshot

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 14, 2019

The --expand and default --no-expand options affect string diff:

toMatchInlineSnapshot expand

toMatchInlineSnapshot no-expand

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 14, 2019

If semantic cleanup doesn’t leave a common substring, then display line diff

The baseline with enclosing double quote marks from pretty-format serialization:

fallback to line diff baseline

The improved without enclosing double quote marks has a cleaner line diff:

fallback to line diff improved

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 14, 2019

Codecov Report

Merging #8569 into master will increase coverage by 0.02%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8569      +/-   ##
==========================================
+ Coverage    63.2%   63.22%   +0.02%     
==========================================
  Files         271      272       +1     
  Lines       11284    11304      +20     
  Branches     2749     2757       +8     
==========================================
+ Hits         7132     7147      +15     
- Misses       3538     3540       +2     
- Partials      614      617       +3
Impacted Files Coverage Δ
packages/jest-snapshot/src/index.ts 28.77% <50%> (+0.4%) ⬆️
packages/jest-snapshot/src/print.ts 63.63% <63.63%> (ø)
packages/jest-snapshot/src/utils.ts 94.87% <0%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe14493...1da96c1. Read the comment docs.

Copy link
Collaborator

thymikee left a comment

<3

packages/jest-snapshot/src/index.ts Outdated Show resolved Hide resolved
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 14, 2019

EDIT: After researching the history, I see that unescape is limited by design :)

[Uh oh, I need to fix a mistake for backslash in unescape helper function:

  • which had been consistently incorrect because called for both expected and received
  • but now is inconsistently incorrect because called only for expected if both are strings]

escape back slash

Also I will factor out one or more pure helper functions for report into utils.ts so it can have unit tests to increase coverage and prevent regression.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 14, 2019

unescape defined in #2852 removes backslash escape preceding double quote marks

The escape is added again for single line string because both are formatted by pretty-format

escape double quote mark

To be consistent because we know that the expected snapshot is a serialized string, let’s also remove backslash escape preceding backslash.

Incorrect at left and corrected at right for multi line string which does not have escape:

escape back slash multi

Incorrect at left and corrected at right for single line string which does have escape:

escape back slash single

Regular expression still has consistent extra escaping but that is out of scope for this pull request:

escape regexp single

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 14, 2019

Although I forgot to make pictures of baseline here are improved when diff is not relevant:

empty expected

empty received

@jeysal
jeysal approved these changes Jun 15, 2019
Copy link
Collaborator

jeysal left a comment

Awesome! Do we need some tests for the non-trivial cases here?

CHANGELOG.md Outdated Show resolved Hide resolved
@pedrottimark pedrottimark changed the title expect: Highlight substring differences when matcher fails, part 3 jest-snapshot: Highlight substring differences when matcher fails, part 3 Jun 15, 2019
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 15, 2019

Added another test and got a mistake-1-fix-2 special. When I added the test with snapshot of baseline report, and then corrected the code, and then ran test again without -u option, the diff report for baseline versus improved was another edge case to fix. TDD for the win, in this case :)

Here is baseline and improved for the first fix:

single line received is not string

`;

exports[`empty string received and expected multi line 1`] = `
Snapshot: <g>"multi</>

This comment has been minimized.

Copy link
@jeysal

jeysal Jun 16, 2019

Collaborator

In the empty/multiline case, maybe start the multiline string on the next line so that the indentation is less off (still off by one because of the ")?

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Jun 16, 2019

Author Collaborator

Super question, as usual. Extra credit for thinking about alignment.

Here are 2 alternatives for response or maybe they provoke some other solution:

multiline empty

I though about omitting double quote marks, but content of string might be ambiguous with serialization of other data types.

I’m thinking backticks for multiline string property in future data-driven diff at right:

multiline property value

This comment has been minimized.

Copy link
@jeysal

jeysal Jun 16, 2019

Collaborator

Second one is probably easier on the eyes if the string itself has lines indented by 2. Wondering if inserting indentation into people's strings is a good idea though - is there a legitimate use case for copying it out of the terminal?

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Jun 16, 2019

Author Collaborator

Yes, inserting indentation is a concern. Although Jest indents the reports in console.

Of alternatives above, I agree with you to prefer at the right.

Here is the minimal solution at the left and another indented alternative at the right:

multi 2

Unless there is a clear winner, I think to leave it alone (as it has been) for this pull request.

import {serialize, unescape} from '../utils';

// This is an experiment to read snapshots more easily:
// * to avoid first line misaligned because of opening double quote mark,

This comment has been minimized.

Copy link
@jeysal

jeysal Jun 16, 2019

Collaborator

Nice! Does some things more and some things less than https://github.com/ikatyang/jest-snapshot-serializer-raw - maybe we should think about making a full-featured serializer for edge case strings 😄

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Jun 17, 2019

Author Collaborator

Thank you for the link! It answers questions in back of my mind about prettier snapshots.

So I am exploring if this pull request can also highlight substrings when:

  • received is string
  • expected is serialized by application-specific plugin instead of default stringify
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 17, 2019

In printDiffOrStringified

  • improve first if condition: received has default stringify and snapshot looks like it has
  • otherwise support special use case of application-specific serialization for string itself

Baseline without else falls back to line diff:

prettier 5590 baseline

Improved with else displays string diff:

prettier 5590 improved

The more realistic object wrapper with symbol key, which is beyond the scope of this pull request, goes into my mental slow cooker as example of need to extend the future data-driven diff.

cc @ikatyang if you have thoughts to share how we can improve report when snapshot fails

@ikatyang

This comment has been minimized.

Copy link
Contributor

ikatyang commented Jun 18, 2019

This looks great 👍

(It's a bit off topic but the biggest problem to me for the snapshot escapes is that the content in the .snap file still needs to be escaped due to the fact that .snap is still a JS file, which causes it hard to read and the line width is also affected. Is it possible to have something like file-level snapshots to completely get rid of escapes?)

cc @duailibe

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 18, 2019

@ikatyang your thought about file-level snapshots sounds super for prettier tests. For example:

tests/typescript_conditional_types contains:

  • conditional-types.ts
  • infer-type.ts

tests/typescript_conditional_types/__snapshots__/jsfmt.spec.js.snap contains:

exports[`conditonal-types.ts 1`] = ``
exports[`infer-type.ts 1`] = ``

Here is alternative mapping to file-level “snapshot” per test file:

  • tests/typescript_conditional_types/__snapshots__/conditional-type.ts.snap
  • tests/typescript_conditional_types/__snapshots__/infer-type.ts.snap

Although my understanding of stateful code in jest-snapshot is too little to know how to hack it :)

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jun 18, 2019

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 18, 2019

Simen and Michał an example of your forward thinking was request in part 2 to move getStringDiff into jest-diff therefore future version of jest-file-snapshot can highlight substrings in its report.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 18, 2019

Here is an example adapted from ESLint tests which include backtick quote marks to show that regardless of escaping in a snapshot file, the report displays without any escape.

backtick single multi

A follow-up pull request unrelated to substring highlight will compare unescape function for double quote marks in snapshots with stringify in reports for other matchers.

@SimenB
SimenB approved these changes Jun 18, 2019
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 18, 2019

@SimenB It looks like the CI failure is bogus. Is it okay for me to merge so next minor has all 3 parts?

@jeysal
jeysal approved these changes Jun 18, 2019
Copy link
Collaborator

jeysal left a comment

None of the topics I brought up need to be addressed right here, right now, so go for it ;)

@pedrottimark pedrottimark merged commit 6fbcd9e into facebook:master Jun 18, 2019
10 of 11 checks passed
10 of 11 checks passed
deploy/netlify Deploy preview failed.
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
facebook.jest #20190618.3 succeeded
Details
@pedrottimark pedrottimark deleted the pedrottimark:highlight-substring-3 branch Jun 18, 2019
@duailibe

This comment has been minimized.

Copy link
Contributor

duailibe commented Jun 19, 2019

There is https://www.npmjs.com/package/jest-file-snapshot

@SimenB I didn't know that package, I've built almost the same thing for Prettier but discarded after I've realised there's no way to remove obsolete snapshots, at least not in the worst case scenario when the matcher isn't called at all and there's no chance to detect obsolete snapshots.

I got excited for a second thinking that maybe that lib fixed it somehow, but unfortunately it didn't.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jun 19, 2019

@duailibe If plain text snapshot file is important in Prettier, can run_spec.js do the following:

  • For each test file, assert toMatchFile for path in __file_snapshots__ subdirectory and the corresponding test file name with whatever extension makes sense to y’all
  • If expect.getState().snapshotState._updateSnapshot === 'all' then it (not the matcher) deletes any files in the __file_snapshots__ subdirectory that do not correspond to a test file

For each subdirectory of tests to clean up its own file snapshots is more limited than global cleanup with jest -u but seems good enough even if y’all delete or rename entire subdirectory.

To close a gap in simulated integration, it is even possible for run_spec.js to cause Jest to report a snapshot file without corresponding test file as either obsolete or removed for -u option:

expect.getState()._uncheckedKeys.add(fileName + ' 1'); // must end with a number
aliaksandr-yermalayeu added a commit to aliaksandr-yermalayeu/jest that referenced this pull request Jul 16, 2019
…rt 3 (facebook#8569)

* expect: Highlight substring differences when matcher fails, part 3

* Delete duplicate line comment slashes

* Update CHANGELOG.md

* Remove backslash escape preceding backslash

* Replace expect with jest-snapshot in CHANGELOG.md

* Delete unnecessary isExpand function

* Factor out printDiffOrStringified into added print.ts file

* Add unit tests for printDiffOrStringified function

* Add Facebook comment to 2 added files

* Fix two edge cases

* Fall back to line diff if either serialization has newline

* Also display diff if strings have application-specific serialization

* Add test without serialize

* Edit comments for consistent use of serialization versus stringified

* Call getStringDiff when strings have custom serialization

* Edit comment

* Add tests for backtick and isLineDiffable true single-multi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.