`toStrictEqual` fails to distinguish 0 from 5e-324
dubzzz opened this issue · 18 comments
🐛 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
The native assert
of node
does consider 0 and 5e-304 as different values:
const assert = require('assert');
assert.notStrictEqual(0, 5e-324);
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?
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
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
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
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
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
I'm fine with people needing to polyfill Object.is
, fwiw
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.
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.
@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)
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?
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)
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));
Review updated accordingly.
I don't know how to deal with the failing tests (snapshot tests)
Thank you. I have enjoyed the interaction. I wrote a comment in PR about CI and 2 suggested tests.
Thanks a lot for your help on this issue
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.