From 82174412a522d76c4a381107f5f7c03591c70c6a Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 3 Jun 2019 15:34:35 +0200 Subject: [PATCH] assert: fix error diff In some edge cases an identical line could be printed twice. This is now fixed by changing the algorithm a bit. It will now verify how many lines were identical before the current one. PR-URL: https://github.com/nodejs/node/pull/28058 Reviewed-By: Rich Trott --- lib/internal/assert/assertion_error.js | 66 +++++++++++--------------- test/parallel/test-assert-deep.js | 27 ++++++++--- 2 files changed, 49 insertions(+), 44 deletions(-) diff --git a/lib/internal/assert/assertion_error.js b/lib/internal/assert/assertion_error.js index ce171710c87472..8943a702fa1c17 100644 --- a/lib/internal/assert/assertion_error.js +++ b/lib/internal/assert/assertion_error.js @@ -52,17 +52,11 @@ function inspectValue(val) { maxArrayLength: Infinity, // Assert compares only enumerable properties (with a few exceptions). showHidden: false, - // Having a long line as error is better than wrapping the line for - // comparison for now. - // TODO(BridgeAR): `breakLength` should be limited as soon as soon as we - // have meta information about the inspected properties (i.e., know where - // in what line the property starts and ends). - breakLength: Infinity, // Assert does not detect proxies currently. showProxy: false, sorted: true, // Inspect getters as we also check them when comparing entries. - getters: true + getters: true, } ); } @@ -70,7 +64,6 @@ function inspectValue(val) { function createErrDiff(actual, expected, operator) { let other = ''; let res = ''; - let lastPos = 0; let end = ''; let skipped = false; const actualInspected = inspectValue(actual); @@ -171,51 +164,48 @@ function createErrDiff(actual, expected, operator) { } let printedLines = 0; + let identical = 0; const msg = kReadableOperator[operator] + `\n${green}+ actual${white} ${red}- expected${white}`; const skippedMsg = ` ${blue}...${white} Lines skipped`; for (i = 0; i < maxLines; i++) { - // Only extra expected lines exist - const cur = i - lastPos; if (actualLines.length < i + 1) { - // If the last diverging line is more than one line above and the - // current line is at least line three, add some of the former lines and - // also add dots to indicate skipped entries. - if (cur > 1 && i > 2) { - if (cur > 4) { + // If more than one former line is identical, print that. Collapse those + // in case more than three lines before were identical. + if (identical > 1) { + if (identical > 3) { res += `\n${blue}...${white}`; skipped = true; - } else if (cur > 3) { + } else if (identical > 2) { res += `\n ${expectedLines[i - 2]}`; printedLines++; } res += `\n ${expectedLines[i - 1]}`; printedLines++; } - // Mark the current line as the last diverging one. - lastPos = i; + // No identical lines before. + identical = 0; // Add the expected line to the cache. other += `\n${red}-${white} ${expectedLines[i]}`; printedLines++; // Only extra actual lines exist } else if (expectedLines.length < i + 1) { - // If the last diverging line is more than one line above and the - // current line is at least line three, add some of the former lines and - // also add dots to indicate skipped entries. - if (cur > 1 && i > 2) { - if (cur > 4) { + // If more than one former line is identical, print that. Collapse those + // in case more than three lines before were identical. + if (identical > 1) { + if (identical > 3) { res += `\n${blue}...${white}`; skipped = true; - } else if (cur > 3) { + } else if (identical > 2) { res += `\n ${actualLines[i - 2]}`; printedLines++; } res += `\n ${actualLines[i - 1]}`; printedLines++; } - // Mark the current line as the last diverging one. - lastPos = i; + // No identical lines before. + identical = 0; // Add the actual line to the result. res += `\n${green}+${white} ${actualLines[i]}`; printedLines++; @@ -245,22 +235,21 @@ function createErrDiff(actual, expected, operator) { actualLine += ','; } if (divergingLines) { - // If the last diverging line is more than one line above and the - // current line is at least line three, add some of the former lines and - // also add dots to indicate skipped entries. - if (cur > 1 && i > 2) { - if (cur > 4) { + // If more than one former line is identical, print that. Collapse those + // in case more than three lines before were identical. + if (identical > 1) { + if (identical > 3) { res += `\n${blue}...${white}`; skipped = true; - } else if (cur > 3) { + } else if (identical > 2) { res += `\n ${actualLines[i - 2]}`; printedLines++; } res += `\n ${actualLines[i - 1]}`; printedLines++; } - // Mark the current line as the last diverging one. - lastPos = i; + // No identical lines before. + identical = 0; // Add the actual line to the result and cache the expected diverging // line so consecutive diverging lines show up as +++--- and not +-+-+-. res += `\n${green}+${white} ${actualLine}`; @@ -272,9 +261,10 @@ function createErrDiff(actual, expected, operator) { // and reset the cache. res += other; other = ''; - // If the last diverging line is exactly one line above or if it is the - // very first line, add the line to the result. - if (cur === 1 || i === 0) { + identical++; + // The very first identical line since the last diverging line is be + // added to the result. + if (identical === 1) { res += `\n ${actualLine}`; printedLines++; } @@ -316,7 +306,7 @@ class AssertionError extends Error { if (process.stderr.isTTY) { // Reset on each call to make sure we handle dynamically set environment // variables correct. - if (process.stderr.getColorDepth() !== 1) { + if (process.stderr.hasColors()) { blue = '\u001b[34m'; green = '\u001b[32m'; white = '\u001b[39m'; diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index a00b26f22b12f4..7cd57b0a052711 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -68,7 +68,6 @@ assert.deepEqual(arr, buf); code: 'ERR_ASSERTION', message: `${defaultMsgStartFull} ... Lines skipped\n\n` + ' Buffer [Uint8Array] [\n' + - ' 120,\n' + '...\n' + ' 10,\n' + '+ prop: 1\n' + @@ -87,7 +86,6 @@ assert.deepEqual(arr, buf); code: 'ERR_ASSERTION', message: `${defaultMsgStartFull} ... Lines skipped\n\n` + ' Uint8Array [\n' + - ' 120,\n' + '...\n' + ' 10,\n' + '- prop: 5\n' + @@ -921,13 +919,30 @@ assert.deepStrictEqual(obj1, obj2); const a = new TypeError('foo'); const b = new TypeError('foo'); a.foo = 'bar'; - b.foo = 'baz'; + b.foo = 'baz.'; assert.throws( - () => assert.deepStrictEqual(a, b), + () => assert.throws( + () => assert.deepStrictEqual(a, b), + { + operator: 'throws', + message: `${defaultMsgStartFull}\n\n` + + ' [TypeError: foo] {\n+ foo: \'bar\'\n- foo: \'baz\'\n }', + } + ), { - message: `${defaultMsgStartFull}\n\n` + - ' [TypeError: foo] {\n+ foo: \'bar\'\n- foo: \'baz\'\n }' + message: 'Expected values to be strictly deep-equal:\n' + + '+ actual - expected ... Lines skipped\n' + + '\n' + + ' Comparison {\n' + + '...\n' + + " \"+ foo: 'bar'\\n\" +\n" + + "+ \"- foo: 'baz.'\\n\" +\n" + + "- \"- foo: 'baz'\\n\" +\n" + + " ' }',\n" + + "+ operator: 'deepStrictEqual'\n" + + "- operator: 'throws'\n" + + ' }' } ); }