Skip to content

Commit

Permalink
assert: improve assert.throws
Browse files Browse the repository at this point in the history
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: #19230
PR-URL: #17585
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
  • Loading branch information
BridgeAR authored and MylesBorins committed Mar 20, 2018
1 parent 1e5c7e3 commit 612ba1a
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 51 deletions.
37 changes: 31 additions & 6 deletions doc/api/assert.md
Original file line number Diff line number Diff line change
Expand Up @@ -765,17 +765,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:

<!-- eslint-disable no-restricted-syntax -->
```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()`][],
Expand Down
95 changes: 50 additions & 45 deletions lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,68 +211,73 @@ 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) {
return e;
}
}

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}\n${actual.message}`,
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; };
Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -773,3 +773,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'
}
);

0 comments on commit 612ba1a

Please sign in to comment.