Ignore indentation of data structures in jest-diff

[pedrottimark]

Summary

To minimize incorrect decisions about diffs for tests that fail, display some lines in data structures as unchanged with the received indentation, if their only change is indentation. That is, distinguish content that remains the same from changes to the surrounding hierarchy.

Except multiline strings in data structures [EDIT: and snapshots of strings] where change in indentation seems significant.

Baseline diff is less clear:

 <span>
-  <span>
-    text
-  </span>
+  <strong>
+    <span>
+      text
+    </span>
+  </strong>
 </span>

Proposed diff is more clear:

 <span>
+  <strong>
     <span>
       text
     </span>
+  </strong>
 </span>

I have doubts at times it’s worth 100 lines of code, but the diff for a hypothetical change to hierarchy of rendered elements came up in again yesterday in #2202 (comment)

Test plan

• Deleted 2 assertions under oneline strings which were redundant with other assertions.
• Moved collapses big diffs to patch format to precede React elements. [EDIT: will put it back]
• In React elements replaced 5 regexp assertions with one substring assertion.
• Added 7 new tests.

[codecov-io]

Codecov Report

Merging #3429 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3429      +/-   ##
==========================================
+ Coverage   56.17%   56.37%   +0.19%     
==========================================
  Files         191      191              
  Lines        6424     6452      +28     
  Branches        6        6              
==========================================
+ Hits         3609     3637      +28     
  Misses       2812     2812              
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-diff/src/index.js	81.81% <100%> (+0.86%)	⬆️
packages/pretty-format/src/plugins/convert_ansi.js	100% <100%> (ø)	⬆️
packages/jest-diff/src/diff_strings.js	100% <100%> (ø)	⬆️

---

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 dfdca8b...f178db0. Read the comment docs.

[thymikee]

This is great, but what about testing indentation (e.g. in CLI output)? I wonder if we could add options object as an argument for snapshot matchers, something like:

expect(diff).toMatchSnapshot({whitespace: true});

[pedrottimark]

Good point, you mean that I might have caused a regression for a multiline string by itself?

[thymikee]

Yeah, but I'm not so sure how to deal with it properly. It's still just a diff, so snapshots wouldn't be affected (on the filesystem), but it may be weird for user to see Jest tests passing and having whitespace differences in their commit. I mean this would require to be extra careful while giving code review.
And for regular matchers, our whole test suite for pretty-format would become obsolete, because we test whitespace regular way, not with snapshots. Am I right?

[pedrottimark]

Here are the prerequisites that I know about before we continue reviewing this PR:

[x] For the question about snapshots of multiline strings especially CLI output, verify that unindentLines works correctly because pretty-format enclosed them in quote marks.
[x] Add a test for multiline strings.
[x] Adapt logic from diffLines to structuredPatch branch of logic which Jest uses by default!
[x] Make added tests cover both diffLines and structuredPath branches!

The last two items bring up a question about an inconsistency:

• The diffLines code path that I adapted first (which only runs for --expand option, which reveals that I had limited understanding about the code) outputs a character (plus or minus, or space for unchanged) with a separating space before the content of the line. That is, formatting is under control of Jest.
• The structuredPatch code path outputs the character without a separating space before the content of the line. That is, format is produced by diff package.
  You can see the subtle difference in a diff for a failing test with or without the --expand option. Does this inconsistency matter?

[pedrottimark]

As mentioned above, snapshot of multiline string is okay because it contains quotes.

Ready for review to continue, [EDIT: s/but not yet/and maybe now/] to merge, because I need to follow up on multiline string in toBe and toEqual assertion which if only difference is indentation now reports:

Compared values have no visual difference.

So I need to look more carefully at the code paths to deal with string value not from snapshot.

[EDIT: commit below adds test and conditional code for multiline string not from snapshot.
The condition options.aAnnotation !== 'Snapshot' seems less than ideal. Any other idea?]

[pedrottimark]

To answer the question in #3429 (comment) with a question: can we make sure that (or improve fix needed so) this PR only affects the feedback about a change not the criterion for a change?

Especially if we can make sure that ignoring indentation applies to output from pretty-format then can we conclude that when a test fails, if the diff contains some unchanged parts of content which moved in the hierarchy, then the cause must have been some change in structure that the diff will still display?

[pedrottimark]

Oh, something just hit me. Clunk! The algorithm becomes clearer and safer if:

• When non-snapshot assertion of non-primitive type fails, call pretty-format to produce both indented and unindented strings. I will see if the package already has an option to do that.
• [EDIT on 2017-06-28 the following is obsolete: When snapshot assertion fails, if the received value is non-primitive type, provide it to jest-diff as a new property of the options argument. Call pretty-format to produce the unindented string which corresponds to the indented string which toMatchSnapshot provides as the received argument. To bad, so sad, I think that jest-diff still needs to call its own unindent function on the expected string from the snapshot file.]
• The algorithm still replaces unindented lines with original indented lines in chunks or hunks.

[pedrottimark]

Now ready for review. Here are two changes that affect the public interface:

• [EDIT on 2017-06-28 the following is obsolete: Added snapshot?: boolean to DiffOptions so jest-diff can distinguish snapshot (in which strings are enclosed in quote marks) from assertions like toBe (in which they are not). In jest-snapshot/package.json the dependency on jest-diff might need to be updated at the next release.]
• Removed space between +/-/space and content of line for --expand option which made the diff output inconsistent from default unexpanded comparison. It affected two snapshots. Besides reducing confusion about a subtle difference in the diff, it made it more realistic to extend most of the tests in diff-test.js for both unexpanded and expanded options.

[pedrottimark]

While reading code for #3825 I realized that this change might ignore changes to leading spaces in text content of a React or HTML element. After I return from visit with family next week, I will think more about it. As was suggested elsewhere, maybe develop a diff of data structures instead?

[pedrottimark]

Rebased and reduced scope of improvement to ignore indentation:

• only to compare objects themselves (for example, if a toMatchObject assertion fails) because it is possible to pretty-format the expected and received values without indentation for diffLines or structuredPatch and then replace the original lines.

• not for snapshots because I cannot think of any way to ignore indentation in snapshots without also ignoring differences in leading spaces of text nodes in React elements. Too bad, so sad!

If you think this is too much code change and potential confusion from a difference in feedback for similar tests with snapshot versus non-snapshot assertions, I can cut the PR back as follows:

• Still eliminate the difference in spaces between expanded and unexpanded options
• And therefore make as many tests as possible apply to both options

[pedrottimark]

Oh, after thinking about the code diff for #3962 and then waking up in the middle of the night to stray fireworks thinking red-white-blue, I wonder about a third color code for differences only in indentation? Maybe then it can apply to snapshots too, after all :)

As follow up to this thought, I will

• see if there are any relevant options for diff already in git and GitHub
• look at code to highlight whitespace in Highlight whitespace in expected/received #2674
• make a visual prototype for critique

[pedrottimark]

Your critique is welcome about developer experience when a test fails: to decide whether (or which part of) the diff is correct or incorrect.

Each of the following 4 scenarios has 3 pictures:

• Left: current output from jest-diff for option --expand
• Middle: proposed output of snapshot test (for future PR)
• Right: proposed output of non-snapshot test (for current PR)

First form your impression of the meaning, and then read the intended meaning below :)

Data structure with decreasing indentation

React elements with increasing indentation

React elements with changes to edge spaces in text

React elements with increasing indentation and changes to edge spaces in text

The new third color cyan means the only difference in a line is:

• Snapshot test: leading spaces (algorithm cannot distinguish indentation from leading spaces in text of an element) therefore display adjacent lines as removed/added chunks with the difference in removed/added spaces highlighted at the beginning of lines.

• Non-snapshot test: indentation spaces (algorithm can pretty-format expected and received with indent: 0 option) therefore display as one unchanged line with received indentation. Furthermore:

  • Because a change to leading (non-indentation) spaces (in text of an element) counts as a difference, highlight leading content spaces similar to trailing spaces in green, red, or cyan, in any line that has a text color (that is, not dim for absolutely unchanged).

  • If a line contains only spaces, highlight only content spaces, not indentation spaces.

If we decide to move forward with this or a derived proposal, I will rebase and update this PR for non-snapshot diffs, and then submit another PR for snapshot diffs.

[pedrottimark]

Oh, and do y’all have any preference:

• default non-expanded: content directly after the first column which contains -, +, or space
• --expand as pictures in preceding comment: a space separates content from first column

[pedrottimark]

Pushing these commits isn’t an expectation. I trust y’all to decide on priorities for Jest :)

• Support compared objects as described in Ignore indentation of data structures in jest-diff #3429 (comment)
• Let’s wait on snapshots and multiline strings until after Don't wrap multiline strings with double quotes #4183
• In answer to my question Ignore indentation of data structures in jest-diff #3429 (comment) GitHub agrees with default non-expanded Jest not to separate diff character -/+/space at beginning of line from the rest of the line. So these recent commits remove the space for --expand option to make both options consistent.
• Tomorrow I will review the tests that I had added to diff.test.js with fresh eyes.
• I will probably need to rebase after Join lines with newline in jest-diff #4314

[cpojer]

@pedrottimark you were kind of on a monologue on this PR, and sorry for not jumping in earlier. I don't currently have time to read through the whole thing but I agree with the overall premise. What do we need to do to get this into Jest 21? :)

[pedrottimark]

I need to:

• rebase, as expected
• review code diff
• update tests of indentation and add test of colors
• think if any conflict with serialization of multi-line strings: NO, but one test becomes redundant
• think if any conflict with snapshot-diff package: YES, it can return fewer added/removed lines

[pedrottimark]

@cpojer Ready for review, at your convenience. Summary from “reducer function” for comments :)

• This PR affects jest-diff results when expected and received are data structures (for example, from toEqual or toMatchObject assertions). It formats the values as both:
  • original with default indent option
  • compared with indent: 0 option (that is, diff the lines without indentation)
• Then it replaces compared lines with original lines. If compared lines are equal but original lines are not, then they are unchanged except for indentation and the new format is cyan color.
  • The --no-colors option doesn’t distinguish them from completely unchanged lines.
  • The default --no-expand option can omit some of them as unchanged lines.
• A future PR after Jest 21 will affect jest-diff results for snapshots as expected values.

Except for removing space in second column, pictures in #3429 (comment) are still relevant:

• Left: current result of jest-diff for --expand option
• Middle: future PR result for snapshot test
• Right: this PR result for non-snapshot test

@thymikee

• Whichever of this or Don't wrap multiline strings with double quotes #4183 is merged second will need to resolve merge conflicts, I think :)
• Because snapshot-diff calls jest-diff equivalent to --no-colors and --no-expand options, then this PR could affect some snapshots originally taken with earlier versions, by reducing the number of removed and added lines. For the cases when I would use diff-snapshot this is a change in a helpful direction, even if annoying to trouble shoot. Your feedback is welcome, if you see a greater downside risk.

[cpojer]

"getDiffCharacter"?

[cpojer]

getDiffDigit?

[cpojer]

It's suspicious that these are indented less by one space now. Why was this change made? It makes "Object" not line up with "Received" any longer.

[pedrottimark]

This space or no space confused me for a long while. We can go either way. Your choice!

Before this pull request the result changed according to CLI option:

• default --no-expand option uses hunk format from diff with no space between the first column marker and the actual line
• --expand option formatted the chunk lines from diff with a space there.

[cpojer]

I didn't realize that it was different based on cli options, it shouldn't be! I think two spaces (make it line up with "Received") makes more sense, and it would be good to be consistent.

[pedrottimark]

I like it better too. To make double sure I understand:

• The first column consists of -/+/space for removed/added/unchanged
• The second column consists of space to separate from line content and align with legend

[cpojer]

Yep! The first column may be empty space as well.

[cpojer]

This is really hard to read. Would it hurt to use one if-statement and one ternary? :D

[cpojer]

Could we type this function better?

[pedrottimark]

You mean more specific type than Function for the optional argument?

[cpojer]

Yeah, like annotating the input type and return value using (number) => ReturnValue etc.

[cpojer]

This function call is also incredibly hard to read. Any way we could factor out the inline ternaries etc.?

[cpojer]

Why is cyan added here?

[pedrottimark]

This is side effect of supporting cyan and background colors in ConvertAnsi plugin. I can revert that change and that diff produces the correct colors in a more specific way.

[cpojer]

With all your explanation, I'm still not sure why the cyan is printed here at all. Is this just syntax highlighting for the React component itself? If yes, then this change is fine. If no, I'd like to understand better why the cyan is there now.

[pedrottimark]

Aha, got it. What you said. When we changed from raw ANSI codes to ConvertAnsi plugin, we overlooked that not all colors in the default React plugin color options were supported, so there was a loss of information. The change to display cyan as the default color for markers is correct, but really an unintended side effect.

[cpojer]

Yup, makes sense to me now. Thanks for explaining.

[cpojer]

Thanks @pedrottimark, a lot of work went into this and the result (or intermediary step) is pretty awesome. I have a few questions inline, mostly I think those snapshots shouldn't be changing at this point. What do you think?

[pedrottimark]

To make sure I don’t make a different change than you want: which snapshots did you mean shouldn’t be changing? The React components?

[pedrottimark]

@cpojer Thank you for such careful review, especially of changes to snapshots.

AppVeyor build failed: I think because other packages not linked to jest-diff from PR, see #4135

Now snapshots from expect matchers have updates. It will be a sweeter PR than this, if jest-diff ever highlights changed substrings like these green spaces in split files changed view on GitHub ;)

replace	with
DIFF_D	DIFF_DIGIT
getC	getDiffChar
getD	getDiffDigit
ternary ternary [ternary]	if if return [ternary]
Function	type GetOriginal = (digit: DIFF_DIGIT) => Array<string>

@thymikee Consistently formatting lines with space in second column as in --expand option breaks all diff-snapshots from versions earlier than Jest 21. You have my apology for not looking far enough ahead to make a pull request with just that change for Jest 20.

[cpojer]

Nice! Seems like this fails on AppVeyor though :(

[pedrottimark]

Yeah, the last time that a change affected snapshots in other packages, the Appveyor build healed itself eventually after some other PRs had been merged.

@cpojer Until then, what do you think about a small change to this PR:

By analogy with green background color on the second-column spaces added to snapshots in the diff as displayed on GitHub split view, people could misunderstand green or red background color as meaning leading or trailing spaces are changes. Furthermore, jest-diff might eventually use those background colors to highlight changed substrings within lines.

So how about a gray background color to communicate a neutral meaning: although unchanged, don’t overlook leading or trailing spaces in lines. The bgBlackBright color in current version of chalk shows up against white, black, silver, and even gray-green terminal themes.

[pedrottimark]

Surprise discovery: inverse style modifier in ansi-styles and chalk is more concise than explicit background color bgGreen for green or bgRed for red for substrings in a line.

[cpojer]

I merged this now, and will leave it to you to send follow-up PRs to fix behavior up. Could you send a few screenshots of different styles other than cyan? It's easier to make a decision then.

[github-actions]

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.