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

Highlight changed strings within lines in jest-diff #4619

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@pedrottimark
Copy link
Collaborator

pedrottimark commented Oct 6, 2017

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
1_strings_within

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

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

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

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

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

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

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

This comment has been minimized.

Copy link

codecov-io commented Oct 6, 2017

Codecov Report

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

Impacted file tree graph

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

"jest-get-type": "^21.2.0",
"pretty-format": "^21.2.1"
},
"devDependencies": {

This comment has been minimized.

@SimenB

SimenB Oct 7, 2017

Collaborator

dev deps are at the bottom level package.json

This comment has been minimized.

@pedrottimark

pedrottimark Oct 7, 2017

Author Collaborator

Thank you very much!

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 8, 2017

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

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 13, 2017

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>

unified_worse

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

unified_better

result
.replace(ansiRegex(), (match, offset, string) => {
switch (match) {
case styles.inverse.open:

This comment has been minimized.

@SimenB

This comment has been minimized.

@pedrottimark

pedrottimark Oct 13, 2017

Author Collaborator

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.

if (digit !== 1) {
diffsDel[diffsDel.length - 1].push(diff);
}
if (digit !== -1) {

This comment has been minimized.

@SimenB

SimenB Oct 13, 2017

Collaborator

else if

This comment has been minimized.

@pedrottimark

pedrottimark Oct 13, 2017

Author Collaborator

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

char: string,
i: number,
): number => {
while (i !== lines.length && lines[i][0] === char) {

This comment has been minimized.

@SimenB

SimenB Oct 13, 2017

Collaborator

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

This comment has been minimized.

@SimenB

SimenB Oct 13, 2017

Collaborator

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;

This comment has been minimized.

@pedrottimark

pedrottimark Oct 13, 2017

Author Collaborator

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.

This comment has been minimized.

@pedrottimark

pedrottimark Oct 14, 2017

Author Collaborator

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

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 13, 2017

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

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 14, 2017

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

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 11, 2017

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:

6_semantic_cleanup_hurts

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 8, 2018

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

@cpojer cpojer closed this Feb 8, 2018

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented May 8, 2018

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

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 22, 2019

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

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 22, 2019

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment