toStrictEqual fails to distinguish 0 from 5e-324

[dubzzz]

🐛 Bug Report

While trying to automate the detection of bugs and regressions in Jest (see my previous PR #7938), I found a very strange failing case in toStrictEqual:

expect(0).not.toStrictEqual(5e-324) // throws

//    expect(received).not.toStrictEqual(expected)
//
//    Expected: 5e-324
//    Received: 0

To Reproduce

Steps to reproduce the behavior: expect(0).not.toStrictEqual(5e-324)

Expected behavior

I would have expect Jest to succeed to distinguish 0 from 5e-324.

Run npx envinfo --preset jest

Paste the results here:

$ npx envinfo --preset jest
npx : 1 installé(s) en 4.679s

  System:
    OS: Windows 10
    CPU: (8) x64 Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
  Binaries:
    Node: 10.12.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.10.1 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 6.7.0 - C:\Program Files\nodejs\npm.CMD

[dubzzz]

The native assert of node does consider 0 and 5e-304 as different values:

const assert = require('assert');
assert.notStrictEqual(0, 5e-324);

[pedrottimark]

Good catch. Here is my confirmation and quick analysis:

describe('MIN_VALUE', () => {
  const min = Number.MIN_VALUE;

  test('Object.is for toBe', () => {
    expect(Object.is(0, min)).toBe(false);
  });

  describe('egal for isEqual or isStrictEqual', () => {
    const egal = (a, b) => a != +a ? b != +b : a === 0 ? 1 / a == 1 / b : a == +b;

    test('min received', () => {
      expect(egal(min, 0)).toBe(false);
    });
    test('min expected', () => {
      expect(egal(0, min)).toBe(true); // incorrect and inconsistent
    });
  });
});

@dubzzz Can you research the egal comparison at: https://github.com/facebook/jest/blob/master/packages/expect/src/jasmineUtils.ts#L108-L111

• is it copied correctly from what you consider to be an authoritative source?
• does a source claim that egal is symmetric?

Regardless of the reason for the problem, we can replace it with Object.is(a, b) true?

[dubzzz]

My personal feeling is that equal should be symmetric. If I am not wrong both == and === are symmetric in JavaScript (but not transitive).

In other languages, such as Java we can read the following, equals [...] is symmetric [source].

Object.is would have been a great candidate but unfortunately it does not seem to be compatible with IE so it might be an issue for Jest [source].

@pedrottimark If you are OK with it, I will try to issue a new PR to fix the lines https://github.com/facebook/jest/blob/master/packages/expect/src/jasmineUtils.ts#L108-L111

[dubzzz]

Here is a quick investigation I just did:

function compare(a, b) {
  // a and b are such that:
  // - Object.prototype.toString.call(a) -> [object Number]
  // - Object.prototype.toString.call(b) -> [object Number]
  if (a != +a)               // if a is Number.NaN
    return b != +b;          //   return b is Number.NaN
  if (a === 0)               // if a is 0 (or -0)
    return 1 / a == 1 / b;   //   return (a and b are -0) or (a and b are 0)
  return a == +b;            // return a == +b
}

The code above is the one at https://github.com/facebook/jest/blob/master/packages/expect/src/jasmineUtils.ts#L108-L111 with ternary operators changed into classical if-else.

Problem in the second if:

1 / 0                === Infinity
1 / Number.MIN_VALUE === Infinity

[pedrottimark]

Good point about Object.is yet we have already started to use it, even a few lines upstream: https://github.com/facebook/jest/blob/master/packages/expect/src/jasmineUtils.ts#L90-L92

[dubzzz]

We should maybe just get rid off the case '[object Number]' and replace it by a simple return false because truthy cases are already covered by https://github.com/facebook/jest/blob/master/packages/expect/src/jasmineUtils.ts#L90-L92

[pedrottimark]

Notice that if statement returns if true so the switch statement is providing type-specific criteria for not true. What do you think about bring together the following cases:

case '[object String]':
case '[object Number]':
case '[object Boolean]':
  return Object.is(a, b);

EDIT: return false; might work with current code, but it seems too brittle and subtle

[SimenB]

I'm fine with people needing to polyfill Object.is, fwiw

[dubzzz]

Indeed, I think it would be a good thing to factorize those cases in a single one.

I believe we could also add [object Date] but I am not 100% sure for this one.

If we do the Object.is under this case (number + boolean + string) we can remove the one at lines 90 to 92 and replace it by a simple === for others.

[pedrottimark]

Be careful about Date objects because they needs value comparison instead of object identity.

While we are looking at it, can you take a careful look at symbols, because I don’t see a case for them? Future readers will thank us, if we can bring together logic for the primitive types.

[dubzzz]

@pedrottimark In reality it seems that moving to Object.is will change the current behaviour of the following cases:

// Today:
a = new String('5'); b = '5'; a == String(b); // -> is true
a = new Number(0); b = 0; a != +a ? b != +b : a === 0 && b === 0 ? 1 / a == 1 / b : a == +b; // -> is true
a = new Boolean(true); b = true; +a == +b; +a == +b; // -> is true

// With Object.is:
a = new String('5'); b = '5'; Object.is(a, b); // -> is false
a = new Number(0); b = 0; Object.is(a, b); // -> is false
a = new Boolean(true); b = true; +a == +b; Object.is(a, b); // -> is false

EDIT: tested on Firefox 66.0b9 (64 bits)

[pedrottimark]

Oh yeah, good point, I messed up and tested Number(0) but not new Number(0)

But we can use Number(…) as type cast for Object.is comparison?

That raises a different question in my mind is return a == String(b); symmetrical?

[dubzzz]

Good point for the string case ^^

I just tried to check and it seems ok (no failing assert below):

const assert = require('assert');
const cmp = (a, b) => a == String(b);
assert.ok(cmp('', ''));
assert.ok(cmp('', String('')));
assert.ok(cmp('', new String('')));
assert.ok(cmp(String(''), ''));
assert.ok(cmp(String(''), String('')));
assert.ok(cmp(String(''), new String('')));
assert.ok(cmp(new String(''), ''));
assert.ok(cmp(new String(''), String('')));
assert.ok(cmp(new String(''), new String('')));

Another problem I just spotted is the a === 0 in the https://github.com/facebook/jest/blob/master/packages/expect/src/jasmineUtils.ts#L108-L111 which will not work well if a = new Number(0)

[pedrottimark]

Thank you for walking through this together. Working on Jest is a deep dive into corners of ECMAScript.

Here is a step back to candidate minimal solution taking instances into account:

case '[object Number]':
  return Object.is(Number(a), Number(b));

[dubzzz]

Review updated accordingly.
I don't know how to deal with the failing tests (snapshot tests)

[pedrottimark]

Thank you. I have enjoyed the interaction. I wrote a comment in PR about CI and 2 suggested tests.

[dubzzz]

Thanks a lot for your help on this issue

[github-actions]

This issue 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.