From 2f86e6268188e2e1fa8424b158d4d470d8eba0ca Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 1 May 2019 22:49:04 +0200 Subject: [PATCH 1/4] assert: fix `assert.fail()` stack This makes sure the error message visible in the error stack created when using `assert.fail()` without any arguments or the message set to `undefined` or `null` as only argument. That was masked before due to other changes. --- lib/assert.js | 14 ++++++-------- test/parallel/test-assert-fail.js | 3 ++- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index ffa71bfad8d412..dece9a08a78142 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -92,9 +92,10 @@ function innerFail(obj) { function fail(actual, expected, message, operator, stackStartFn) { const argsLen = arguments.length; - let internalMessage; - if (argsLen === 0) { - internalMessage = 'Failed'; + let internalMessage = false; + if (actual == null && argsLen <= 1) { + internalMessage = true; + message = 'Failed'; } else if (argsLen === 1) { message = actual; actual = undefined; @@ -118,14 +119,11 @@ function fail(actual, expected, message, operator, stackStartFn) { actual, expected, operator: operator === undefined ? 'fail' : operator, - stackStartFn: stackStartFn || fail + stackStartFn: stackStartFn || fail, + message }; - if (message !== undefined) { - errArgs.message = message; - } const err = new AssertionError(errArgs); if (internalMessage) { - err.message = internalMessage; err.generatedMessage = true; } throw err; diff --git a/test/parallel/test-assert-fail.js b/test/parallel/test-assert-fail.js index 51c69372dd8e5d..e2003f2ce91da8 100644 --- a/test/parallel/test-assert-fail.js +++ b/test/parallel/test-assert-fail.js @@ -13,7 +13,8 @@ assert.throws( operator: 'fail', actual: undefined, expected: undefined, - generatedMessage: true + generatedMessage: true, + stack: /Failed/ } ); From 2236e8e64233565228e08a04e6457067d4467149 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 1 May 2019 23:52:37 +0200 Subject: [PATCH 2/4] assert: refine assertion message This makes sure that the error message is more appropriate than before by checking closer what operator is used and which is not. It also increases the total number of lines printed to the user. --- lib/internal/assert/assertion_error.js | 63 ++++++++++--------- test/parallel/test-assert-deep.js | 28 ++++++--- test/parallel/test-assert.js | 24 ++++--- .../test-assert-position-indicator.js | 2 +- 4 files changed, 73 insertions(+), 44 deletions(-) diff --git a/lib/internal/assert/assertion_error.js b/lib/internal/assert/assertion_error.js index 7ab74e6777cdc1..048d7be75fc662 100644 --- a/lib/internal/assert/assertion_error.js +++ b/lib/internal/assert/assertion_error.js @@ -17,19 +17,18 @@ const kReadableOperator = { strictEqual: 'Expected values to be strictly equal:', strictEqualObject: 'Expected "actual" to be reference-equal to "expected":', deepEqual: 'Expected values to be loosely deep-equal:', - equal: 'Expected values to be loosely equal:', notDeepStrictEqual: 'Expected "actual" not to be strictly deep-equal to:', notStrictEqual: 'Expected "actual" to be strictly unequal to:', notStrictEqualObject: 'Expected "actual" not to be reference-equal to "expected":', notDeepEqual: 'Expected "actual" not to be loosely deep-equal to:', - notEqual: 'Expected "actual" to be loosely unequal to:', notIdentical: 'Values identical but not reference-equal:', + notDeepEqualUnequal: 'Expected values not to be loosely deep-equal:' }; // Comparing short primitives should just show === / !== instead of using the // diff. -const kMaxShortLength = 10; +const kMaxShortLength = 12; function copyError(source) { const keys = Object.keys(source); @@ -81,13 +80,12 @@ function createErrDiff(actual, expected, operator) { let i = 0; let indicator = ''; - // In case both values are objects explicitly mark them as not reference equal - // for the `strictEqual` operator. + // In case both values are objects or functions explicitly mark them as not + // reference equal for the `strictEqual` operator. if (operator === 'strictEqual' && - typeof actual === 'object' && - typeof expected === 'object' && - actual !== null && - expected !== null) { + ((typeof actual === 'object' && actual !== null && + typeof expected === 'object' && expected !== null) || + (typeof actual === 'function' && typeof expected === 'function'))) { operator = 'strictEqualObject'; } @@ -153,9 +151,9 @@ function createErrDiff(actual, expected, operator) { // Only remove lines in case it makes sense to collapse those. // TODO: Accept env to always show the full error. - if (actualLines.length > 30) { - actualLines[26] = `${blue}...${white}`; - while (actualLines.length > 27) { + if (actualLines.length > 50) { + actualLines[46] = `${blue}...${white}`; + while (actualLines.length > 47) { actualLines.pop(); } } @@ -282,8 +280,8 @@ function createErrDiff(actual, expected, operator) { } } } - // Inspected object to big (Show ~20 rows max) - if (printedLines > 20 && i < maxLines - 2) { + // Inspected object to big (Show ~50 rows max) + if (printedLines > 50 && i < maxLines - 2) { return `${msg}${skippedMsg}\n${res}\n${blue}...${white}${other}\n` + `${blue}...${white}`; } @@ -348,39 +346,41 @@ class AssertionError extends Error { let base = kReadableOperator[operator]; const res = inspectValue(actual).split('\n'); - // In case "actual" is an object, it should not be reference equal. + // In case "actual" is an object or a function, it should not be + // reference equal. if (operator === 'notStrictEqual' && - typeof actual === 'object' && - actual !== null) { + (typeof actual === 'object' && actual !== null || + typeof actual === 'function')) { base = kReadableOperator.notStrictEqualObject; } // Only remove lines in case it makes sense to collapse those. // TODO: Accept env to always show the full error. - if (res.length > 30) { - res[26] = `${blue}...${white}`; - while (res.length > 27) { + if (res.length > 50) { + res[46] = `${blue}...${white}`; + while (res.length > 47) { res.pop(); } } // Only print a single input. if (res.length === 1) { - super(`${base} ${res[0]}`); + super(`${base}${res[0].length > 5 ? '\n\n' : ' '}${res[0]}`); } else { super(`${base}\n\n${res.join('\n')}\n`); } } else { let res = inspectValue(actual); - let other = ''; + let other = inspectValue(expected); const knownOperators = kReadableOperator[operator]; - if (operator === 'notDeepEqual' || operator === 'notEqual') { - res = `${kReadableOperator[operator]}\n\n${res}`; + if ((operator === 'notDeepEqual' || operator === 'notEqual') && + res === other) { + res = `${knownOperators}\n\n${res}`; if (res.length > 1024) { res = `${res.slice(0, 1021)}...`; } + super(res); } else { - other = `${inspectValue(expected)}`; if (res.length > 512) { res = `${res.slice(0, 509)}...`; } @@ -388,12 +388,19 @@ class AssertionError extends Error { other = `${other.slice(0, 509)}...`; } if (operator === 'deepEqual' || operator === 'equal') { - res = `${knownOperators}\n\n${res}\n\nshould equal\n\n`; + const eq = operator === 'deepEqual' ? 'deep-equal' : 'equal'; + res = `${knownOperators}\n\n${res}\n\nshould loosely ${eq}\n\n`; } else { - other = ` ${operator} ${other}`; + const newOperator = kReadableOperator[`${operator}Unequal`]; + if (newOperator) { + const eq = operator === 'notDeepEqual' ? 'deep-equal' : 'equal'; + res = `${newOperator}\n\n${res}\n\nshould not loosely ${eq}\n\n`; + } else { + other = ` ${operator} ${other}`; + } } + super(`${res}${other}`); } - super(`${res}${other}`); } } diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 3675a18a284fd4..013c47121b95b0 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -195,7 +195,7 @@ function assertDeepAndStrictEqual(a, b) { function assertNotDeepOrStrict(a, b, err) { assert.throws( () => assert.deepEqual(a, b), - err || re`${a}\n\nshould equal\n\n${b}` + err || re`${a}\n\nshould loosely deep-equal\n\n${b}` ); assert.throws( () => assert.deepStrictEqual(a, b), @@ -204,7 +204,7 @@ function assertNotDeepOrStrict(a, b, err) { assert.throws( () => assert.deepEqual(b, a), - err || re`${b}\n\nshould equal\n\n${a}` + err || re`${b}\n\nshould loosely deep-equal\n\n${a}` ); assert.throws( () => assert.deepStrictEqual(b, a), @@ -651,6 +651,20 @@ assertDeepAndStrictEqual(-0, -0); assertDeepAndStrictEqual(a, b); } +assert.throws( + () => assert.notDeepEqual(1, true), + { + message: /1\n\nshould not loosely deep-equal\n\ntrue/ + } +); + +assert.throws( + () => assert.notDeepEqual(1, 1), + { + message: /Expected "actual" not to be loosely deep-equal to:\n\n1/ + } +); + assert.deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)); assert.throws(() => { assert.deepEqual(new Date(), new Date(2000, 3, 14)); }, @@ -779,7 +793,7 @@ assert.throws( () => assert.notDeepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)), { name: 'AssertionError', - message: 'Expected "actual" not to be strictly deep-equal to: ' + + message: 'Expected "actual" not to be strictly deep-equal to:\n\n' + util.inspect(new Date(2000, 3, 14)) } ); @@ -815,11 +829,11 @@ assert.throws( message: `${defaultMsgStartFull}\n\n+ /a/m\n- /a/` }); assert.throws( - () => assert.deepStrictEqual(/a/igm, /a/im), + () => assert.deepStrictEqual(/aa/igm, /aa/im), { code: 'ERR_ASSERTION', name: 'AssertionError', - message: `${defaultMsgStartFull}\n\n+ /a/gim\n- /a/im\n ^` + message: `${defaultMsgStartFull}\n\n+ /aa/gim\n- /aa/im\n ^` }); { @@ -939,12 +953,12 @@ assert.deepStrictEqual(obj1, obj2); } // Strict equal with identical objects that are not identical -// by reference and longer than 30 elements +// by reference and longer than 50 elements // E.g., assert.deepStrictEqual({ a: Symbol() }, { a: Symbol() }) { const a = {}; const b = {}; - for (let i = 0; i < 35; i++) { + for (let i = 0; i < 55; i++) { a[`symbol${i}`] = Symbol(); b[`symbol${i}`] = Symbol(); } diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 98f728acfa4ed0..b7d81f549547ed 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -80,12 +80,20 @@ assert.throws( assert.throws( () => a.notStrictEqual('a '.repeat(30), 'a '.repeat(30)), { - message: 'Expected "actual" to be strictly unequal to: ' + + message: 'Expected "actual" to be strictly unequal to:\n\n' + `'${'a '.repeat(30)}'`, name: 'AssertionError' } ); +assert.throws( + () => a.notEqual(1, 1), + { + message: '1 != 1', + operator: '!=' + } +); + a.notStrictEqual(2, '2'); // Testing the throwing. @@ -281,12 +289,12 @@ testShortAssertionMessage('a', '"a"'); testShortAssertionMessage('foo', '\'foo\''); testShortAssertionMessage(0, '0'); testShortAssertionMessage(Symbol(), 'Symbol()'); +testShortAssertionMessage(undefined, 'undefined'); +testShortAssertionMessage(-Infinity, '-Infinity'); testAssertionMessage([], '[]'); testAssertionMessage(/a/, '/a/'); testAssertionMessage(/abc/gim, '/abc/gim'); testAssertionMessage({}, '{}'); -testAssertionMessage(undefined, 'undefined'); -testAssertionMessage(-Infinity, '-Infinity'); testAssertionMessage([1, 2, 3], '[\n+ 1,\n+ 2,\n+ 3\n+ ]'); testAssertionMessage(function f() {}, '[Function: f]'); testAssertionMessage(function() {}, '[Function (anonymous)]'); @@ -579,12 +587,12 @@ assert.throws( `${actExp} ... Lines skipped\n` + '\n' + ' [\n' + - '+ 1,\n'.repeat(10) + + '+ 1,\n'.repeat(25) + '...\n' + - '- 2,\n'.repeat(10) + + '- 2,\n'.repeat(25) + '...'; assert.throws( - () => assert.deepEqual(Array(12).fill(1), Array(12).fill(2)), + () => assert.deepEqual(Array(28).fill(1), Array(28).fill(2)), { message }); const obj1 = {}; @@ -612,8 +620,8 @@ assert.throws( ); message = 'Expected "actual" not to be strictly deep-equal to:' + - `\n\n[${'\n 1,'.repeat(25)}\n...\n`; - const data = Array(31).fill(1); + `\n\n[${'\n 1,'.repeat(45)}\n...\n`; + const data = Array(51).fill(1); assert.throws( () => assert.notDeepEqual(data, data), { message }); diff --git a/test/pseudo-tty/test-assert-position-indicator.js b/test/pseudo-tty/test-assert-position-indicator.js index 26f82b5b13fb1b..e56299d2744761 100644 --- a/test/pseudo-tty/test-assert-position-indicator.js +++ b/test/pseudo-tty/test-assert-position-indicator.js @@ -13,6 +13,6 @@ assert.throws( // Confirm that there is a position indicator. assert.throws( - () => { assert.deepStrictEqual('aaa', 'aaaa'); }, + () => { assert.deepStrictEqual('aaaa', 'aaaaa'); }, (err) => err.message.includes('^') ); From d993bb9419f91256b6594ecec168053659cbec79 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 1 May 2019 23:54:02 +0200 Subject: [PATCH 3/4] assert: use less read operations This reduces the total amount of reads when using `assert.ok()` with a falsy value. That increases the read performance significantly. Also remove a comment that can not be addressed. --- lib/assert.js | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index dece9a08a78142..740afb8bdb5b31 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -172,8 +172,8 @@ function getCode(fd, line, column) { let lines = 0; // Prevent blocking the event loop by limiting the maximum amount of // data that may be read. - let maxReads = 64; // bytesPerRead * maxReads = 512 kb - const bytesPerRead = 8192; + let maxReads = 2 ** 5; // bytesPerRead * maxReads = 512 kb + const bytesPerRead = 2 ** 14; // Use a single buffer up front that is reused until the call site is found. let buffer = Buffer.allocUnsafe(bytesPerRead); while (maxReads-- !== 0) { @@ -619,11 +619,6 @@ function checkIsPromise(obj) { // Accept native ES6 promises and promises that are implemented in a similar // way. Do not accept thenables that use a function as `obj` and that have no // `catch` handler. - - // TODO: thenables are checked up until they have the correct methods, - // but according to documentation, the `then` method should receive - // the `fulfill` and `reject` arguments as well or it may be never resolved. - return isPromise(obj) || obj !== null && typeof obj === 'object' && typeof obj.then === 'function' && From 1ae35715cc344b05ffda402746fcec33a2718666 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 3 May 2019 13:13:41 +0200 Subject: [PATCH 4/4] fixup! assert: use less read operations --- lib/assert.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 740afb8bdb5b31..6e0b850b40c6a7 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -172,8 +172,8 @@ function getCode(fd, line, column) { let lines = 0; // Prevent blocking the event loop by limiting the maximum amount of // data that may be read. - let maxReads = 2 ** 5; // bytesPerRead * maxReads = 512 kb - const bytesPerRead = 2 ** 14; + let maxReads = 32; // bytesPerRead * maxReads = 512 kb + const bytesPerRead = 16384; // Use a single buffer up front that is reused until the call site is found. let buffer = Buffer.allocUnsafe(bytesPerRead); while (maxReads-- !== 0) {