From f242258a228b7d8906996cbe83690deca2ca0595 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 29 Sep 2015 20:03:30 -0700 Subject: [PATCH 1/4] lib: assert.deepEqual() fix for Error objects Error objects have non-enumerable properties. So, unexpectedly, assert.deepEqual(new Error('a'), new Error('b')) will not throw an AssertionError. This commit changes that behavior. Fixes: https://github.com/nodejs/node/issues/3122 --- lib/assert.js | 15 ++++++++++++--- test/parallel/test-assert.js | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 6b99098c5fda35..7e9e5545ef6c1c 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -202,9 +202,18 @@ function objEquiv(a, b, strict) { b = pSlice.call(b); return _deepEqual(a, b, strict); } - var ka = Object.keys(a), - kb = Object.keys(b), - key, i; + var ka, kb, key, i; + + function _getKeys(obj) { + if (obj instanceof Error) { + return Object.getOwnPropertyNames(obj); + } + return Object.keys(obj); + } + + ka = _getKeys(a); + kb = _getKeys(b); + // having the same number of owned properties (keys incorporates // hasOwnProperty) if (ka.length !== kb.length) diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index ce19462a62387c..96ab74f19f1e7f 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -362,6 +362,9 @@ try { gotError = true; } +// https://github.com/nodejs/node/issues/3122 +a.throws(makeBlock(a.deepEqual, Error('a'), Error('b'))); + // GH-7178. Ensure reflexivity of deepEqual with `arguments` objects. var args = (function() { return arguments; })(); a.throws(makeBlock(a.deepEqual, [], args)); From a5b09a5740fb3697ad1ed8b02da61de786d071c6 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 10 Oct 2015 19:55:11 -0700 Subject: [PATCH 2/4] fixup: generalize, remove special handling for Error, Buffer, and RegExp --- lib/assert.js | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 7e9e5545ef6c1c..e5e6dec429e317 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -25,9 +25,7 @@ 'use strict'; // UTILITY -const compare = process.binding('buffer').compare; const util = require('util'); -const Buffer = require('buffer').Buffer; const pSlice = Array.prototype.slice; // 1. The assert module provides functions that throw @@ -146,24 +144,12 @@ function _deepEqual(actual, expected, strict) { // 7.1. All identical values are equivalent, as determined by ===. if (actual === expected) { return true; - } else if (actual instanceof Buffer && expected instanceof Buffer) { - return compare(actual, expected) === 0; // 7.2. If the expected value is a Date object, the actual value is // equivalent if it is also a Date object that refers to the same time. } else if (util.isDate(actual) && util.isDate(expected)) { return actual.getTime() === expected.getTime(); - // 7.3 If the expected value is a RegExp object, the actual value is - // equivalent if it is also a RegExp object with the same source and - // properties (`global`, `multiline`, `lastIndex`, `ignoreCase`). - } else if (util.isRegExp(actual) && util.isRegExp(expected)) { - return actual.source === expected.source && - actual.global === expected.global && - actual.multiline === expected.multiline && - actual.lastIndex === expected.lastIndex && - actual.ignoreCase === expected.ignoreCase; - // 7.4. Other pairs that do not both pass typeof value == 'object', // equivalence is determined by ==. } else if ((actual === null || typeof actual !== 'object') && @@ -185,6 +171,10 @@ function isArguments(object) { return Object.prototype.toString.call(object) == '[object Arguments]'; } +function isArrayOrString(object) { + return object instanceof Array || object instanceof String; +} + function objEquiv(a, b, strict) { if (a === null || a === undefined || b === null || b === undefined) return false; @@ -202,18 +192,26 @@ function objEquiv(a, b, strict) { b = pSlice.call(b); return _deepEqual(a, b, strict); } - var ka, kb, key, i; - - function _getKeys(obj) { - if (obj instanceof Error) { - return Object.getOwnPropertyNames(obj); + var ka = Object.getOwnPropertyNames(a), + kb = Object.getOwnPropertyNames(b), + key, i; + + // when comparing String/Array to non-Strings/non-Array, ignore length prop + const aLengthIndex = ka.indexOf('length'); + const bLengthIndex = kb.indexOf('length'); + const aHasLength = (aLengthIndex !== -1); + const bHasLength = (bLengthIndex !== -1); + + if (isArrayOrString(a) !== isArrayOrString(b)) { + if (aHasLength !== bHasLength) { + if (aHasLength) { + ka.splice(bLengthIndex); + } else { + kb.splice(aLengthIndex); + } } - return Object.keys(obj); } - ka = _getKeys(a); - kb = _getKeys(b); - // having the same number of owned properties (keys incorporates // hasOwnProperty) if (ka.length !== kb.length) From a70eb3cfb559c3bbbf56452f79c3cf5de23925ff Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 12 Oct 2015 10:19:13 -0700 Subject: [PATCH 3/4] fixup: symbols too! --- lib/assert.js | 8 ++++++-- test/parallel/test-assert.js | 10 +++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index e5e6dec429e317..d66ada45e65bb8 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -175,6 +175,10 @@ function isArrayOrString(object) { return object instanceof Array || object instanceof String; } +function getPropertiesAndSymbols(o) { + return Object.getOwnPropertyNames(o).concat(Object.getOwnPropertySymbols(o)); +} + function objEquiv(a, b, strict) { if (a === null || a === undefined || b === null || b === undefined) return false; @@ -192,8 +196,8 @@ function objEquiv(a, b, strict) { b = pSlice.call(b); return _deepEqual(a, b, strict); } - var ka = Object.getOwnPropertyNames(a), - kb = Object.getOwnPropertyNames(b), + var ka = getPropertiesAndSymbols(a), + kb = getPropertiesAndSymbols(b), key, i; // when comparing String/Array to non-Strings/non-Array, ignore length prop diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 96ab74f19f1e7f..046a57ddea3c40 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -363,7 +363,15 @@ try { } // https://github.com/nodejs/node/issues/3122 -a.throws(makeBlock(a.deepEqual, Error('a'), Error('b'))); +a.throws(makeBlock(a.deepEqual, Error('a'), Error('b')), + a.AssertionError); + +// https://github.com/nodejs/node/pull/3124#issuecomment-147416176 +const symbol = Symbol(); +a.doesNotThrow(makeBlock(a.deepEqual, {[symbol]: 1}, {[symbol]: 1})); +a.throws(makeBlock(a.deepEqual, {[Symbol('foo')]: 1}, {[Symbol('foo')]: 1}), + a.AssertionError); + // GH-7178. Ensure reflexivity of deepEqual with `arguments` objects. var args = (function() { return arguments; })(); From 92cf155c0fea16d3afada3ac00de8cb28c0ebbc7 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 12 Oct 2015 10:41:55 -0700 Subject: [PATCH 4/4] fixup: argh, linter hates computed properties, boo! --- test/parallel/test-assert.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 046a57ddea3c40..290b2095dafc27 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -367,9 +367,20 @@ a.throws(makeBlock(a.deepEqual, Error('a'), Error('b')), a.AssertionError); // https://github.com/nodejs/node/pull/3124#issuecomment-147416176 +// argh! this const/var blizzard due to current eslint we're using flagging +// computed properties as a lint error. +// Change to computed properties after https://github.com/nodejs/node/pull/2286 +// lands. const symbol = Symbol(); -a.doesNotThrow(makeBlock(a.deepEqual, {[symbol]: 1}, {[symbol]: 1})); -a.throws(makeBlock(a.deepEqual, {[Symbol('foo')]: 1}, {[Symbol('foo')]: 1}), +const symbol2 = Symbol(); +var obj1 = {}; +var obj2 = {}; +var obj3 = {}; +obj1[symbol] = 1; +obj2[symbol] = 1; +obj3[symbol2] = 1; +a.doesNotThrow(makeBlock(a.deepEqual, obj1, obj2)); +a.throws(makeBlock(a.deepEqual, obj1, obj3), a.AssertionError);