From 147aeedc8d6d58313778d3bb030aad7858b84d48 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 9 Dec 2017 21:47:49 -0200 Subject: [PATCH] assert: improve assert.throws Throw a TypeError in case a error message is provided in the second argument and a third argument is present as well. This is clearly a mistake and should not be done. Backport-PR-URL: https://github.com/nodejs/node/pull/23223 PR-URL: https://github.com/nodejs/node/pull/17585 Reviewed-By: Rich Trott Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil Reviewed-By: Evan Lucas --- doc/api/assert.md | 37 +++++++++++--- lib/assert.js | 95 +++++++++++++++++++----------------- test/parallel/test-assert.js | 13 ++++- 3 files changed, 93 insertions(+), 52 deletions(-) diff --git a/doc/api/assert.md b/doc/api/assert.md index d337a09b328921..b1c7168e94c7d7 100644 --- a/doc/api/assert.md +++ b/doc/api/assert.md @@ -691,17 +691,42 @@ assert.throws( Note that `error` can not be a string. If a string is provided as the second argument, then `error` is assumed to be omitted and the string will be used for -`message` instead. This can lead to easy-to-miss mistakes: +`message` instead. This can lead to easy-to-miss mistakes. Please read the +example below carefully if using a string as the second argument gets +considered: ```js -// THIS IS A MISTAKE! DO NOT DO THIS! -assert.throws(myFunction, 'missing foo', 'did not throw with expected message'); - -// Do this instead. -assert.throws(myFunction, /missing foo/, 'did not throw with expected message'); +function throwingFirst() { + throw new Error('First'); +} +function throwingSecond() { + throw new Error('Second'); +} +function notThrowing() {} + +// The second argument is a string and the input function threw an Error. +// In that case both cases do not throw as neither is going to try to +// match for the error message thrown by the input function! +assert.throws(throwingFirst, 'Second'); +assert.throws(throwingSecond, 'Second'); + +// The string is only used (as message) in case the function does not throw: +assert.throws(notThrowing, 'Second'); +// AssertionError [ERR_ASSERTION]: Missing expected exception: Second + +// If it was intended to match for the error message do this instead: +assert.throws(throwingSecond, /Second$/); +// Does not throw because the error messages match. +assert.throws(throwingFirst, /Second$/); +// Throws a error: +// Error: First +// at throwingFirst (repl:2:9) ``` +Due to the confusing notation, it is recommended not to use a string as the +second argument. This might lead to difficult-to-spot errors. + ## Caveats For the following cases, consider using ES2015 [`Object.is()`][], diff --git a/lib/assert.js b/lib/assert.js index 79c61fa7559a16..b0513c9795bed5 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -675,7 +675,11 @@ function expectedException(actual, expected) { return expected.call({}, actual) === true; } -function tryBlock(block) { +function getActual(block) { + if (typeof block !== 'function') { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'block', 'Function', + block); + } try { block(); } catch (e) { @@ -683,60 +687,61 @@ function tryBlock(block) { } } -function innerThrows(shouldThrow, block, expected, message) { - var details = ''; +// Expected to throw an error. +assert.throws = function throws(block, error, message) { + const actual = getActual(block); - if (typeof block !== 'function') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'block', 'function', - block); - } + if (typeof error === 'string') { + if (arguments.length === 3) + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'error', + ['Function', 'RegExp'], + error); - if (typeof expected === 'string') { - message = expected; - expected = null; + message = error; + error = null; } - const actual = tryBlock(block); - - if (shouldThrow === true) { - if (actual === undefined) { - if (expected && expected.name) { - details += ` (${expected.name})`; - } - details += message ? `: ${message}` : '.'; - innerFail({ - actual, - expected, - operator: 'throws', - message: `Missing expected exception${details}`, - stackStartFn: assert.throws - }); - } - if (expected && expectedException(actual, expected) === false) { - throw actual; - } - } else if (actual !== undefined) { - if (!expected || expectedException(actual, expected)) { - details = message ? `: ${message}` : '.'; - innerFail({ - actual, - expected, - operator: 'doesNotThrow', - message: `Got unwanted exception${details}`, - stackStartFn: assert.doesNotThrow - }); + if (actual === undefined) { + let details = ''; + if (error && error.name) { + details += ` (${error.name})`; } + details += message ? `: ${message}` : '.'; + innerFail({ + actual, + expected: error, + operator: 'throws', + message: `Missing expected exception${details}`, + stackStartFn: throws + }); + } + if (error && expectedException(actual, error) === false) { throw actual; } -} - -// Expected to throw an error. -assert.throws = function throws(block, error, message) { - innerThrows(true, block, error, message); }; assert.doesNotThrow = function doesNotThrow(block, error, message) { - innerThrows(false, block, error, message); + const actual = getActual(block); + if (actual === undefined) + return; + + if (typeof error === 'string') { + message = error; + error = null; + } + + if (!error || expectedException(actual, error)) { + const details = message ? `: ${message}` : '.'; + innerFail({ + actual, + expected: error, + operator: 'doesNotThrow', + message: `Got unwanted exception${details}\n${actual.message}`, + stackStartFn: doesNotThrow + }); + } + throw actual; }; assert.ifError = function ifError(err) { if (err) throw err; }; diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index a8ba1bf5d1fc6d..72606a911e5a15 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -640,7 +640,7 @@ try { common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError, - message: 'The "block" argument must be of type function. Received ' + + message: 'The "block" argument must be of type Function. Received ' + `type ${typeName(block)}` })(e); } @@ -731,3 +731,14 @@ common.expectsError( message: 'null == true' } ); + +common.expectsError( + // eslint-disable-next-line no-restricted-syntax + () => assert.throws(() => {}, 'Error message', 'message'), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "error" argument must be one of type Function or RegExp. ' + + 'Received type string' + } +);