Jest diff numbers and booleans

[jeysal]

Summary

Currently, jest-diff returns null when diffing numbers or booleans (or NO_DIFF_MESSAGE if they are equal).
This is because the matchers that use jest-diff already print something like

Expected: 2
Received: 1

if they differ, so the jest-diff output would be redundant.

This behavior is unintuitive if you are using jest-diff for something other than Jest's test failure output.
I stumbled upon this trying to build a diff-like CLI for jest-diff.
It was also quite inexplicable to me when I first read the jest-diff code ("why does it just not do anything if a and b are numbers?"), so I think this piece of code needed clarification in general.

This PR changes jest-diff to behave like normal diffing by default, with an omitTrivial option that the matchers use to tell jest-diff not to generate a diff between numbers or booleans.

Test plan

Added tests for numbers and booleans to diff.test.
e2e tests unchanged, confirming that this does not change the overall behavior of Jest.
One exception: Along the way, I also fixed the following contradictory output :D

Expected: 0
Received: -0

Difference:

Compared values have no visual difference.

[SimenB]

Thanks again @jeysal! @pedrottimark how does this fit into #6961 and #7557?

[jeysal]

Yeah I saw those two open PRs too - might have small merge conflicts but it's semantically different concerns so I think we should be fine.

[pedrottimark]

Definite yes to the change from === operator to Object.is call.

The exceptions of null and the two message strings as return values seem like too close coupling between jest-diff and expect packages. I like the idea of options for a CLI utility as a way to think about what to do instead. To help me comment more effectively:

@jeysal can you summarize the pros and cons of adding a new option to keep the current behavior (therefore a breaking change) versus to give the improvement (therefore opt-in and not a breaking change). According to npm there are 88 dependents.

EDIT: To clarify a possible misunderstanding, I don’t mean that users of CLI tool shouldn’t need to specify an option to get desired behavior.

[jeysal]

@jeysal can you summarize the pros and cons of adding a new option to keep the current behavior (therefore a breaking change) versus to give the improvement (therefore opt-in and not a breaking change). According to npm there are 88 dependents.

Con - the breaking change 😄
Pro

• It's super confusing that it returns null for numbers, especially given that there is no documentation.
  • In consequence, I don't think many dependents rely on this behavior. Most will probably be surprised to hear that jest-diff does nothing useful for numbers and booleans.
• As mentioned in the OP, I also think that it "cleans up" the Jest codebase itself (despite adding more code). Just the existence of {omitTrivial: true} in the calls and if (omitTrivial) in the impl leads you to look up what the option does in DiffOptions instead of wondering why these strange exceptions exist.

[jeysal]

On the too close coupling:
I agree that this is currently not ideal. Giving jest-diff a more reasonable default behavior like this PR does would reduce the coupling a little IMO.
Alternatively:
Maybe we should just have expect (and other jest-diff consumers) not call diff at all if they're dealing with something like number comparisons?

[thymikee]

Maybe we should just have expect (and other jest-diff consumers) not call diff at all if they're dealing with something like number comparisons?

I think in the long run we want jest-diff to be able to produce a diff with highlights, which would be nice when comparing bigger numbers/strings :)

[jeysal]

I think in the long run we want jest-diff to be able to produce a diff with highlights, which would be nice when comparing bigger numbers/strings :)

Yeah that would be cool - then we could remove the special handling altogether. Do you think that as it is the number/boolean special handling should be in jest-diff behind omitTrivial or in expect etc. where the "Expected x to be y" is printed and thus the redundant output created?

[thymikee]

I think I'm leaning towards the no-omitTrivial scenario where expect decides about how to show the difference. omitTrivial seems hacky and it's only to facilitate current behavior, which is not ideal. I'm cool however with using this as a temporary solution and adjust matchers later.

With inline highlights we could have the output from the matcher like this:

expected(received).toBe(expected)

Expected: 1234567894123 // green with highlights
Received: 1230061194132 // red   with highlights

since it would know we only have 2 single-line values to compare which would be so awesome.

Anyway, @pedrottimark has it probably all figured out couple of months ahead, so he's the best person to answer

[jeysal]

Yeah I think that would be better as well. If we go this route, probably best to do it now though, otherwise we have a breaking change again when we remove omitTrivial. Shouldn't be that hard to implement, unless @pedrottimark sees a problem with that approach that I missed 😄

[codecov-io]

Codecov Report

Merging #7605 into master will increase coverage by 0.01%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #7605      +/-   ##
==========================================
+ Coverage   68.26%   68.27%   +0.01%     
==========================================
  Files         249      249              
  Lines        9623     9627       +4     
  Branches        6        6              
==========================================
+ Hits         6569     6573       +4     
  Misses       3052     3052              
  Partials        2        2

Impacted Files	Coverage Δ
packages/jest-circus/src/formatNodeAssertErrors.js	18.18% <ø> (ø)	⬆️
packages/jest-snapshot/src/index.js	52.63% <ø> (ø)	⬆️
packages/jest-diff/src/diffStrings.js	98.52% <ø> (ø)	⬆️
packages/jest-jasmine2/src/jasmine/Spec.js	0% <0%> (ø)	⬆️
packages/jest-jasmine2/src/jasmine/Env.js	0% <0%> (ø)	⬆️
packages/expect/src/matchers.js	99.43% <100%> (ø)	⬆️
packages/expect/src/spyMatchers.js	97.44% <100%> (ø)	⬆️
packages/jest-diff/src/index.js	84.09% <100%> (+1.59%)	⬆️

---

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 f425bd4...23dc84f. Read the comment docs.

[SimenB]

+1 to not add a omitTrivial. Would be awesome to land in time for Jest 24, which will (barring anything unforeseen happening) happen this week.

[jeysal]

Okay, I'll try to build the required expect behavior in time for Jest 24.

[SimenB]

We can regress a tiny bit on the output from expect and solve that in a patch release in a week, if it turns out to be time consuming. But the api change in jest-diff needs to land before that 🙂

(preferable would be for both to land at the same time, of course)

[pedrottimark]

Thank you for patience while this simmered in my mental slow cooker.

I will share some thoughts this afternoon.

For long view, see #7603 (comment)

[jeysal]

I've removed all things omitTrivial and instead built a shouldPrintDiff function in jest-matcher-utils that tells expect not to print a diff when comparing two numbers or two booleans (and that could be extended if we want to omit the diff on other pairs of values in the future).
Would appreciate if everyone involved takes a look at this new implementation :)

[jeysal]

Oh right, node assert...

[SimenB]

Node assert code should probably live in jest-matcher-utils instead of being duplicated across jest-jasmine and jest-circus. Maybe for another PR, though 🙂

[jeysal]

Given that jest-jasmine2 assertionErrorMessage.js and jest-circus formatNodeAssertErrors.js will also need

shouldPrintDiff(a, b) ? jestDiff(a, b, options) : null

, maybe extracting it into jest-matcher-utils is not so bad after all, even though it would add the jest-matcher-utils => jest-diff dependency?
Anyone opposed to adding the dependency? jest-matcher-utils already has
https://github.com/facebook/jest/blob/198282580f3aa9cf87465e208dc514f3d7a4546e/packages/jest-matcher-utils/package.json#L15-L19

[SimenB]

I'm fine with that as a dep, seems natural that a matcher utility can also do diffing

[jeysal]

Done. This also removed two "Compared values have no visual difference."s in failures.test.js, but that looks alright to me

[jeysal]

Also using the diff from jest-matcher-utils in spyMatchers.
The only remaining direct consumer of jest-diff is now jest-snapshot.

[SimenB]

This is great, thank you!

[pedrottimark]

@jeysal Brilliant solution to make the change available to other dependents!

Look forward to collaborating on future improvements to reports in Jest, if you are interested :)

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