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.

Sign up for GitHub

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

Jump to bottom

assert: use same value equal for deepStrictEqual NaN #15036

Closed
wants to merge 2 commits into from
Closed

assert: use same value equal for deepStrictEqual NaN #15036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bf1af3
doc: add missing changelogs to assert docs
BridgeAR Aug 25, 2017
07e36df
assert: use Same-value eql. for deepStrictEqual NaN
BridgeAR Aug 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 48 additions & 4 deletions doc/api/assert.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ An alias of [`assert.ok()`][].
<!-- YAML
added: v0.1.21
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15001
description: Error names and messages are now properly compared
- version: v8.0.0
pr-url: https://github.com/nodejs/node/pull/12142
description: Set and Map content is also compared
Expand Down Expand Up @@ -105,7 +108,10 @@ parameter is undefined, a default error message is assigned.
added: v1.2.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/12142
pr-url: https://github.com/nodejs/node/pull/15036
description: NaN is now compared using the [SameValueZero][] comparison.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15001
description: Error names and messages are now properly compared
- version: v8.0.0
pr-url: https://github.com/nodejs/node/pull/12142
Expand All @@ -126,9 +132,10 @@ changes:

Generally identical to `assert.deepEqual()` with three exceptions:

1. Primitive values are compared using the [Strict Equality Comparison][]
( `===` ). Set values and Map keys are compared using the [SameValueZero][]
comparison. (Which means they are free of the [caveats][]).
1. Primitive values besides `NaN` are compared using the [Strict Equality
Comparison][] ( `===` ). Set and Map values, Map keys and `NaN` are compared
using the [SameValueZero][] comparison (which means they are free of the
[caveats][]).
2. [`[[Prototype]]`][prototype-spec] of objects are compared using
the [Strict Equality Comparison][] too.
3. [Type tags][Object.prototype.toString()] of objects should be the same.
Expand Down Expand Up @@ -161,6 +168,8 @@ assert.deepEqual(date, fakeDate);
assert.deepStrictEqual(date, fakeDate);
// AssertionError: 2017-03-11T14:25:31.849Z deepStrictEqual Date {}
// Different type tags
assert.deepStrictEqual(NaN, NaN);
// OK, because of the SameValueZero comparison
```

If the values are not equal, an `AssertionError` is thrown with a `message`
Expand Down Expand Up @@ -345,6 +354,22 @@ assert.ifError(new Error());
## assert.notDeepEqual(actual, expected[, message])
<!-- YAML
added: v0.1.21
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15001
description: Error names and messages are now properly compared
- version: v8.0.0
pr-url: https://github.com/nodejs/node/pull/12142
description: Set and Map content is also compared
- version: v6.4.0, v4.7.1
pr-url: https://github.com/nodejs/node/pull/8002
description: Typed array slices are handled correctly now.
- version: v6.1.0, v4.5.0
pr-url: https://github.com/nodejs/node/pull/6432
description: Objects with circular references can be used as inputs now.
- version: v5.10.1, v4.4.3
pr-url: https://github.com/nodejs/node/pull/5910
description: Handle non-`Uint8Array` typed arrays correctly.
-->
* `actual` {any}
* `expected` {any}
Expand Down Expand Up @@ -392,6 +417,25 @@ parameter is undefined, a default error message is assigned.
## assert.notDeepStrictEqual(actual, expected[, message])
<!-- YAML
added: v1.2.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15036
description: NaN is now compared using the [SameValueZero][] comparison.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15001
description: Error names and messages are now properly compared
- version: v8.0.0
pr-url: https://github.com/nodejs/node/pull/12142
description: Set and Map content is also compared
- version: v6.4.0, v4.7.1
pr-url: https://github.com/nodejs/node/pull/8002
description: Typed array slices are handled correctly now.
- version: v6.1.0
pr-url: https://github.com/nodejs/node/pull/6432
description: Objects with circular references can be used as inputs now.
- version: v5.10.1, v4.4.3
pr-url: https://github.com/nodejs/node/pull/5910
description: Handle non-`Uint8Array` typed arrays correctly.
-->
* `actual` {any}
* `expected` {any}
Expand Down
7 changes: 5 additions & 2 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,11 @@ function isObjectOrArrayTag(tag) {
// a) The same built-in type tags
// b) The same prototypes.
function strictDeepEqual(actual, expected) {
if (actual === null || expected === null ||
typeof actual !== 'object' || typeof expected !== 'object') {
if (typeof actual !== 'object') {
return typeof actual === 'number' && Number.isNaN(actual) &&
Copy link
Member

Choose a reason for hiding this comment

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

I think typeof actual === 'number' can be removed, Number.isNaN(actual) is sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct but it is faster for all other types to check for number first and if it is a number it is not a real penalty because that check is super fast.

Copy link
Member

Choose a reason for hiding this comment

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

Does it makes sense to do the same for expected then?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could but I feel like that is overdoing it as it would only apply for the case where Number.isNaN(actual), especially as I would expect expected to also be NaN in that case more often then not being of type number.

Number.isNaN(expected);
}
if (typeof expected !== 'object' || actual === null || expected === null) {
return false;
}
const actualTag = objectToString(actual);
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-assert-deep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ assertOnlyDeepEqual(
assertDeepAndStrictEqual(m3, m4);
}

// Handle sparse arrays
assertDeepAndStrictEqual([1, , , 3], [1, , , 3]);
assertOnlyDeepEqual([1, , , 3], [1, , , 3, , , ]);

Expand All @@ -481,4 +482,11 @@ assertOnlyDeepEqual([1, , , 3], [1, , , 3, , , ]);
assertOnlyDeepEqual(err1, {}, assert.AssertionError);
}

// Handle NaN
assert.throws(() => { assert.deepEqual(NaN, NaN); }, assert.AssertionError);
assert.doesNotThrow(() => { assert.deepStrictEqual(NaN, NaN); });
assert.doesNotThrow(() => { assert.deepStrictEqual({ a: NaN }, { a: NaN }); });
assert.doesNotThrow(
() => { assert.deepStrictEqual([ 1, 2, NaN, 4 ], [ 1, 2, NaN, 4 ]); });

/* eslint-enable */