Skip to content

Commit

Permalink
assert: adjust loose assertions
Browse files Browse the repository at this point in the history 
This changes the loose deep equal comparison by using the same logic
as done in the strict deep equal comparison besides comparing
primitives loosely, not comparing symbol properties and not comparing
the prototype.

`assert.deepEqual` is still commenly used and this is likely the
biggest pitfall.

Most changes are only minor and won't have a big impact besides
likely fixing user expectations.

PR-URL: nodejs#25008
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
  • Loading branch information
@BridgeAR
BridgeAR committed Jan 27, 2019
1 parent 5cb1964 commit 7493db2
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 161 deletions.
47 changes: 32 additions & 15 deletions doc/api/assert.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@ An alias of [`assert.ok()`][].
<!-- YAML
added: v0.1.21
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/25008
description: The type tags are now properly compared and there are a couple
minor comparison adjustments to make the check less surprising.
- version: v9.0.0
pr-url: https://github.com/nodejs/node/pull/15001
description: The `Error` names and messages are now properly compared
Expand Down Expand Up @@ -191,26 +195,39 @@ An alias of [`assert.deepStrictEqual()`][].

> Stability: 0 - Deprecated: Use [`assert.deepStrictEqual()`][] instead.
Tests for deep equality between the `actual` and `expected` parameters.
Primitive values are compared with the [Abstract Equality Comparison][]
( `==` ).
Tests for deep equality between the `actual` and `expected` parameters. Consider
using [`assert.deepStrictEqual()`][] instead. [`assert.deepEqual()`][] can have
potentially surprising results.

"Deep" equality means that the enumerable "own" properties of child objects
are also recursively evaluated by the following rules.

### Comparison details

* Primitive values are compared with the [Abstract Equality Comparison][]
( `==` ).
* [Type tags][Object.prototype.toString()] of objects should be the same.
* Only [enumerable "own" properties][] are considered.
* [`Error`][] names and messages are always compared, even if these are not
enumerable properties.
* [Object wrappers][] are compared both as objects and unwrapped values.
* `Object` properties are compared unordered.
* [`Map`][] keys and [`Set`][] items are compared unordered.
* Recursion stops when both sides differ or both sides encounter a circular
reference.
* Implementation does not test the [`[[Prototype]]`][prototype-spec] of
objects.
* [`Symbol`][] properties are not compared.
* [`WeakMap`][] and [`WeakSet`][] comparison does not rely on their values.

Only [enumerable "own" properties][] are considered. The
[`assert.deepEqual()`][] implementation does not test the
[`[[Prototype]]`][prototype-spec] of objects or enumerable own [`Symbol`][]
properties. For such checks, consider using [`assert.deepStrictEqual()`][]
instead. [`assert.deepEqual()`][] can have potentially surprising results. The
following example does not throw an `AssertionError` because the properties on
the [`RegExp`][] object are not enumerable:
The following example does not throw an `AssertionError` because the primitives
are considered equal by the [Abstract Equality Comparison][] ( `==` ).

```js
// WARNING: This does not throw an AssertionError!
assert.deepEqual(/a/gi, new Date());
assert.deepEqual('+00000000', false);
```

An exception is made for [`Map`][] and [`Set`][]. `Map`s and `Set`s have their
contained items compared too, as expected.

"Deep" equality means that the enumerable "own" properties of child objects
are evaluated also:

Expand Down Expand Up @@ -304,7 +321,7 @@ are recursively evaluated also by the following rules.
* Enumerable own [`Symbol`][] properties are compared as well.
* [Object wrappers][] are compared both as objects and unwrapped values.
* `Object` properties are compared unordered.
* `Map` keys and `Set` items are compared unordered.
* [`Map`][] keys and [`Set`][] items are compared unordered.
* Recursion stops when both sides differ or both sides encounter a circular
reference.
* [`WeakMap`][] and [`WeakSet`][] comparison does not rely on their values. See
Expand Down
153 changes: 47 additions & 106 deletions lib/internal/util/comparisons.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ const {
isStringObject,
isBooleanObject,
isBigIntObject,
isSymbolObject
isSymbolObject,
isFloat32Array,
isFloat64Array
} = require('internal/util/types');
const {
getOwnNonIndexProperties,
Expand Down Expand Up @@ -84,18 +86,6 @@ function areEqualArrayBuffers(buf1, buf2) {
compare(new Uint8Array(buf1), new Uint8Array(buf2)) === 0;
}

function isFloatTypedArrayTag(tag) {
return tag === '[object Float32Array]' || tag === '[object Float64Array]';
}

function isArguments(tag) {
return tag === '[object Arguments]';
}

function isObjectOrArrayTag(tag) {
return tag === '[object Array]' || tag === '[object Object]';
}

function isEqualBoxedPrimitive(val1, val2) {
if (isNumberObject(val1)) {
return isNumberObject(val2) &&
Expand Down Expand Up @@ -132,23 +122,45 @@ function isEqualBoxedPrimitive(val1, val2) {
// For strict comparison, objects should have
// a) The same built-in type tags
// b) The same prototypes.
function strictDeepEqual(val1, val2, memos) {
if (typeof val1 !== 'object') {
return typeof val1 === 'number' && numberIsNaN(val1) &&
numberIsNaN(val2);

function innerDeepEqual(val1, val2, strict, memos) {
// All identical values are equivalent, as determined by ===.
if (val1 === val2) {
if (val1 !== 0)
return true;
return strict ? objectIs(val1, val2) : true;
}
if (typeof val2 !== 'object' || val1 === null || val2 === null) {
return false;

// Check more closely if val1 and val2 are equal.
if (strict) {
if (typeof val1 !== 'object') {
return typeof val1 === 'number' && numberIsNaN(val1) &&
numberIsNaN(val2);
}
if (typeof val2 !== 'object' || val1 === null || val2 === null) {
return false;
}
if (getPrototypeOf(val1) !== getPrototypeOf(val2)) {
return false;
}
} else {
if (val1 === null || typeof val1 !== 'object') {
if (val2 === null || typeof val2 !== 'object') {
// eslint-disable-next-line eqeqeq
return val1 == val2;
}
return false;
}
if (val2 === null || typeof val2 !== 'object') {
return false;
}
}
const val1Tag = objectToString(val1);
const val2Tag = objectToString(val2);

if (val1Tag !== val2Tag) {
return false;
}
if (getPrototypeOf(val1) !== getPrototypeOf(val2)) {
return false;
}
if (Array.isArray(val1)) {
// Check for sparse arrays and general fast path
if (val1.length !== val2.length) {
Expand All @@ -159,10 +171,10 @@ function strictDeepEqual(val1, val2, memos) {
if (keys1.length !== keys2.length) {
return false;
}
return keyCheck(val1, val2, kStrict, memos, kIsArray, keys1);
return keyCheck(val1, val2, strict, memos, kIsArray, keys1);
}
if (val1Tag === '[object Object]') {
return keyCheck(val1, val2, kStrict, memos, kNoIterator);
return keyCheck(val1, val2, strict, memos, kNoIterator);
}
if (isDate(val1)) {
if (dateGetTime(val1) !== dateGetTime(val2)) {
Expand All @@ -174,13 +186,16 @@ function strictDeepEqual(val1, val2, memos) {
}
} else if (isNativeError(val1) || val1 instanceof Error) {
// Do not compare the stack as it might differ even though the error itself
// is otherwise identical. The non-enumerable name should be identical as
// the prototype is also identical. Otherwise this is caught later on.
if (val1.message !== val2.message) {
// is otherwise identical.
if (val1.message !== val2.message || val1.name !== val2.name) {
return false;
}
} else if (isArrayBufferView(val1)) {
if (!areSimilarTypedArrays(val1, val2)) {
if (!strict && (isFloat32Array(val1) || isFloat64Array(val1))) {
if (!areSimilarFloatArrays(val1, val2)) {
return false;
}
} else if (!areSimilarTypedArrays(val1, val2)) {
return false;
}
// Buffer.compare returns true, so val1.length === val2.length. If they both
Expand All @@ -191,84 +206,25 @@ function strictDeepEqual(val1, val2, memos) {
if (keys1.length !== keys2.length) {
return false;
}
return keyCheck(val1, val2, kStrict, memos, kNoIterator, keys1);
return keyCheck(val1, val2, strict, memos, kNoIterator, keys1);
} else if (isSet(val1)) {
if (!isSet(val2) || val1.size !== val2.size) {
return false;
}
return keyCheck(val1, val2, kStrict, memos, kIsSet);
return keyCheck(val1, val2, strict, memos, kIsSet);
} else if (isMap(val1)) {
if (!isMap(val2) || val1.size !== val2.size) {
return false;
}
return keyCheck(val1, val2, kStrict, memos, kIsMap);
return keyCheck(val1, val2, strict, memos, kIsMap);
} else if (isAnyArrayBuffer(val1)) {
if (!areEqualArrayBuffers(val1, val2)) {
return false;
}
} else if (isBoxedPrimitive(val1) && !isEqualBoxedPrimitive(val1, val2)) {
return false;
}
return keyCheck(val1, val2, kStrict, memos, kNoIterator);
}

function looseDeepEqual(val1, val2, memos) {
if (val1 === null || typeof val1 !== 'object') {
if (val2 === null || typeof val2 !== 'object') {
// eslint-disable-next-line eqeqeq
return val1 == val2;
}
return false;
}
if (val2 === null || typeof val2 !== 'object') {
return false;
}
const val1Tag = objectToString(val1);
const val2Tag = objectToString(val2);
if (val1Tag === val2Tag) {
if (isObjectOrArrayTag(val1Tag)) {
return keyCheck(val1, val2, kLoose, memos, kNoIterator);
}
if (isArrayBufferView(val1)) {
if (isFloatTypedArrayTag(val1Tag)) {
return areSimilarFloatArrays(val1, val2);
}
return areSimilarTypedArrays(val1, val2);
}
if (isDate(val1) && isDate(val2)) {
return val1.getTime() === val2.getTime();
}
if (isRegExp(val1) && isRegExp(val2)) {
return areSimilarRegExps(val1, val2);
}
if (val1 instanceof Error && val2 instanceof Error) {
if (val1.message !== val2.message || val1.name !== val2.name)
return false;
}
// Ensure reflexivity of deepEqual with `arguments` objects.
// See https://github.com/nodejs/node-v0.x-archive/pull/7178
} else if (isArguments(val1Tag) || isArguments(val2Tag)) {
return false;
}
if (isSet(val1)) {
if (!isSet(val2) || val1.size !== val2.size) {
return false;
}
return keyCheck(val1, val2, kLoose, memos, kIsSet);
} else if (isMap(val1)) {
if (!isMap(val2) || val1.size !== val2.size) {
return false;
}
return keyCheck(val1, val2, kLoose, memos, kIsMap);
} else if (isSet(val2) || isMap(val2)) {
return false;
}
if (isAnyArrayBuffer(val1) && isAnyArrayBuffer(val2)) {
if (!areEqualArrayBuffers(val1, val2)) {
return false;
}
}
return keyCheck(val1, val2, kLoose, memos, kNoIterator);
return keyCheck(val1, val2, strict, memos, kNoIterator);
}

function getEnumerables(val, keys) {
Expand Down Expand Up @@ -370,21 +326,6 @@ function keyCheck(val1, val2, strict, memos, iterationType, aKeys) {
return areEq;
}

function innerDeepEqual(val1, val2, strict, memos) {
// All identical values are equivalent, as determined by ===.
if (val1 === val2) {
if (val1 !== 0)
return true;
return strict ? objectIs(val1, val2) : true;
}

// Check more closely if val1 and val2 are equal.
if (strict === true)
return strictDeepEqual(val1, val2, memos);

return looseDeepEqual(val1, val2, memos);
}

function setHasEqualElement(set, val1, strict, memo) {
// Go looking.
for (const val2 of set) {
Expand Down
9 changes: 6 additions & 3 deletions test/es-module/test-esm-dynamic-import.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@ function expectMissingModuleError(result) {
function expectOkNamespace(result) {
Promise.resolve(result)
.then(common.mustCall((ns) => {
// Can't deepStrictEqual because ns isn't a normal object
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(ns, { default: true });
const expected = { default: true };
Object.defineProperty(expected, Symbol.toStringTag, {
value: 'Module'
});
Object.setPrototypeOf(expected, Object.getPrototypeOf(ns));
assert.deepStrictEqual(ns, expected);
}));
}

Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-assert-checktag.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ if (process.stdout.isTTY)
FakeDate.prototype = Date.prototype;
const fake = new FakeDate();

assert.deepEqual(date, fake);
assert.deepEqual(fake, date);
assert.notDeepEqual(date, fake);
assert.notDeepEqual(fake, date);

// For deepStrictEqual we check the runtime type,
// then reveal the fakeness of the fake date
Expand All @@ -45,7 +45,7 @@ if (process.stdout.isTTY)
for (const prop of Object.keys(global)) {
fakeGlobal[prop] = global[prop];
}
assert.deepEqual(fakeGlobal, global);
assert.notDeepEqual(fakeGlobal, global);
// Message will be truncated anyway, don't validate
assert.throws(() => assert.deepStrictEqual(fakeGlobal, global),
assert.AssertionError);
Expand All @@ -57,7 +57,7 @@ if (process.stdout.isTTY)
for (const prop of Object.keys(process)) {
fakeProcess[prop] = process[prop];
}
assert.deepEqual(fakeProcess, process);
assert.notDeepEqual(fakeProcess, process);
// Message will be truncated anyway, don't validate
assert.throws(() => assert.deepStrictEqual(fakeProcess, process),
assert.AssertionError);
Expand Down
Loading

0 comments on commit 7493db2

Please sign in to comment.