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

Update toEqual failure message #7325

Merged
merged 8 commits into from
Jan 9, 2019
Merged

Update toEqual failure message #7325

merged 8 commits into from
Jan 9, 2019

Conversation

phapp88
Copy link
Contributor

@phapp88 phapp88 commented Nov 3, 2018

Summary

This pull request updates the failure message returned by toEqual so that it is identical to the improved message shown in #7105. The changes made are similar to the changes made in #7224.

Test plan

The matcher is tested with a snapshot test.

2
Received:
1

Copy link
Contributor

Choose a reason for hiding this comment

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

This report which doesn’t display expected and received values is evidence for request to remove && !oneline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making the change you suggested did not have any effect on this snapshot. I believe that is because jest-diff returns null if the type of the expected value is number and two numbers are being compared for this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

CI failed because this snapshot (which now has added lines we wanted) wasn’t updated in committed files:

Expected: 2
Received: 1

` ${printReceived(received)}` +
matcherHint('.toEqual', undefined, undefined, {
isNot: this.isNot,
}) +
(diffString && !oneline ? `\n\nDifference:\n\n${diffString}` : '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove && !oneline so the report displays expected and received values even if one line.

@pedrottimark
Copy link
Contributor

@phapp88 Thank you. Please forgive the delay to review your work. I requested one change.

@codecov-io
Copy link

Codecov Report

Merging #7325 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7325      +/-   ##
==========================================
- Coverage   66.64%   66.64%   -0.01%     
==========================================
  Files         241      241              
  Lines        9345     9344       -1     
  Branches        6        5       -1     
==========================================
- Hits         6228     6227       -1     
  Misses       3114     3114              
  Partials        3        3
Impacted Files Coverage Δ
packages/expect/src/matchers.js 99.37% <100%> (-0.01%) ⬇️

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 9812e1b...d316498. Read the comment docs.

@pedrottimark
Copy link
Contributor

pedrottimark commented Nov 19, 2018

Yes, my bad, jest-diff does return one-line diff for string args, but null for number or boolean.

expect('received').toEqual('expected'); // diffString is string
expect(1).toEqual(0); // diffString is null
expect(true).toEqual(false); // diffString is null

Therefore, it looks like we need the following EDIT in the second message function:

return (
  matcherHint('.toEqual', undefined, undefined, {
    isNot: this.isNot,
  }) +
  '\n\n' +
  (diffString
    ? `Difference:\n\n${diffString}`
    : `Expected: ${printExpected(expected)}\n` +
      `Received: ${printReceived(received)}`)
);

<green>-0</>
Received:
<red>0</>

Difference:

<dim>Compared values have no visual difference.</>"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is because jest-diff use strict equality === operator when Object.is might be better.

That would be a separate pull request.

Copy link
Member

Choose a reason for hiding this comment

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

we should land that on master, then rebase this, right? To ensure the message is good

<green>ArrayContaining [1, 2]</>
Received:
<red>1</>

Difference:

Comparing two different types of values. Expected <green>array</> but received <red>number</>."
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a problem though.

@pedrottimark
Copy link
Contributor

@phapp88 Thank you for your patient follow through. It might be until Wednesday before my mental slow cooker has suggestion what to do about edge cases from jest-diff package.

@phapp88
Copy link
Contributor Author

phapp88 commented Nov 19, 2018

@pedrottimark No problem. I'd be willing to submit other pull requests related to jest-diff to help resolve these issues.

@pedrottimark
Copy link
Contributor

pedrottimark commented Nov 20, 2018

Here is another condition to display Expected and Received values when jest-diff returns:

  • Compared values have no visual difference.
  • Comparing two different types of values.

EDIT incorrect: RegExp because node.green displays String.prototype.includes as red for early 6.x versions

Not anchored at start because of escape sequences for green and red color.

const diffString = diff(expected, received, {expand: this.expand});

return (
  matcherHint('.toEqual', undefined, undefined, {
    isNot: this.isNot,
  }) +
  '\n\n' +
  (diffString && diffString.includes('- Expect')
    ? `Difference:\n\n${diffString}`
    : `Expected: ${printExpected(expected)}\n` +
      `Received: ${printReceived(received)}`)
);

The memory is coming back: these tedious details are why this matcher was left until later :)

@SimenB
Copy link
Member

SimenB commented Nov 20, 2018

expect(1).toEqual(0); // diffString is null
expect(true).toEqual(false); // diffString is null

That's a bug, no?

@pedrottimark
Copy link
Contributor

@SimenB hard to say if bug or feature: jest-diff package returns null for some edge cases:

  • number
  • boolean
  • asymmetric matchers

I got stuck at the beginning of 2018 thinking about consequences of changing its contract, so now am looking for ways to move forward on expect package, despite the tighter-than-ideal coupling in my suggestions above.

By the way, can you give a second opinion about https://github.com/facebook/jest/blob/master/packages/jest-diff/src/index.js#L53-L55

I think instead of a === b it should be Object.is(a, b) to deal with 0/-0 and NaN, true?

@SimenB
Copy link
Member

SimenB commented Nov 20, 2018

I think instead of a === b it should be Object.is(a, b) to deal with 0/-0 and NaN, true?

Yes 🙂

@SimenB
Copy link
Member

SimenB commented Nov 20, 2018

RegExp because node.green displays String.prototype.includes as red for early 6.x versions

It's been in Node since v4? https://node.green/#ES2015-built-in-extensions-String-prototype-methods-String-prototype-includes

@pedrottimark
Copy link
Contributor

You are correct. My bad, I think I misunderstood a table row under well-known symbols.


Difference:

<dim>Compared values have no visual difference.</>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Super that the work-around fixed this existing problem.


Difference:

Comparing two different types of values. Expected <green>array</> but received <red>number</>."
Copy link
Contributor

Choose a reason for hiding this comment

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

Bravo, also fixed this problem.

`Received:\n` +
` ${printReceived(received)}` +
(diffString && !oneline ? `\n\nDifference:\n\n${diffString}` : '')
(diffString && diffString.includes('- Expect')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pedrottimark it's not particular fault of this PR, but generally why do we show diffString in this branch, but not on the other? Looking at the snapshots and seeing the diff sometimes shown for one-liners and sometimes not is weird :|

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question about the asymmetry: first branch means that .not.toBe fails, therefore not likely to have differences. For some other matchers the expected might be asymmetric.

<red>+ Received</>

<green>- apple</>
<red>+ banana</>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be shown as:

Expected: apple
Received: banana

?
It's weird seeing the diff for single lines, when Expected/Received fits better for this purpose

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the reason is maybe in the future to display substring differences.

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.

Most of the diff looks better, so I'm keen on getting this to master and iterate on the edge cases causing showing the full "Difference: " message for single-line values

@pedrottimark pedrottimark merged commit 6cc2a85 into jestjs:master Jan 9, 2019
@pedrottimark
Copy link
Contributor

@phapp88 thank you for contributing to Jest

@phapp88 phapp88 deleted the update-toEqual branch January 9, 2019 21:57
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
* Update toEqual failure message

* Update changelog

* Update failing snapshots

* Show one line reports

* Update failure message when diffString is null

* Update failing snapshot

* Update failure message for jest-diff edge cases
@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 12, 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.

6 participants