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.

PR-URL: https://github.com/nodejs/node/pull/19886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

5 files changed, 56 insertions, 18 deletions

diff --git a/doc/api/assert.md b/doc/api/assert.md
index 6d96359d00..62c96801d3 100644
--- a/doc/api/assert.md
+++ b/doc/api/assert.md
@@ -387,8 +387,10 @@ 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 the
+error handler is skipped.
 
 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,8 +931,10 @@ 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
-checking the error handler.
+`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 the error
+handler is skipped.
 
 Besides the async nature to await the completion behaves identically to
 [`assert.throws()`][].
@@ -1138,6 +1142,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 84a74c51e0..df81718e7d 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1186,6 +1186,12 @@ An invalid `options.protocol` was passed.
 Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which
 is not supported.
 
+<a id="ERR_INVALID_RETURN_VALUE"></a>
+### ERR_INVALID_RETURN_VALUE
+
+Thrown in case a function option does not return an expected value on execution.
+For example when a function is expected to return a promise.
+
 <a id="ERR_INVALID_SYNC_FORK_INPUT"></a>
 ### ERR_INVALID_SYNC_FORK_INPUT
 
diff --git a/lib/assert.js b/lib/assert.js
index 62847e55c3..05b43b5b54 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 4823f0b65c..88c7f86eb9 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -904,6 +904,16 @@ E('ERR_INVALID_PROTOCOL',
   TypeError);
 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 00a4fe7c72..4b3664e0cb 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(