Skip to content

Commit

Permalink
assert: make sure throws is able to handle primitives
Browse files Browse the repository at this point in the history
This fixes some possible issues with `assert.throws` and
`assert.rejects` in combination with an validation object. It will
now properly handle primitive values being thrown as error.

It also makes sure the `generatedMessage` property is properly set
if `assert.throws` or `assert.rejects` is used in combination with
an validation object and improves the error performance in such cases
by only creating the error once.

In addition it will fix detecting regular expressions from a different
context such as n-api that are passed through as validator for
`assert.throws` or `assert.rejects`. Until now those were not tested.

PR-URL: nodejs#20482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
  • Loading branch information
BridgeAR committed May 10, 2018
1 parent 5e6ca89 commit 560925f
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 7 deletions.
30 changes: 25 additions & 5 deletions lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -382,16 +382,16 @@ function compareExceptionKey(actual, expected, key, message, keys) {
const a = new Comparison(actual, keys);
const b = new Comparison(expected, keys, actual);

const tmpLimit = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
const err = new AssertionError({
actual: a,
expected: b,
operator: 'deepStrictEqual',
stackStartFn: assert.throws
});
Error.stackTraceLimit = tmpLimit;
message = err.message;
err.actual = actual;
err.expected = expected;
err.operator = 'throws';
throw err;
}
innerFail({
actual,
Expand All @@ -405,14 +405,34 @@ function compareExceptionKey(actual, expected, key, message, keys) {

function expectedException(actual, expected, msg) {
if (typeof expected !== 'function') {
if (expected instanceof RegExp)
if (isRegExp(expected))
return expected.test(actual);
// assert.doesNotThrow does not accept objects.
if (arguments.length === 2) {
throw new ERR_INVALID_ARG_TYPE(
'expected', ['Function', 'RegExp'], expected
);
}

// TODO: Disallow primitives as error argument.
// This is here to prevent a breaking change.
if (typeof expected !== 'object') {
return true;
}

// Handle primitives properly.
if (typeof actual !== 'object' || actual === null) {
const err = new AssertionError({
actual,
expected,
message: msg,
operator: 'deepStrictEqual',
stackStartFn: assert.throws
});
err.operator = 'throws';
throw err;
}

const keys = Object.keys(expected);
// Special handle errors to make sure the name and the message are compared
// as well.
Expand Down
4 changes: 2 additions & 2 deletions test/message/assert_throws_stack.out
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
assert.js:*
throw new AssertionError(obj);
^
throw err;
^

AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual
Expand Down
45 changes: 45 additions & 0 deletions test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -740,7 +740,9 @@ common.expectsError(
const frames = err.stack.split('\n');
const [, filename, line, column] = frames[1].match(/\((.+):(\d+):(\d+)\)/);
// Reset the cache to check again
const size = errorCache.size;
errorCache.delete(`${filename}${line - 1}${column - 1}`);
assert.strictEqual(errorCache.size, size - 1);
const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` +
'ok(failed(badly));';
try {
Expand Down Expand Up @@ -849,6 +851,7 @@ common.expectsError(
{
name: 'AssertionError [ERR_ASSERTION]',
code: 'ERR_ASSERTION',
generatedMessage: true,
message: `${start}\n${actExp}\n\n` +
" Comparison {\n name: 'Error',\n- message: 'foo'" +
"\n+ message: ''\n }"
Expand Down Expand Up @@ -940,3 +943,45 @@ assert.throws(
' }'
}
);

{
let actual = null;
const expected = { message: 'foo' };
assert.throws(
() => assert.throws(
() => { throw actual; },
expected
),
{
operator: 'throws',
actual,
expected,
generatedMessage: true,
message: `${start}\n${actExp}\n\n` +
'- null\n' +
'+ {\n' +
"+ message: 'foo'\n" +
'+ }'
}
);

actual = 'foobar';
const message = 'message';
assert.throws(
() => assert.throws(
() => { throw actual; },
{ message: 'foobar' },
message
),
{
actual,
message,
operator: 'throws',
generatedMessage: false
}
);
}

// TODO: This case is only there to make sure there is no breaking change.
// eslint-disable-next-line no-restricted-syntax, no-throw-literal
assert.throws(() => { throw 4; }, 4);

0 comments on commit 560925f

Please sign in to comment.