expect: Highlight substring differences when matcher fails, part 1

[pedrottimark]

Summary

Fixes #6881 although space and no-break space still look the same

If positive assertion fails with one-line strings for both expected and received:

Then if character difference with “semantic cleanup” has any common substring:

Display changes similar to Changed Files on GitHub:

• Expected: inverse green for deleted characters
• Received: inverse red for inserted characters

Else if no common substring, display without any inverse colors

Build:

EDIT: The following list is generally but not specifically true, because printDiffOrStringify moved to jest-matcher-utils package

• packages/expect/build-es5/index.js is about 1M bytes, increased < 40K bytes
• although I added diff-sequences dependency, it was already in build-es5 bundle
• added to expect/tsconfig.json see expect: Highlight substring differences when matcher fails, part 1 #8448 (comment)
• review of packages/expect/src/cleanupSemantic.d.ts is especially welcome
• see expect: Highlight substring differences when matcher fails, part 1 #8448 (comment) about packages/expect/build/cleanupSemantic.d.js

License:

Does anyone know if I have correctly handled code with Apache License, Version 2.0?

• added comment describing changes to cleanupSemantic.js
• added file to .eslintignore and .prettierignore
• added file path as exception to scripts/checkCopyrightHeaders.js

Residue:

• Decide how to display results for multi-line strings (especially deleted or inserted newlines)
• Discover whether long strings cause performance risk as had been for many lines
• Apply to more changed strings (for example, property values) in future data-driven diff
• Adapt for toThrow(object) which does not call the same helper function
• Explore how to display results of failed substring match for toContain, toMatch, or toThrow [EDIT: and found its effectiveness to be disappointingly hit or miss]

Test plan

EDIT: Updated a distantly related snapshot test, see see #8448 (comment)

Existing tests pass, because they have no common substring with “semantic cleanup”

Added 1 snapshot each:

• toBe and then added a second test for empty string, see expect: Highlight substring differences when matcher fails, part 1 #8448 (comment)
• toEqual
• toHaveProperty(path, value)
• toStrictEqual

See also pictures in following comment

[pedrottimark]

Example pictures baseline at left and improved at right

Change a word:

Change spaces to no-break spaces:

Edit around a phrase:

I added a simpler example (than the above) to unit tests

Edit around and in a phrase:

[pedrottimark]

I am not sure if the .d.ts file in src directory is correctly included in the build:

Here is packages/expect/build/cleanupSemantic.d.js

'use strict';

function _defineProperty(obj, key, value) {
  if (key in obj) {
    Object.defineProperty(obj, key, {
      value: value,
      enumerable: true,
      configurable: true,
      writable: true
    });
  } else {
    obj[key] = value;
  }
  return obj;
}

[pedrottimark]

Thinking ahead to remove limitation on one-line strings in future pull request, what do y’all think about displaying inserted or deleted line break as U+21B5 downwards arrow with corner leftwards:

[SimenB]

Oh my god, I'm so excited! 😀

I am not sure if the .d.ts file in src directory is correctly included in the build:

It should be, we had the same thing @jest/reporters and it worked

[SimenB]

So good!

[SimenB]

Would it make sense for the vendored file to live in diff-sequences and not expect?

[SimenB]

And yes, rendering some explicit newline character would be awesome, like ava does

[codecov-io]

Codecov Report

Merging #8448 into master will increase coverage by 0.45%.
The diff coverage is 79.79%.

@@            Coverage Diff             @@
##           master    #8448      +/-   ##
==========================================
+ Coverage   62.32%   62.78%   +0.45%     
==========================================
  Files         266      267       +1     
  Lines       10734    11021     +287     
  Branches     2615     2674      +59     
==========================================
+ Hits         6690     6919     +229     
- Misses       3461     3497      +36     
- Partials      583      605      +22

Impacted Files	Coverage Δ
packages/expect/src/print.ts	98.76% <100%> (+1.03%)	⬆️
packages/expect/src/cleanupSemantic.ts	76.8% <76.8%> (ø)

---

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 db5e472...dd56dec. Read the comment docs.

[pedrottimark]

@SimenB it’s related to your #8085 (comment) although jest-reporters had no src/*.js file:

It seems like those files are not copied over by tsc

there are some d.js files there, funnily enough

I think the problem is https://github.com/facebook/jest/blob/master/scripts/build.js#L122-L125

• it does not copy the .d.ts file because it does match TS_FILES_PATTERN
• but then I assume that babel compiles it as if it is source code

Does it make sense to swap the logic to avoid double negative in if-else as follows:

if (
  micromatch.isMatch(file, JS_FILES_PATTERN) ||
  (micromatch.isMatch(file, TS_FILES_PATTERN) &&
    !micromatch.isMatch(D_TS_FILES_PATTERN))
) {
  // compile file
} else {
  // copy file
}

[SimenB]

They should work - we deleted some in #8294 which worked fine previously.

It's the tsc run that should copy it, not babel. Try to run build:ts

[pedrottimark]

Good question about packaging. jest-matcher-utils is a possibility after interface becomes clear

The reason to keep semanticCleanup out of diff-sequences is same reason we did not just depend on the excellent diff-match-patch package:

• it has features we don’t need that I’m not sure tree shaking could get rid of
• its class interface it is harder to adapt than our callback interface

[pedrottimark]

@SimenB this is making me learn more about Jest build scripts and TypeScript config :)

yarn build-clean and then yarn build:ts creates .d.ts and .d.ts.map only for src/*.ts files

• It does not copy through src/*.d.ts to build directory
• Even, by the way, if I add it explicitly to a files array in expect/tsconfig.json file

Compiling as .ts there are 22 errors but allowJs: true and // @ts-check comment only 7 errors because tsc uses JSDoc comments. However that doesn’t give us the .d.ts file (Option 'allowJs' cannot be specified with option 'declaration' error) which is the problem to solve.

[pedrottimark]

Solved by renaming as cleanupSemantic.ts which was not as hard as I had supposed:

• Converted Diff to class from constructor and prototype
• Added type annotations for arguments and return values

Finding that tricky Diff.prototype.length = 2; isn’t possible in TypeScript took most of the time ;)

Question: in packages/expect/tsconfig.json is it an optional or necessary manual edit to add {"path": "../diff-sequences"}, to "references" array for new dependency?

[jeysal]

Wooooo, this is super awesome ❤️

[jeysal]

Question: in packages/expect/tsconfig.json is it an optional or necessary manual edit to add {"path": "../diff-sequences"}, to "references" array for new dependency?

Mandatory (TSC won't ever look at the folder if it's not referenced) - if it works without, it's probably "lucky" because it's also referenced from ´jest-diff`

[SimenB]

Ah, the way we solved it for Istanbul was a /// <reference /> entry. Since you've converted already that's probably a better solution :)

[pedrottimark]

While writing tests for multi-line strings, I found an edge case of comparison to empty string

Highlighting the non-empty string at left seems less helpful than no highlight at right:

Added a test and then updated it after adding the missing condition

For the test where I found the problem, do you prefer the report at left or right, and why?

[SimenB]

I prefer the right one, since it's explicit about the value of "Received". The left one looks like a bug due to it missing received

[pedrottimark]

• moved printDiffOrStringify from expect to jest-matcher-utils package
• updated one snapshot test because it is incorrect to display diff for asymmetric matcher

Residue

• part 2: specialized diff with highlight for multiline strings and also use in toThrow(object)
• part 3: when received is a string, call printDiffOrStringify for report when snapshot fails
• data-driven diff: use substring diff when changed array item or object property is a string

[jeysal]

Still LGTM after the tweaks, feel free to merge if you have nothing more to include here in mind ;)

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