From 5aa3a2d172efb5cdca07ba6f0f2b8b054ced97ea Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater <ruben@bridgewater.de> Date: Sat, 9 Dec 2017 19:38:20 -0200 Subject: [PATCH] assert: improve error messages From now on all error messages produced by `assert` in strict mode will produce a error diff. Backport-PR-URL: https://github.com/nodejs/node/pull/19230 PR-URL: https://github.com/nodejs/node/pull/17615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> --- doc/api/assert.md | 33 ++++++++ lib/assert.js | 16 +++- lib/internal/errors.js | 150 ++++++++++++++++++++++++++++++++++- test/parallel/test-assert.js | 143 ++++++++++++++++++++++++++++++++- 4 files changed, 332 insertions(+), 10 deletions(-) diff --git a/doc/api/assert.md b/doc/api/assert.md index d53bccd21a998d..68b01350dfdee6 100644 --- a/doc/api/assert.md +++ b/doc/api/assert.md @@ -17,6 +17,9 @@ For more information about the used equality comparisons see <!-- YAML added: REPLACEME changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/REPLACEME + description: Added error diffs to the strict mode - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/17002 description: Added strict mode to the assert module. @@ -26,12 +29,42 @@ When using the `strict mode`, any `assert` function will use the equality used i the strict function mode. So [`assert.deepEqual()`][] will, for example, work the same as [`assert.deepStrictEqual()`][]. +On top of that, error messages which involve objects produce an error diff +instead of displaying both objects. That is not the case for the legacy mode. + It can be accessed using: ```js const assert = require('assert').strict; ``` +Example error diff (the `expected`, `actual`, and `Lines skipped` will be on a +single row): + +```js +const assert = require('assert').strict; + +assert.deepEqual([[[1, 2, 3]], 4, 5], [[[1, 2, '3']], 4, 5]); +``` + +```diff +AssertionError [ERR_ASSERTION]: Input A expected to deepStrictEqual input B: ++ expected +- actual +... Lines skipped + + [ + [ +... + 2, +- 3 ++ '3' + ], +... + 5 + ] +``` + ## Legacy mode > Stability: 0 - Deprecated: Use strict mode instead. diff --git a/lib/assert.js b/lib/assert.js index c3570afeb38a3a..48670de0c5458a 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -25,6 +25,10 @@ const { isDeepEqual, isDeepStrictEqual } = const errors = require('internal/errors'); const { inspect } = require('util'); +const ERR_DIFF_DEACTIVATED = 0; +const ERR_DIFF_NOT_EQUAL = 1; +const ERR_DIFF_EQUAL = 2; + // The assert module provides functions that throw // AssertionError's when particular conditions are not met. The // assert module must conform to the following interface. @@ -154,7 +158,8 @@ assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) { expected, message, operator: 'deepStrictEqual', - stackStartFn: deepStrictEqual + stackStartFn: deepStrictEqual, + errorDiff: this === strict ? ERR_DIFF_EQUAL : ERR_DIFF_DEACTIVATED }); } }; @@ -167,7 +172,8 @@ function notDeepStrictEqual(actual, expected, message) { expected, message, operator: 'notDeepStrictEqual', - stackStartFn: notDeepStrictEqual + stackStartFn: notDeepStrictEqual, + errorDiff: this === strict ? ERR_DIFF_NOT_EQUAL : ERR_DIFF_DEACTIVATED }); } } @@ -180,7 +186,8 @@ assert.strictEqual = function strictEqual(actual, expected, message) { expected, message, operator: '===', - stackStartFn: strictEqual + stackStartFn: strictEqual, + errorDiff: this === strict ? ERR_DIFF_EQUAL : ERR_DIFF_DEACTIVATED }); } }; @@ -194,7 +201,8 @@ assert.notStrictEqual = function notStrictEqual(actual, expected, message) { expected, message, operator: '!==', - stackStartFn: notStrictEqual + stackStartFn: notStrictEqual, + errorDiff: this === strict ? ERR_DIFF_NOT_EQUAL : ERR_DIFF_DEACTIVATED }); } }; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 1d927df397b953..02802746951fad 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -78,13 +78,131 @@ function makeNodeError(Base) { }; } +function createErrDiff(actual, expected, operator) { + var other = ''; + var res = ''; + var lastPos = 0; + var end = ''; + var skipped = false; + const util = lazyUtil(); + const actualLines = util + .inspect(actual, { compact: false }).split('\n'); + const expectedLines = util + .inspect(expected, { compact: false }).split('\n'); + const msg = `Input A expected to ${operator} input B:\n` + + '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m'; + const skippedMsg = ' ... Lines skipped'; + + // Remove all ending lines that match (this optimizes the output for + // readability by reducing the number of total changed lines). + var a = actualLines[actualLines.length - 1]; + var b = expectedLines[expectedLines.length - 1]; + var i = 0; + while (a === b) { + if (i++ < 2) { + end = `\n ${a}${end}`; + } else { + other = a; + } + actualLines.pop(); + expectedLines.pop(); + a = actualLines[actualLines.length - 1]; + b = expectedLines[expectedLines.length - 1]; + } + if (i > 3) { + end = `\n...${end}`; + skipped = true; + } + if (other !== '') { + end = `\n ${other}${end}`; + other = ''; + } + + const maxLines = Math.max(actualLines.length, expectedLines.length); + var printedLines = 0; + for (i = 0; i < maxLines; i++) { + // Only extra expected lines exist + const cur = i - lastPos; + if (actualLines.length < i + 1) { + if (cur > 1 && i > 2) { + if (cur > 4) { + res += '\n...'; + skipped = true; + } else if (cur > 3) { + res += `\n ${expectedLines[i - 2]}`; + printedLines++; + } + res += `\n ${expectedLines[i - 1]}`; + printedLines++; + } + lastPos = i; + other += `\n\u001b[32m+\u001b[39m ${expectedLines[i]}`; + printedLines++; + // Only extra actual lines exist + } else if (expectedLines.length < i + 1) { + if (cur > 1 && i > 2) { + if (cur > 4) { + res += '\n...'; + skipped = true; + } else if (cur > 3) { + res += `\n ${actualLines[i - 2]}`; + printedLines++; + } + res += `\n ${actualLines[i - 1]}`; + printedLines++; + } + lastPos = i; + res += `\n\u001b[31m-\u001b[39m ${actualLines[i]}`; + printedLines++; + // Lines diverge + } else if (actualLines[i] !== expectedLines[i]) { + if (cur > 1 && i > 2) { + if (cur > 4) { + res += '\n...'; + skipped = true; + } else if (cur > 3) { + res += `\n ${actualLines[i - 2]}`; + printedLines++; + } + res += `\n ${actualLines[i - 1]}`; + printedLines++; + } + lastPos = i; + res += `\n\u001b[31m-\u001b[39m ${actualLines[i]}`; + other += `\n\u001b[32m+\u001b[39m ${expectedLines[i]}`; + printedLines += 2; + // Lines are identical + } else { + res += other; + other = ''; + if (cur === 1 || i === 0) { + res += `\n ${actualLines[i]}`; + printedLines++; + } + } + // Inspected object to big (Show ~20 rows max) + if (printedLines > 20 && i < maxLines - 2) { + return `${msg}${skippedMsg}\n${res}\n...${other}\n...`; + } + } + return `${msg}${skipped ? skippedMsg : ''}\n${res}${other}${end}`; +} + class AssertionError extends Error { constructor(options) { if (typeof options !== 'object' || options === null) { throw new exports.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object'); } - var { actual, expected, message, operator, stackStartFn } = options; - if (message) { + var { + actual, + expected, + message, + operator, + stackStartFn, + errorDiff = 0 + } = options; + + if (message != null) { super(message); } else { const util = lazyUtil(); @@ -92,8 +210,32 @@ class AssertionError extends Error { actual = `${actual.name}: ${actual.message}`; if (expected && expected.stack && expected instanceof Error) expected = `${expected.name}: ${expected.message}`; - super(`${util.inspect(actual).slice(0, 128)} ` + - `${operator} ${util.inspect(expected).slice(0, 128)}`); + + if (errorDiff === 0) { + let res = util.inspect(actual); + let other = util.inspect(expected); + if (res.length > 128) + res = `${res.slice(0, 125)}...`; + if (other.length > 128) + other = `${other.slice(0, 125)}...`; + super(`${res} ${operator} ${other}`); + } else if (errorDiff === 1) { + // In case the objects are equal but the operator requires unequal, show + // the first object and say A equals B + const res = util + .inspect(actual, { compact: false }).split('\n'); + + if (res.length > 20) { + res[19] = '...'; + while (res.length > 20) { + res.pop(); + } + } + // Only print a single object. + super(`Identical input passed to ${operator}:\n${res.join('\n')}`); + } else { + super(createErrDiff(actual, expected, operator)); + } } this.generatedMessage = !message; diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 462c01425dab5b..fa4ee21a6bd202 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -710,7 +710,8 @@ assert.throws(() => { assert.strictEqual('A'.repeat(1000), ''); }, common.expectsError({ code: 'ERR_ASSERTION', - message: new RegExp(`^'${'A'.repeat(127)} === ''$`) })); + message: /^'A{124}\.\.\. === ''$/ +})); { // bad args to AssertionError constructor should throw TypeError @@ -751,7 +752,6 @@ common.expectsError( assert.equal(assert.notEqual, assert.notStrictEqual); assert.equal(assert.notDeepEqual, assert.notDeepStrictEqual); assert.equal(Object.keys(assert).length, Object.keys(a).length); - /* eslint-enable no-restricted-properties */ assert(7); common.expectsError( () => assert(), @@ -761,6 +761,145 @@ common.expectsError( message: 'undefined == true' } ); + + // Test error diffs + const start = 'Input A expected to deepStrictEqual input B:'; + const actExp = '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m'; + const plus = '\u001b[32m+\u001b[39m'; + const minus = '\u001b[31m-\u001b[39m'; + let message = [ + start, + `${actExp} ... Lines skipped`, + '', + ' [', + ' [', + '...', + ' 2,', + `${minus} 3`, + `${plus} '3'`, + ' ]', + '...', + ' 5', + ' ]'].join('\n'); + assert.throws( + () => assert.deepEqual([[[1, 2, 3]], 4, 5], [[[1, 2, '3']], 4, 5]), + { message }); + + message = [ + start, + `${actExp} ... Lines skipped`, + '', + ' [', + ' 1,', + '...', + ' 0,', + `${plus} 1,`, + ' 1,', + '...', + ' 1', + ' ]' + ].join('\n'); + assert.throws( + () => assert.deepEqual( + [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1], + [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1]), + { message }); + + message = [ + start, + `${actExp} ... Lines skipped`, + '', + ' [', + ' 1,', + '...', + ' 0,', + `${minus} 1,`, + ' 1,', + '...', + ' 1', + ' ]' + ].join('\n'); + assert.throws( + () => assert.deepEqual( + [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1], + [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1]), + { message }); + + message = [ + start, + actExp, + '', + ' [', + ' 1,', + `${minus} 2,`, + `${plus} 1,`, + ' 1,', + ' 1,', + ' 0,', + `${minus} 1,`, + ' 1', + ' ]' + ].join('\n'); + assert.throws( + () => assert.deepEqual( + [1, 2, 1, 1, 0, 1, 1], + [1, 1, 1, 1, 0, 1]), + { message }); + + message = [ + start, + actExp, + '', + `${minus} [`, + `${minus} 1,`, + `${minus} 2,`, + `${minus} 1`, + `${minus} ]`, + `${plus} undefined`, + ].join('\n'); + assert.throws( + () => assert.deepEqual([1, 2, 1]), + { message }); + + message = [ + start, + actExp, + '', + ' [', + `${minus} 1,`, + ' 2,', + ' 1', + ' ]' + ].join('\n'); + assert.throws( + () => assert.deepEqual([1, 2, 1], [2, 1]), + { message }); + + message = `${start}\n` + + `${actExp} ... Lines skipped\n` + + '\n' + + ' [\n' + + `${minus} 1,\n`.repeat(10) + + '...\n' + + `${plus} 2,\n`.repeat(10) + + '...'; + assert.throws( + () => assert.deepEqual(Array(12).fill(1), Array(12).fill(2)), + { message }); + + // notDeepEqual tests + message = 'Identical input passed to notDeepStrictEqual:\n[\n 1\n]'; + assert.throws( + () => assert.notDeepEqual([1], [1]), + { message }); + + message = 'Identical input passed to notDeepStrictEqual:' + + `\n[${'\n 1,'.repeat(18)}\n...`; + const data = Array(21).fill(1); + assert.throws( + () => assert.notDeepEqual(data, data), + { message }); + /* eslint-enable no-restricted-properties */ } common.expectsError(