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

assert: use Same-value equality in deepStrictEqual #15398

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
9 changes: 9 additions & 0 deletions doc/api/assert.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ parameter is an instance of an `Error` then it will be thrown instead of the
<!-- YAML
added: v1.2.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/REPLACEME
description: zero is now compared using the [Object.is][] comparison.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The fact that Object.is() is used under the hood is probably not of interest to the end user compared to what the impact is for the end user. Like, if the big change is that +0 and -0 are no longer considered equal, maybe let's say that instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15036
description: NaN is now compared using the [SameValueZero][] comparison.
Expand Down Expand Up @@ -144,6 +147,7 @@ Generally identical to `assert.deepEqual()` with a few exceptions:
the [Strict Equality Comparison][] too.
3. [Type tags][Object.prototype.toString()] of objects should be the same.
4. [Object wrappers][] are compared both as objects and unwrapped values.
5. `0` and `-0` are not considered equal.

```js
const assert = require('assert');
Expand Down Expand Up @@ -181,6 +185,11 @@ assert.deepStrictEqual(new Number(1), new Number(2));
// Fails because the wrapped number is unwrapped and compared as well.
assert.deepStrictEqual(new String('foo'), Object('foo'));
// OK because the object and the string are identical when unwrapped.

assert.deepStrictEqual(-0, -0);
// OK
assert.deepStrictEqual(0, -0);
// AssertionError: 0 deepStrictEqual -0
```

If the values are not equal, an `AssertionError` is thrown with a `message`
Expand Down
26 changes: 14 additions & 12 deletions lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,13 @@ function areSimilarRegExps(a, b) {
// For small buffers it's faster to compare the buffer in a loop. The c++
// barrier including the Uint8Array operation takes the advantage of the faster
// binary compare otherwise. The break even point was at about 300 characters.
function areSimilarTypedArrays(a, b) {
// NOTE: Floats should only use binary comparison in non strict mode!
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Will this comment be clear to implementers? Would it be better to remove it and let tests catch any errors introduced by maintainers?

Copy link
Member

Choose a reason for hiding this comment

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

Likely

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I removed the comment and the tests already include cases for this, so there is nothing to add anymore.

function areSimilarTypedArrays(a, b, max) {
const len = a.byteLength;
if (len !== b.byteLength) {
return false;
}
if (len < 300) {
if (len < max) {
for (var offset = 0; offset < len; offset++) {
if (a[offset] !== b[offset]) {
return false;
Expand Down Expand Up @@ -160,10 +161,7 @@ function isObjectOrArrayTag(tag) {
// reasonable to interpret their underlying memory in the same way,
// which is checked by comparing their type tags.
// (e.g. a Uint8Array and a Uint16Array with the same memory content
// could still be different because they will be interpreted differently)
// Never perform binary comparisons for Float*Arrays, though,
// since e.g. +0 === -0 is true despite the two values' bit patterns
// not being identical.
// could still be different because they will be interpreted differently).
//
// For strict comparison, objects should have
// a) The same built-in type tags
Expand Down Expand Up @@ -211,8 +209,9 @@ function strictDeepEqual(actual, expected) {
if (actual.message !== expected.message) {
return false;
}
} else if (!isFloatTypedArrayTag(actualTag) && ArrayBuffer.isView(actual)) {
if (!areSimilarTypedArrays(actual, expected)) {
} else if (ArrayBuffer.isView(actual)) {
if (!areSimilarTypedArrays(actual, expected,
isFloatTypedArrayTag(actualTag) ? 0 : 300)) {
return false;
}
// Buffer.compare returns true, so actual.length === expected.length
Expand Down Expand Up @@ -266,9 +265,10 @@ function looseDeepEqual(actual, expected) {
const actualTag = objectToString(actual);
const expectedTag = objectToString(expected);
if (actualTag === expectedTag) {
if (!isObjectOrArrayTag(actualTag) && !isFloatTypedArrayTag(actualTag) &&
ArrayBuffer.isView(actual)) {
return areSimilarTypedArrays(actual, expected);
if (!isObjectOrArrayTag(actualTag) && ArrayBuffer.isView(actual)) {
return areSimilarTypedArrays(actual, expected,
isFloatTypedArrayTag(actualTag) ?
Infinity : 300);
}
// Ensure reflexivity of deepEqual with `arguments` objects.
// See https://github.com/nodejs/node-v0.x-archive/pull/7178
Expand All @@ -280,7 +280,9 @@ function looseDeepEqual(actual, expected) {
function innerDeepEqual(actual, expected, strict, memos) {
// All identical values are equivalent, as determined by ===.
if (actual === expected) {
return true;
if (actual !== 0)
return true;
return strict ? Object.is(actual, expected) : true;
}

// Returns a boolean if (not) equal and undefined in case we have to check
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-assert-deep.js
Original file line number Diff line number Diff line change
Expand Up @@ -507,5 +507,8 @@ assert.doesNotThrow(
boxedSymbol.slow = true;
assertNotDeepOrStrict(boxedSymbol, {});
}
// Minus zero
assertOnlyDeepEqual(0, -0);
assertDeepAndStrictEqual(-0, -0);

/* eslint-enable */
28 changes: 23 additions & 5 deletions test/parallel/test-assert-typedarray-deepequal.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,20 @@ const equalArrayPairs = [
[new Int32Array(1e5), new Int32Array(1e5)],
[new Float32Array(1e5), new Float32Array(1e5)],
[new Float64Array(1e5), new Float64Array(1e5)],
[new Int16Array(256), new Uint16Array(256)],
[new Int16Array([256]), new Uint16Array([256])],
[new Float32Array([+0.0]), new Float32Array([-0.0])],
[new Float64Array([+0.0]), new Float32Array([-0.0])],
[new Float64Array([+0.0]), new Float64Array([-0.0])],
[new Float32Array([+0.0]), new Float32Array([+0.0])],
[new Uint8Array([1, 2, 3, 4]).subarray(1), new Uint8Array([2, 3, 4])],
[new Uint16Array([1, 2, 3, 4]).subarray(1), new Uint16Array([2, 3, 4])],
[new Uint32Array([1, 2, 3, 4]).subarray(1, 3), new Uint32Array([2, 3])]
];

const looseEqualArrayPairs = [
[new Float64Array([+0.0]), new Float32Array([-0.0])],
[new Int16Array(256), new Uint16Array(256)],
[new Int16Array([256]), new Uint16Array([256])],
[new Float32Array([+0.0]), new Float32Array([-0.0])],
[new Float64Array([+0.0]), new Float64Array([-0.0])]
];

const notEqualArrayPairs = [
[new Uint8Array(2), new Uint8Array(3)],
[new Uint8Array([1, 2, 3]), new Uint8Array([4, 5, 6])],
Expand All @@ -46,6 +50,16 @@ const notEqualArrayPairs = [
equalArrayPairs.forEach((arrayPair) => {
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(arrayPair[0], arrayPair[1]);
assert.deepStrictEqual(arrayPair[0], arrayPair[1]);
});

looseEqualArrayPairs.forEach((arrayPair) => {
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(arrayPair[0], arrayPair[1]);
assert.throws(
makeBlock(assert.deepStrictEqual, arrayPair[0], arrayPair[1]),
assert.AssertionError
);
});

notEqualArrayPairs.forEach((arrayPair) => {
Expand All @@ -54,4 +68,8 @@ notEqualArrayPairs.forEach((arrayPair) => {
makeBlock(assert.deepEqual, arrayPair[0], arrayPair[1]),
assert.AssertionError
);
assert.throws(
makeBlock(assert.deepStrictEqual, arrayPair[0], arrayPair[1]),
assert.AssertionError
);
});