Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

assert: improve assertion error inspection #28058

Closed
wants to merge 3 commits into from
Closed

assert: improve assertion error inspection #28058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15f393c
assert: limit string inspection when logging assertion errors
BridgeAR Jun 2, 2019
fa7c39f
assert: fix error diff
BridgeAR Jun 3, 2019
97ec3b2
assert: print more lines in the error diff
BridgeAR Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 85 additions & 61 deletions lib/internal/assert/assertion_error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,25 +52,18 @@ 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,
}
);
}

function createErrDiff(actual, expected, operator) {
let other = '';
let res = '';
let lastPos = 0;
let end = '';
let skipped = false;
const actualInspected = inspectValue(actual);
Expand Down Expand Up @@ -129,7 +122,7 @@ function createErrDiff(actual, expected, operator) {
let a = actualLines[actualLines.length - 1];
let b = expectedLines[expectedLines.length - 1];
while (a === b) {
if (i++ < 2) {
if (i++ < 3) {
end = `\n ${a}${end}`;
} else {
other = a;
Expand Down Expand Up @@ -161,7 +154,9 @@ function createErrDiff(actual, expected, operator) {
return `${kReadableOperator.notIdentical}\n\n${actualLines.join('\n')}\n`;
}

if (i > 3) {
// There were at least five identical lines at the end. Mark a couple of
// skipped.
if (i >= 5) {
end = `\n${blue}...${white}${end}`;
skipped = true;
}
Expand All @@ -171,54 +166,51 @@ 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`;

let lines = actualLines;
let plusMinus = `${green}+${white}`;
let maxLength = expectedLines.length;
if (actualLines.length < maxLines) {
lines = expectedLines;
plusMinus = `${red}-${white}`;
maxLength = actualLines.length;
}

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) {
res += `\n${blue}...${white}`;
skipped = true;
} else if (cur > 3) {
res += `\n ${expectedLines[i - 2]}`;
if (maxLength < i + 1) {
// If more than two former lines are identical, print them. Collapse them
// in case more than five lines were identical.
if (identical > 2) {
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
if (identical > 3) {
if (identical > 4) {
if (identical === 5) {
res += `\n ${lines[i - 3]}`;
printedLines++;
} else {
res += `\n${blue}...${white}`;
skipped = true;
}
}
res += `\n ${lines[i - 2]}`;
printedLines++;
}
res += `\n ${expectedLines[i - 1]}`;
res += `\n ${lines[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) {
res += `\n${blue}...${white}`;
skipped = true;
} else if (cur > 3) {
res += `\n ${actualLines[i - 2]}`;
printedLines++;
}
res += `\n ${actualLines[i - 1]}`;
printedLines++;
if (lines === actualLines) {
res += `\n${plusMinus} ${lines[i]}`;
} else {
other += `\n${plusMinus} ${lines[i]}`;
}
// 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++;
// Only extra actual lines exist
// Lines diverge
} else {
const expectedLine = expectedLines[i];
Expand All @@ -245,22 +237,27 @@ 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}`;
skipped = true;
} else if (cur > 3) {
// If more than two former lines are identical, print them. Collapse
// them in case more than five lines were identical.
if (identical > 2) {
if (identical > 3) {
if (identical > 4) {
if (identical === 5) {
res += `\n ${actualLines[i - 3]}`;
printedLines++;
} else {
res += `\n${blue}...${white}`;
skipped = true;
}
}
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}`;
Expand All @@ -272,9 +269,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 <= 2) {
res += `\n ${actualLine}`;
printedLines++;
}
Expand Down Expand Up @@ -316,7 +314,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';
Expand Down Expand Up @@ -429,11 +427,37 @@ class AssertionError extends Error {
}

[inspect.custom](recurseTimes, ctx) {
// Long strings should not be fully inspected.
const tmpActual = this.actual;
const tmpExpected = this.expected;

for (const name of ['actual', 'expected']) {
if (typeof this[name] === 'string') {
const lines = this[name].split('\n');
if (lines.length > 10) {
lines.length = 10;
this[name] = `${lines.join('\n')}\n...`;
} else if (this[name].length > 512) {
this[name] = `${this[name].slice(512)}...`;
}
}
}

// This limits the `actual` and `expected` property default inspection to
// 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 });
const result = inspect(this, {
...ctx,
customInspect: false,
depth: 0
});

// Reset the properties after inspection.
this.actual = tmpActual;
this.expected = tmpExpected;

return result;
}
}

Expand Down
39 changes: 30 additions & 9 deletions test/parallel/test-assert-deep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ assert.throws(
code: 'ERR_ASSERTION',
message: `${defaultMsgStartFull} ... Lines skipped\n\n` +
'+ Uint8Array [\n' +
'- Buffer [Uint8Array] [\n 120,\n...\n 10\n ]'
'- Buffer [Uint8Array] [\n 120,\n...\n 122,\n 10\n ]'
}
);
assert.deepEqual(arr, buf);
Expand All @@ -66,10 +66,11 @@ assert.deepEqual(arr, buf);
() => assert.deepStrictEqual(buf2, buf),
{
code: 'ERR_ASSERTION',
message: `${defaultMsgStartFull} ... Lines skipped\n\n` +
message: `${defaultMsgStartFull}\n\n` +
' Buffer [Uint8Array] [\n' +
' 120,\n' +
'...\n' +
' 121,\n' +
' 122,\n' +
' 10,\n' +
'+ prop: 1\n' +
' ]'
Expand All @@ -85,10 +86,11 @@ assert.deepEqual(arr, buf);
() => assert.deepStrictEqual(arr, arr2),
{
code: 'ERR_ASSERTION',
message: `${defaultMsgStartFull} ... Lines skipped\n\n` +
message: `${defaultMsgStartFull}\n\n` +
' Uint8Array [\n' +
' 120,\n' +
'...\n' +
' 121,\n' +
' 122,\n' +
' 10,\n' +
'- prop: 5\n' +
' ]'
Expand Down Expand Up @@ -921,13 +923,32 @@ 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' +
" message: 'Expected values to be strictly deep-equal:\\n' +\n" +
'...\n' +
" ' [TypeError: foo] {\\n' +\n" +
" \"+ foo: 'bar'\\n\" +\n" +
"+ \"- foo: 'baz.'\\n\" +\n" +
"- \"- foo: 'baz'\\n\" +\n" +
" ' }',\n" +
"+ operator: 'deepStrictEqual'\n" +
"- operator: 'throws'\n" +
' }'
}
);
}
Expand Down
9 changes: 7 additions & 2 deletions test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,12 +482,14 @@ assert.throws(
'',
' [',
' [',
'...',
' [',
' 1,',
' 2,',
'+ 3',
"- '3'",
' ]',
'...',
' 4,',
' 5',
' ]'].join('\n');
assert.throws(
Expand All @@ -501,10 +503,12 @@ assert.throws(
' [',
' 1,',
'...',
' 1,',
' 0,',
'- 1,',
' 1,',
'...',
' 1,',
' 1',
' ]'
].join('\n');
Expand All @@ -521,10 +525,11 @@ assert.throws(
' [',
' 1,',
'...',
' 1,',
' 0,',
'+ 1,',
' 1,',
'...',
' 1,',
' 1',
' ]'
].join('\n');
Expand Down
3 changes: 2 additions & 1 deletion test/pseudo-tty/test-assert-colors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ try {
// active.
process.env.TERM = 'FOOBAR';
delete process.env.NODE_DISABLE_COLORS;
assert.deepStrictEqual([1, 2, 2, 2], [2, 2, 2, 2]);
assert.deepStrictEqual([1, 2, 2, 2, 2], [2, 2, 2, 2, 2]);
} catch (err) {
const expected = 'Expected values to be strictly deep-equal:\n' +
'\u001b[32m+ actual\u001b[39m \u001b[31m- expected\u001b[39m' +
Expand All @@ -19,6 +19,7 @@ try {
'\u001b[31m-\u001b[39m 2,\n' +
' 2,\n' +
'\u001b[34m...\u001b[39m\n' +
' 2,\n' +
' 2\n' +
' ]';
assert.strictEqual(err.message, expected);
Expand Down