Fix asymmetric equal for Number

[dubzzz]

Summary

Fixes #7941: toStrictEqual fails to distinguish 0 from 5e-324

The real problem is that the equal operation defined into jasmineUtils is not symmetric.

Test plan

Before that change, the following assertion was failing:

expect(0).not.toStrictEqual(5e-324);

Now it passes as expected.

[dubzzz]

I have not added tests as I did not know where to add them. Today the jasmineUtils file does not seem to be covered by direct tests - it is indirectly covered by tests on toStrictEqual and other expectations offered by Jest.

[dubzzz]

I would be interested into having your opinion on #7938 (comment). It suggests to add automatic test suites to detect such bugs more easily in the future.

[pedrottimark]

If a recent commit broke CI, then you might need to merge the fix from master branch tomorrow.

To answer your question about tests, I just added two cases after the first 3 in the packages/expect/src/__tests__/matchers.test.js file:

describe('.toEqual()', () => {
  [
    [true, false],
    [1, 2],
    [0, -0],
    [0, Number.MIN_VALUE], // issues/7941
    [Number.MIN_VALUE, 0],
    // and so on

Look down in the test file to see that the tuples consist of [received, expected]

For me, the first new case throws because of the error and the second writes a new snapshot.

On your branch, you can expect two new snapshots in packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap file.

[pedrottimark]

Considering the potential untested regression that you saved us (the we who is me :) from, here are 3 suggested test cases following the first 2 existing cases farther down:

  [
    [true, true],
    [1, 1],
    [NaN, NaN],
    [0, new Number(0)],
    [new Number(0), 0],
    // and so on

The add 3 new snapshots from the expected-to-fail .not.toEqual assertions.

[pedrottimark]

Off topic of this pull request, serialization in this report confused me, even if I should have known:

Expected: <green>0</>
Received: <red>{}</>

Good thing that this edge case might only even happen in Jest test suite:

expect(new Number(0)).toEqual(0);

[pedrottimark]

Thank you. Sometimes how you don’t fix it (and why not) is as important as how you do fix it :)

[SimenB]

I didn't know you could call Number with new... Should toEqual handle that and unbox the value? Calling .valueOf() seems to do the trick

[SimenB]

Looking at the MDN, this seems to be a case that toEqual should handle that toBe would fail on

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Syntax

Probably off topic

[pedrottimark]

YDKJS :) Nicolas reminded me new Number instantiates object but Number alone is type cast:

var a = new Number('123'); // a === 123 is false
var b = Number('123'); // b === 123 is true
a instanceof Number; // is true
b instanceof Number; // is false

Yes, this is an off-the-edge case in which deep equality differs from referential equality:

describe('toBe', () => {
  test('2 instances', () => {
    expect(new Number(0)).not.toBe(new Number(0));
  });
  test('instance and primitive', () => {
    expect(new Number(0)).not.toBe(0);
  });
});

EDIT: so that is reason for return Object.is(Number(a), Number(b)); in code called by toEqual

[dubzzz]

I do not reproduce the AppVeyor build failed locally. I get other issues - even on master - but not the one reported by AppVeyor.

I'll rebase the PR to keep on track with master

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@5d48312). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #7948   +/-   ##
========================================
  Coverage          ?   64.8%           
========================================
  Files             ?     256           
  Lines             ?   10047           
  Branches          ?    1464           
========================================
  Hits              ?    6511           
  Misses            ?    3218           
  Partials          ?     318

Impacted Files	Coverage Δ
packages/expect/src/jasmineUtils.ts	76.03% <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 5d48312...e9a1873. Read the comment docs.

[dubzzz]

Is there something blocking the merge of this PR?

[SimenB]

@pedrottimark feel free to merge these PRs to expect (unless you really want a second/third pair of eyes - and if so please say so) - you know the package best 🙂

[SimenB]

such a beautiful fix 😀

[pedrottimark]

@dubzzz Thank you for your contribution and your patience.

@SimenB Yes, my friend. I need to pull my weight :)

[SimenB]

I need to pull my weight :)

No danger of not doing that 😛

[dubzzz]

@pedrottimark No worry ;) Thanks a lot for the quick reviews. You gave me really useful hints concerning where the bug comes from so that I was able to fix it

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