Highlight changed strings within lines in jest-diff

[pedrottimark]

Summary

Highlight changed strings within lines which also have unchanged strings to achieve goals:

• Practical: “During information-search tasks, users quickly scan for relevant information and tend to interact with items that have high information scent.”
• Psychological: “People tend to be more forgiving of beautiful designs” which Jest will need in a minority of test failures when the highlighted changes are not the most relevant information.

See https://www.nngroup.com/articles/first-impressions-human-automaticity/

Call diff-match-patch package with semantic cleanup as a second pass for adjacent sequences of deleted and inserted lines.

Because highlighted changes seem more intuitive when lines have been compared without indentation in the first pass, this pull request:

• includes toEqual or toMatchObject assertions: whenever both received and expected values are data structures
• does not include toBe or toMatch with multiline strings, nor toMatchSnapshot assertions

Examples of proposed results adapted from tests:

Regress scenario: simulated click fails
Insider issue: strings within line

Progress scenario: component changes to become more reusable
Insider issue: strings at beginning of line

Progress scenario: data structure changes
Insider issue: string and break at end of line

Progress scenario: change props
Insider issue: breaks within line
Counterexample: no highlight because no changed line has BOTH changed AND unchanged strings

Progress scenario: render sibling element and also edit text
Insider issue: semantic cleanup helps (compare to no cleanup at right)

Progress scenario: CSS-in-JS style prop changes
Insider issue: semantic cleanup hurts (compare to no cleanup at right)

Regress scenario: internationalization failed
Progress scenario: lorem ipsum replaced by actual content

While you review this long pull request, enjoy cup of hot beverage or glass of cold :)

Test plan

Added new test file with 13 pairs of snapshot tests and consistency comparison for expanded and unexpanded options, which call different functions from diff package.

Updated existing snapshots of diffs which compare well with highlight on GitHub:

• jest-diff
• expect
• jest-matcher-utils

[codecov-io]

Codecov Report

Merging #4619 into master will increase coverage by 0.58%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4619      +/-   ##
==========================================
+ Coverage   56.17%   56.76%   +0.58%     
==========================================
  Files         194      194              
  Lines        6544     6633      +89     
  Branches        3        3              
==========================================
+ Hits         3676     3765      +89     
  Misses       2867     2867              
  Partials        1        1

Impacted Files	Coverage Δ
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 f264780...d1e4c87. Read the comment docs.

[SimenB]

dev deps are at the bottom level package.json

[pedrottimark]

Thank you very much!

[pedrottimark]

Future goal: extend “align” loop to get lines and strings from diff-match-patch without diff

[pedrottimark]

Rebased and ready for review, if it passes CI. Yet again, many thanks to Yarn team!

Experiment to replace this two-pass lines-followed-by-strings with one unified pass using diff-match-patch alone without diff was worthwhile, but no go. Sometimes the diff is too different:

Experiment at right has a misleading change from <h2> to <header>

Experiment at right nicely separates change in <p> from change in text. But improving the order of deleted and inserted lines is not a goal, and it would be a breaking change for diff-snapshot

[SimenB]

should these be in the snapshot serializer? (https://github.com/facebook/jest/blob/master/packages/pretty-format/src/plugins/convert_ansi.js)

[pedrottimark]

The goal for this subset of ConvertAnsi plugin is because the default for deleted and for inserted mess up the indentation, which is important in these tests.

[SimenB]

else if

[pedrottimark]

Aha, no-else-return is not among the rules for this project :)

[SimenB]

return lines.find(line => line[o] !== char) instead of the while, right?

[SimenB]

No, you're looking for the index. nvm then 🙂

(technically this works, not convinced if it's cleaner/clearer, though)

let returnValue;

lines.find((line, index) => {
  returnValue = index;
  return line[o] !== char
});

return returnValue;

[pedrottimark]

Yeah, more while loops that I wrote in a while.

Important here to return initial index if line does not have that char or if at end of array.

[pedrottimark]

On second thought, what I meant to say is I would have followed more fluent JavaScript idiom to return -1 if not found, except that findIndex method does not have a fromIndex argument like indexOf method. Therefore, this C language style of parsing loop instead.

[SimenB]

I have to admit the diff is a bit too large for me to grok it, but the tests, and screenshots, looks really good. And nothing jumped out at least looking at the implementation 🙂

[pedrottimark]

Thank you for review. The alignDelInsDiffs function is the most critical change. If it doesn’t put the diff items together corresponding to the original lines, then formatLine will access length property of undefined I would feel better if I were such a fuzz buster as Christopher Chedeau :)

[pedrottimark]

Can we wait to close this sometime in 1Q 2018 when I replace it with new pull request after:

• I replace diff dependency with jest-diff-arrays
• I write a substitute for cleanup from diff-match-patch so we get right instead of left of:

[cpojer]

Closing as discussed on chat. We'll reopen when it's ready.

[SimenB]

@pedrottimark Any news on this? 🙂 Would be super awesome to land this as part of Jest 23 - quite flashy and really useful!

[SimenB]

@pedrottimark any news on this now that we've swapped diffing algorithms?

[pedrottimark]

Yes, here is warm-up exercise to improve report when assertion fails for some matchers:

• .not.toMatch(string | regexp) or .not.toThrow(string | regexp) inverse highlight received substring that did match the expected substring or pattern (copy a snippet of code from pretty-format to format substrings without escaping the formatting codes)

• .toMatch(string) or .toThrow(string) diff of expected and received strings (compare diff-sequences to diff-match-patch with its semantic clean up heuristic for some realistic values)

[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.