From 1e6bd0384818fccd9f0053ea8f45207b4b042efd Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 24 Sep 2018 10:15:52 +0200 Subject: [PATCH 1/3] assert: reduce diff noise Assertion errors that produce a diff show a diff for identical entries in case one side of the comparison has more object properties than the other one. Those lines are now taken into account and will not show up as diverging lines anymore. Refs: https://github.com/nodejs/node/issues/22763 --- lib/internal/assert.js | 55 +++++++++++++++++++------------ test/parallel/test-assert-deep.js | 24 +++++++++----- 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/lib/internal/assert.js b/lib/internal/assert.js index 36e3bbf57b5613..09b6063a8540c0 100644 --- a/lib/internal/assert.js +++ b/lib/internal/assert.js @@ -191,29 +191,42 @@ function createErrDiff(actual, expected, operator) { res += `\n${green}+${white} ${actualLines[i]}`; printedLines++; // Lines diverge - } else if (actualLines[i] !== expectedLines[i]) { - if (cur > 1 && i > 2) { - if (cur > 4) { - res += `\n${blue}...${white}`; - skipped = true; - } else if (cur > 3) { - res += `\n ${actualLines[i - 2]}`; + } else { + const expectedLine = expectedLines[i]; + let actualLine = actualLines[i]; + let divergingLines = actualLine !== expectedLine && + (!actualLine.endsWith(',') || + actualLine.slice(0, -1) !== expectedLine); + if (divergingLines && + expectedLine.endsWith(',') && + expectedLine.slice(0, -1) === actualLine) { + divergingLines = false; + actualLine += ','; + } + if (divergingLines) { + if (cur > 1 && i > 2) { + if (cur > 4) { + res += `\n${blue}...${white}`; + skipped = true; + } else if (cur > 3) { + res += `\n ${actualLines[i - 2]}`; + printedLines++; + } + res += `\n ${actualLines[i - 1]}`; + printedLines++; + } + lastPos = i; + res += `\n${green}+${white} ${actualLine}`; + other += `\n${red}-${white} ${expectedLine}`; + printedLines += 2; + // Lines are identical + } else { + res += other; + other = ''; + if (cur === 1 || i === 0) { + res += `\n ${actualLine}`; printedLines++; } - res += `\n ${actualLines[i - 1]}`; - printedLines++; - } - lastPos = i; - res += `\n${green}+${white} ${actualLines[i]}`; - other += `\n${red}-${white} ${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) diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index fab681a89f6a03..9c5fdbb90814e4 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -65,9 +65,13 @@ assert.deepEqual(arr, buf); () => assert.deepStrictEqual(buf2, buf), { code: 'ERR_ASSERTION', - message: `${defaultMsgStartFull}\n\n` + - ' Buffer [Uint8Array] [\n 120,\n 121,\n 122,\n' + - '+ 10,\n+ prop: 1\n- 10\n ]' + message: `${defaultMsgStartFull} ... Lines skipped\n\n` + + ' Buffer [Uint8Array] [\n' + + ' 120,\n' + + '...\n' + + ' 10,\n' + + '+ prop: 1\n' + + ' ]' } ); assert.deepEqual(buf2, buf); @@ -80,9 +84,13 @@ assert.deepEqual(arr, buf); () => assert.deepStrictEqual(arr, arr2), { code: 'ERR_ASSERTION', - message: `${defaultMsgStartFull}\n\n` + - ' Uint8Array [\n 120,\n 121,\n 122,\n' + - '+ 10\n- 10,\n- prop: 5\n ]' + message: `${defaultMsgStartFull} ... Lines skipped\n\n` + + ' Uint8Array [\n' + + ' 120,\n' + + '...\n' + + ' 10,\n' + + '- prop: 5\n' + + ' ]' } ); assert.deepEqual(arr, arr2); @@ -822,7 +830,7 @@ assert.throws( code: 'ERR_ASSERTION', name: 'AssertionError [ERR_ASSERTION]', message: `${defaultMsgStartFull}\n\n ` + - '{\n+ a: 4\n- a: 4,\n- b: true\n }' + '{\n a: 4,\n- b: true\n }' }); assert.throws( () => assert.deepStrictEqual(['a'], { 0: 'a' }), @@ -891,7 +899,7 @@ assert.deepStrictEqual(obj1, obj2); assert.throws( () => assert.deepStrictEqual(arrProxy, [1, 2, 3]), { message: `${defaultMsgStartFull}\n\n` + - ' [\n 1,\n+ 2\n- 2,\n- 3\n ]' } + ' [\n 1,\n 2,\n- 3\n ]' } ); util.inspect.defaultOptions = tmp; From 11236dfd54e15639ae628597f3933beace8e90e0 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 24 Sep 2018 14:18:21 +0200 Subject: [PATCH 2/3] assert: add comments for diff algorithm It became hard to follow what was actually happening in the algorithm. This adds comments to improve the situation. --- lib/internal/assert.js | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/lib/internal/assert.js b/lib/internal/assert.js index 09b6063a8540c0..2cdcb77faa381f 100644 --- a/lib/internal/assert.js +++ b/lib/internal/assert.js @@ -160,6 +160,9 @@ function createErrDiff(actual, expected, operator) { // 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) { res += `\n${blue}...${white}`; @@ -171,11 +174,16 @@ function createErrDiff(actual, expected, operator) { res += `\n ${expectedLines[i - 1]}`; printedLines++; } + // Mark the current line as the last diverging one. lastPos = i; + // 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) { res += `\n${blue}...${white}`; @@ -187,16 +195,30 @@ function createErrDiff(actual, expected, operator) { res += `\n ${actualLines[i - 1]}`; printedLines++; } + // Mark the current line as the last diverging one. lastPos = i; + // Add the actual line to the result. res += `\n${green}+${white} ${actualLines[i]}`; printedLines++; // Lines diverge } else { const expectedLine = expectedLines[i]; let actualLine = actualLines[i]; + // If the lines diverge, specifically check for lines that only diverge by + // a trailing comma. In that case it is actually identical and we should + // mark it as such. let divergingLines = actualLine !== expectedLine && (!actualLine.endsWith(',') || actualLine.slice(0, -1) !== expectedLine); + // If the expected line has a trailing comma but is otherwise identical, + // add a comma at the end of the actual line. Otherwise the output could + // look weird as in: + // + // [ + // 1 // No comma at the end! + // + 2 + // ] + // if (divergingLines && expectedLine.endsWith(',') && expectedLine.slice(0, -1) === actualLine) { @@ -204,6 +226,9 @@ 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) { res += `\n${blue}...${white}`; @@ -215,14 +240,21 @@ function createErrDiff(actual, expected, operator) { res += `\n ${actualLines[i - 1]}`; printedLines++; } + // Mark the current line as the last diverging one. lastPos = i; + // 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}`; other += `\n${red}-${white} ${expectedLine}`; printedLines += 2; // Lines are identical } else { + // Add all cached information to the result before adding other things + // and reset the cache. res += other; other = ''; + // If the last diverging line is not more than two lines above, print + // that line as well. if (cur === 1 || i === 0) { res += `\n ${actualLine}`; printedLines++; @@ -232,7 +264,7 @@ function createErrDiff(actual, expected, operator) { // Inspected object to big (Show ~20 rows max) if (printedLines > 20 && i < maxLines - 2) { return `${msg}${skippedMsg}\n${res}\n${blue}...${white}${other}\n` + - `${blue}...${white}`; + `${blue}...${white}`; } } From 8b493b5d4e04f24a78982ec8798152396761d857 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 25 Sep 2018 11:02:26 +0200 Subject: [PATCH 3/3] fixup: fix wrong wording --- lib/internal/assert.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/assert.js b/lib/internal/assert.js index 2cdcb77faa381f..d754230394f054 100644 --- a/lib/internal/assert.js +++ b/lib/internal/assert.js @@ -253,8 +253,8 @@ function createErrDiff(actual, expected, operator) { // and reset the cache. res += other; other = ''; - // If the last diverging line is not more than two lines above, print - // that line as well. + // 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) { res += `\n ${actualLine}`; printedLines++;