From 386f664dd81e023c9244f3c5d691dcbb1a775826 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 9 Apr 2018 00:38:41 +0200 Subject: [PATCH] assert: validate the block return type This makes sure the returned value when calling `block` is actually of type promise in case `assert.rejects` or `assert.doesNotReject` is called. --- doc/api/assert.md | 14 ++++++++--- doc/api/errors.md | 6 +++++ lib/assert.js | 8 ++++++- lib/internal/errors.js | 10 ++++++++ test/parallel/test-assert-async.js | 37 +++++++++++++++++++----------- 5 files changed, 58 insertions(+), 17 deletions(-) diff --git a/doc/api/assert.md b/doc/api/assert.md index 6d96359d005697..5d84484df5c1d1 100644 --- a/doc/api/assert.md +++ b/doc/api/assert.md @@ -387,8 +387,13 @@ function and awaits the returned promise to complete. It will then check that the promise is not rejected. If `block` is a function and it throws an error synchronously, -`assert.doesNotReject()` will return a rejected Promise with that error without -checking the error handler. +`assert.doesNotReject()` will return a rejected Promise with that error. If the +function does not return a promise, `assert.doesNotReject()` will return a +rejected Promise with an [`ERR_INVALID_RETURN_VALUE`][] error. In both cases +without checking the error handler. + +If `block` is a function and it does not return a promise, an +[`ERR_INVALID_RETURN_VALUE`][] error is going to be rejected. Please note: Using `assert.doesNotReject()` is actually not useful because there is little benefit by catching a rejection and then rejecting it again. Instead, @@ -929,7 +934,9 @@ function and awaits the returned promise to complete. It will then check that the promise is rejected. If `block` is a function and it throws an error synchronously, -`assert.rejects()` will return a rejected Promise with that error without +`assert.rejects()` will return a rejected Promise with that error. If the +function does not return a promise, `assert.rejects()` will return a rejected +Promise with an [`ERR_INVALID_RETURN_VALUE`][] error. In both cases without checking the error handler. Besides the async nature to await the completion behaves identically to @@ -1138,6 +1145,7 @@ assert.throws(throwingFirst, /Second$/); 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. +[`ERR_INVALID_RETURN_VALUE`]: errors.html#errors_err_invalid_return_value [`Class`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes [`Error.captureStackTrace`]: errors.html#errors_error_capturestacktrace_targetobject_constructoropt [`Error`]: errors.html#errors_class_error diff --git a/doc/api/errors.md b/doc/api/errors.md index 49834414decffe..80f458ace7b6de 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1173,6 +1173,12 @@ An invalid `options.protocol` was passed. Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which is not supported. + +### ERR_INVALID_RETURN_VALUE + +Thrown in case a function option does not return a expected value on execution. +For example when a function is expected to return a promise. + ### ERR_INVALID_SYNC_FORK_INPUT diff --git a/lib/assert.js b/lib/assert.js index 62847e55c3e26f..05b43b5b54d074 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -30,7 +30,8 @@ const { errorCache, codes: { ERR_AMBIGUOUS_ARGUMENT, - ERR_INVALID_ARG_TYPE + ERR_INVALID_ARG_TYPE, + ERR_INVALID_RETURN_VALUE } } = require('internal/errors'); const { openSync, closeSync, readSync } = require('fs'); @@ -455,6 +456,11 @@ async function waitForActual(block) { if (typeof block === 'function') { // Return a rejected promise if `block` throws synchronously. resultPromise = block(); + // Fail in case no promise is returned. + if (!checkIsPromise(resultPromise)) { + throw new ERR_INVALID_RETURN_VALUE('instance of Promise', + 'block', resultPromise); + } } else if (checkIsPromise(block)) { resultPromise = block; } else { diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 35c3fb74b08216..770922282b3f11 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -903,6 +903,16 @@ E('ERR_INVALID_PERFORMANCE_MARK', E('ERR_INVALID_PROTOCOL', 'Protocol "%s" not supported. Expected "%s"', Error); E('ERR_INVALID_REPL_EVAL_CONFIG', 'Cannot specify both "breakEvalOnSigint" and "eval" for REPL', TypeError); +E('ERR_INVALID_RETURN_VALUE', (input, name, value) => { + let type; + if (value && value.constructor && value.constructor.name) { + type = `instance of ${value.constructor.name}`; + } else { + type = `type ${typeof value}`; + } + return `Expected ${input} to be returned from the "${name}"` + + ` function but got ${type}.`; +}, TypeError); E('ERR_INVALID_SYNC_FORK_INPUT', 'Asynchronous forks do not support Buffer, Uint8Array or string input: %s', TypeError); diff --git a/test/parallel/test-assert-async.js b/test/parallel/test-assert-async.js index 00a4fe7c722d7a..4b3664e0cbdf15 100644 --- a/test/parallel/test-assert-async.js +++ b/test/parallel/test-assert-async.js @@ -29,20 +29,25 @@ const promises = []; `${err.name} is not instance of AssertionError`); assert.strictEqual(err.code, 'ERR_ASSERTION'); assert.strictEqual(err.message, - 'Missing expected rejection (handler).'); + 'Missing expected rejection (mustNotCall).'); assert.strictEqual(err.operator, 'rejects'); assert.ok(!err.stack.includes('at Function.rejects')); return true; }; - let promise = assert.rejects(async () => {}, handler); - promises.push(assert.rejects(promise, handler)); + let promise = assert.rejects(async () => {}, common.mustNotCall()); + promises.push(assert.rejects(promise, common.mustCall(handler))); - promise = assert.rejects(() => {}, handler); - promises.push(assert.rejects(promise, handler)); + promise = assert.rejects(() => {}, common.mustNotCall()); + promises.push(assert.rejects(promise, { + name: 'TypeError [ERR_INVALID_RETURN_VALUE]', + code: 'ERR_INVALID_RETURN_VALUE', + message: 'Expected instance of Promise to be returned ' + + 'from the "block" function but got type undefined.' + })); - promise = assert.rejects(Promise.resolve(), handler); - promises.push(assert.rejects(promise, handler)); + promise = assert.rejects(Promise.resolve(), common.mustNotCall()); + promises.push(assert.rejects(promise, common.mustCall(handler))); } { @@ -67,7 +72,13 @@ promises.push(assert.rejects( // Check `assert.doesNotReject`. { // `assert.doesNotReject` accepts a function or a promise as first argument. - promises.push(assert.doesNotReject(() => {})); + const promise = assert.doesNotReject(() => new Map(), common.mustNotCall()); + promises.push(assert.rejects(promise, { + message: 'Expected instance of Promise to be returned ' + + 'from the "block" function but got instance of Map.', + code: 'ERR_INVALID_RETURN_VALUE', + name: 'TypeError [ERR_INVALID_RETURN_VALUE]' + })); promises.push(assert.doesNotReject(async () => {})); promises.push(assert.doesNotReject(Promise.resolve())); } @@ -93,14 +104,14 @@ promises.push(assert.rejects( const rejectingFn = async () => assert.fail(); - let promise = assert.doesNotReject(rejectingFn, handler1); - promises.push(assert.rejects(promise, handler2)); + let promise = assert.doesNotReject(rejectingFn, common.mustCall(handler1)); + promises.push(assert.rejects(promise, common.mustCall(handler2))); - promise = assert.doesNotReject(rejectingFn(), handler1); - promises.push(assert.rejects(promise, handler2)); + promise = assert.doesNotReject(rejectingFn(), common.mustCall(handler1)); + promises.push(assert.rejects(promise, common.mustCall(handler2))); promise = assert.doesNotReject(() => assert.fail(), common.mustNotCall()); - promises.push(assert.rejects(promise, handler1)); + promises.push(assert.rejects(promise, common.mustCall(handler1))); } promises.push(assert.rejects(