From ca7ad0370c983ddd95bae692191bb81db5ed02f7 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 28 Dec 2018 15:36:26 +0100 Subject: [PATCH 1/5] assert: make `actual` and `expected` getters The `actual` and `expected` properties on an instance of `AssertionError` is now a getter to prevent inspecting these when inspecting the error. These values will be visible in the error message and showing them otherwise would decrease the readability of the error. --- lib/internal/assert.js | 28 ++++++++++++++++++++++++++-- test/parallel/test-assert.js | 10 ++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/lib/internal/assert.js b/lib/internal/assert.js index 29769fc5617b95..61508ddda73245 100644 --- a/lib/internal/assert.js +++ b/lib/internal/assert.js @@ -390,8 +390,32 @@ class AssertionError extends Error { this.generatedMessage = !message; this.name = 'AssertionError [ERR_ASSERTION]'; this.code = 'ERR_ASSERTION'; - this.actual = actual; - this.expected = expected; + // This prevents inspecting the actual and expected value by default. + // That is logical since the summary will be printed already. Users can then + // explicitly access these properties in case they want to know what value + // was used. + Object.defineProperties(this, { + actual: { + get() { + return actual; + }, + set(val) { + actual = val; + }, + enumerable: true, + configurable: true + }, + expected: { + get() { + return expected; + }, + set(val) { + expected = val; + }, + enumerable: true, + configurable: true + } + }); this.operator = operator; Error.captureStackTrace(this, stackStartFn); } diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index db00ed2836327f..41eefe32601685 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -308,6 +308,16 @@ try { `${strictEqualMessageStart}\n1 !== 2\n` ); assert.ok(e.generatedMessage, 'Message not marked as generated'); + const descriptors = Object.getOwnPropertyDescriptors(e); + assert.strictEqual(typeof descriptors.actual.get, 'function'); + assert.strictEqual(typeof descriptors.expected.get, 'function'); + const { actual, expected } = e; + e.actual++; + e.expected++; + assert.strictEqual(descriptors.actual.enumerable, true); + assert.strictEqual(descriptors.expected.enumerable, true); + assert.strictEqual(e.actual, actual + 1); + assert.strictEqual(e.expected, expected + 1); } try { From d92a714d725137b47f972600e73bc65c24349319 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 28 Dec 2018 18:08:03 +0100 Subject: [PATCH 2/5] test: fix failing assertion One test did not cause an assertion. By changing the test to use `assert.throws()` all tests have to throw, otherwise the test will fail. --- test/parallel/test-assert.js | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 41eefe32601685..7592fc626f5732 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -257,16 +257,14 @@ const circular = { y: 1 }; circular.x = circular; function testAssertionMessage(actual, expected, msg) { - try { - assert.strictEqual(actual, ''); - } catch (e) { - assert.strictEqual( - e.message, - msg || strictEqualMessageStart + - `+ actual - expected\n\n+ ${expected}\n- ''` - ); - assert.ok(e.generatedMessage, 'Message not marked as generated'); - } + assert.throws( + () => assert.strictEqual(actual, ''), + { + generatedMessage: true, + message: msg || strictEqualMessageStart + + `+ actual - expected\n\n+ ${expected}\n- ''` + } + ); } function testShortAssertionMessage(actual, expected) { @@ -280,7 +278,7 @@ testShortAssertionMessage(false, 'false'); testShortAssertionMessage(100, '100'); testShortAssertionMessage(NaN, 'NaN'); testShortAssertionMessage(Infinity, 'Infinity'); -testShortAssertionMessage('', '""'); +testShortAssertionMessage('a', '"a"'); testShortAssertionMessage('foo', '\'foo\''); testShortAssertionMessage(0, '0'); testShortAssertionMessage(Symbol(), 'Symbol()'); From bc2ccc25433f479ffc16ab3c3e9e5c86e2281fa3 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 29 Dec 2018 00:02:01 +0100 Subject: [PATCH 3/5] fixup: use custom inspect function to minimize input inspection --- lib/internal/assert.js | 28 ++++++---------------------- test/parallel/test-assert.js | 21 +++++++++++---------- 2 files changed, 17 insertions(+), 32 deletions(-) diff --git a/lib/internal/assert.js b/lib/internal/assert.js index 61508ddda73245..2e3ab73f8cdad1 100644 --- a/lib/internal/assert.js +++ b/lib/internal/assert.js @@ -394,31 +394,15 @@ class AssertionError extends Error { // That is logical since the summary will be printed already. Users can then // explicitly access these properties in case they want to know what value // was used. - Object.defineProperties(this, { - actual: { - get() { - return actual; - }, - set(val) { - actual = val; - }, - enumerable: true, - configurable: true - }, - expected: { - get() { - return expected; - }, - set(val) { - expected = val; - }, - enumerable: true, - configurable: true - } - }); + this.actual = actual; + this.expected = expected; this.operator = operator; Error.captureStackTrace(this, stackStartFn); } + + [inspect.custom](recurseTimes, ctx) { + return inspect(this, { ...ctx, customInspect: false, depth: 0 }); + } } module.exports = { diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 7592fc626f5732..a7c37e8fb33024 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -306,16 +306,6 @@ try { `${strictEqualMessageStart}\n1 !== 2\n` ); assert.ok(e.generatedMessage, 'Message not marked as generated'); - const descriptors = Object.getOwnPropertyDescriptors(e); - assert.strictEqual(typeof descriptors.actual.get, 'function'); - assert.strictEqual(typeof descriptors.expected.get, 'function'); - const { actual, expected } = e; - e.actual++; - e.expected++; - assert.strictEqual(descriptors.actual.enumerable, true); - assert.strictEqual(descriptors.expected.enumerable, true); - assert.strictEqual(e.actual, actual + 1); - assert.strictEqual(e.expected, expected + 1); } try { @@ -1147,3 +1137,14 @@ assert.throws( '{\n a: true\n}\n' } ); + +{ + let threw = false; + try { + assert.deepStrictEqual(Array(100).fill(1), 'foobar'); + } catch (err) { + threw = true; + assert(/actual: \[Array],\n expected: 'foobar',/.test(inspect(err))); + } + assert(threw); +} From 06e53886e0d47f7bf4ea50a96e588f3eb47a1eb3 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 29 Dec 2018 00:47:13 +0100 Subject: [PATCH 4/5] fixup: move comment --- lib/internal/assert.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/assert.js b/lib/internal/assert.js index 2e3ab73f8cdad1..919e5c44640695 100644 --- a/lib/internal/assert.js +++ b/lib/internal/assert.js @@ -390,10 +390,6 @@ class AssertionError extends Error { this.generatedMessage = !message; this.name = 'AssertionError [ERR_ASSERTION]'; this.code = 'ERR_ASSERTION'; - // This prevents inspecting the actual and expected value by default. - // That is logical since the summary will be printed already. Users can then - // explicitly access these properties in case they want to know what value - // was used. this.actual = actual; this.expected = expected; this.operator = operator; @@ -401,6 +397,10 @@ class AssertionError extends Error { } [inspect.custom](recurseTimes, ctx) { + // This limits the `actual` and `expected` property default inspection to + // the minimum depth. Otherwise those values would be to verbose compared + // to the actual error message which contains a combined view of these two + // input values. return inspect(this, { ...ctx, customInspect: false, depth: 0 }); } } From 0f7ff0e6a95ea5eea11869549b9c907b0ee8a439 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 Jan 2019 02:36:59 +0100 Subject: [PATCH 5/5] fixup: fix typo Co-Authored-By: BridgeAR --- lib/internal/assert.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/assert.js b/lib/internal/assert.js index 919e5c44640695..4e059db6b9d478 100644 --- a/lib/internal/assert.js +++ b/lib/internal/assert.js @@ -398,7 +398,7 @@ class AssertionError extends Error { [inspect.custom](recurseTimes, ctx) { // This limits the `actual` and `expected` property default inspection to - // the minimum depth. Otherwise those values would be to verbose compared + // the minimum depth. Otherwise those values would be too verbose compared // to the actual error message which contains a combined view of these two // input values. return inspect(this, { ...ctx, customInspect: false, depth: 0 });