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

expect: Highlight substring differences when matcher fails, part 2 #8528

Merged
merged 7 commits into from Jun 13, 2019

Conversation

pedrottimark
Copy link
Contributor

Summary

Add specific diff with highlight to printDiffOrStringify for multiline strings

  1. Unless string length is > 20K, it supersedes generic line diff:

    • toBe
    • toEqual
    • toStrictEqual
    • toHaveProperty(path, value)
  2. Also highlight differences in toThrow(object) report if message is not equal

  3. To avoid ambiguity with inverse highlight to mean changed, display middle dot for spaces at end of line for multiline strings in generic print functions:

    • printExpected
    • printReceived

Friendly reviewers, take your time and enjoy with a cup or mug of your favorite beverage :)

Test plan

Added unit tests to jest-matcher-utils package to achieve coverage:

  • getAlignedDiffs.test.ts has 24 snapshots
  • joinAlignedDiffs.test.ts has 6 snapshots
  • print.test.ts has 6 snapshots plus another test

In expect package:

updated added matcher
2 0 toBe
0 1 toEqual
0 1 toStrictEqual
0 1 toHaveProperty(path, value)
2 2 toThrow(object)

See also pictures in following comments

Example pictures baseline at left and improved at right

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jun 3, 2019

Display as diff if either string is multiline:

Notice middle dot instead of highlight to avoid misunderstanding when trailing space is not a change:

toBe

Support single or multiline substring diff for equality comparison of error message:

toThrow highlight

But report for partial match of substring or regexp does not have any highlight:

toThrow expected

toThrow received

A following #8528 (comment) about rough edges explains why not.

@pedrottimark
Copy link
Contributor Author

Small differences are the best case and interleave related lines:

toStrictEqual

@@ -3080,6 +3097,26 @@ Expected value: <green>\\"\\\\\\"That <inverse>cat </>cartoon\\\\\\"\\"</>
Received value: <red>\\"\\\\\\"That cartoon\\\\\\"\\"</>"
`;

exports[`.toHaveProperty() {pass: false} expect({"children": ["Roses are red.
Violets are blue.
Testing with Jest is good for you."], "props": null, "type": "pre"}).toHaveProperty('children,0', "Roses are red, violets are blue.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😀

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jun 3, 2019

Realistic example to replace space with newline:

[By mistake, I reversed expected and received in the added test which is subset of this example]

With default --no-expand option for 5 lines of adjacent context:

prettier 5272 no-expand

With --expand option:

prettier 5272 expand

Artificial example to replace space with newline and the opposite:

toHaveProperty

By the way, except for ↵ to solve very edge case of empty common line at beginning or end, it turned out that displaying an symbol for line breaks was often confusing and complicated the code.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, as always! 🚀

Should we stick all of these diff utils in jest-diff instead? jest-matcher-utils are exposed as this.utils in custom matchers, and I think it'd make sense to group these diff utils onto this.diff as a sort of a namespace thing. Thoughts?

@pedrottimark
Copy link
Contributor Author

And now for rough edge cases for semantic cleanup algorithm.

Eliminate an equality that is smaller or equal to the edits on both sides of it.

In the following picture, receivedLabel: string, has length <= the deleted comments, therefore the common string is merged into the changes. It looks inconsistent because expectedLabel: string, has same length but is exempt from cleanup because it is at the start (that is, does not have change on both sides).

By the way, the relative length of match versus mismatch is the reason that diff was hit-or-miss in my tests for report when partial match fails, like toContain(substring), toMatch(substring), toThrow(substring)

cleanup between

This solution is not perfect. It has tunnel vision; it is unable to see beyond the immediate neighbourhood of each equality it evaluates. This can result in small groups of chaff surviving.

In the following picture, the period at the end survived the cleanup:

cleanup at end

If nothing survives the cleanup, then the report falls back to a line diff:

toEqual prettier 5521

@pedrottimark
Copy link
Contributor Author

@SimenB Super question about packaging!

It is especially helpful that you specifically ask about the context object for matchers.

The reason I exported only printDiffOrStringify for now is to give us time to think about it.

@codecov-io
Copy link

codecov-io commented Jun 4, 2019

Codecov Report

Merging #8528 into master will increase coverage by 2.58%.
The diff coverage is 92.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8528      +/-   ##
==========================================
+ Coverage   60.56%   63.14%   +2.58%     
==========================================
  Files         269      271       +2     
  Lines       11045    11265     +220     
  Branches     2690     2742      +52     
==========================================
+ Hits         6689     7113     +424     
+ Misses       3770     3537     -233     
- Partials      586      615      +29
Impacted Files Coverage Δ
packages/jest-diff/src/cleanupSemantic.ts 79.59% <ø> (ø)
packages/expect/src/toThrowMatchers.ts 86.48% <ø> (ø) ⬆️
packages/jest-diff/src/index.ts 78.04% <100%> (+0.54%) ⬆️
packages/jest-diff/src/getAlignedDiffs.ts 100% <100%> (ø)
packages/jest-diff/src/joinAlignedDiffs.ts 100% <100%> (ø)
packages/jest-diff/src/printDiffs.ts 67.24% <67.24%> (ø)
packages/jest-diff/src/diffStrings.ts 78.94% <78.94%> (-18.22%) ⬇️
packages/jest-matcher-utils/src/index.ts 80.28% <90.9%> (+11.62%) ⬆️
packages/jest-diff/src/diffLines.ts 97.03% <97.03%> (ø)
... and 5 more

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 8212e62...a053e67. Read the comment docs.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Agree with @SimenB on extracting some (especially copied) logic to jest-diff

@pedrottimark
Copy link
Contributor Author

Is the following code in jest-get-type the model to add a named export to jest-diff package?

getType.isPrimitive = (value: unknown) => Object(value) !== value;

export = getType;

@jeysal
Copy link
Contributor

jeysal commented Jun 11, 2019

Until we've migrated to default exports, yeah it probably is :/

@SimenB
Copy link
Member

SimenB commented Jun 11, 2019

Yup, until the next major

@pedrottimark
Copy link
Contributor Author

Thank y’all for suggestion about packaging. Here is minimal interface for now:

  • jest-diff exports getStringDiff for single or multiple line strings
  • jest-matcher-utils imports it and exports printDiffOrStringify to matchers

In part 3, snapshot matchers also call printDiffOrStringify if received is string

@pedrottimark
Copy link
Contributor Author

Faithful reviewers, moving files and making changes confused Changed Files:

  • Renamed diffLines.ts has minor changes to factor out createPatchMark and printAnnotation
  • Moved diffStrings.ts which collides with former name of the renamed file has no real changes

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants