From b3c1bd38f6e51e381144eff26e5ad8b2c486e3d4 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sun, 8 Apr 2018 23:28:30 +0200
Subject: [PATCH] assert: add direct promises support in rejects
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This adds direct promise support to `assert.rejects` and
`assert.doesNotReject`. It will now accept both, functions and ES2015
promises as input.

Besides this the documentation was updated to reflect the latest
changes.

It also refactors the tests to a non blocking way to improve the
execution performance and improves the coverage.

PR-URL: https://github.com/nodejs/node/pull/19885
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
---
 doc/api/assert.md                  |  54 ++++++----
 lib/assert.js                      |  53 +++++-----
 test/parallel/test-assert-async.js | 156 ++++++++++++++++++-----------
 3 files changed, 161 insertions(+), 102 deletions(-)

diff --git a/doc/api/assert.md b/doc/api/assert.md
index 6a587052f2307c..c868ef64e0a2c2 100644
--- a/doc/api/assert.md
+++ b/doc/api/assert.md
@@ -378,22 +378,23 @@ parameter is an instance of an [`Error`][] then it will be thrown instead of the
 <!-- YAML
 added: REPLACEME
 -->
-* `block` {Function}
+* `block` {Function|Promise}
 * `error` {RegExp|Function}
 * `message` {any}
 
-Awaits for the promise returned by function `block` to complete and not be
-rejected.
+Awaits the `block` promise or, if `block` is a function, immediately calls the
+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.
 
 Please note: Using `assert.doesNotReject()` is actually not useful because there
 is little benefit by catching a rejection and then rejecting it again. Instead,
 consider adding a comment next to the specific code path that should not reject
 and keep error messages as expressive as possible.
 
-When `assert.doesNotReject()` is called, it will immediately call the `block`
-function, and awaits for completion. See [`assert.rejects()`][] for more
-details.
-
 Besides the async nature to await the completion behaves identically to
 [`assert.doesNotThrow()`][].
 
@@ -409,12 +410,10 @@ Besides the async nature to await the completion behaves identically to
 ```
 
 ```js
-assert.doesNotReject(
-  () => Promise.reject(new TypeError('Wrong value')),
-  SyntaxError
-).then(() => {
-  // ...
-});
+assert.doesNotReject(Promise.reject(new TypeError('Wrong value')))
+  .then(() => {
+    // ...
+  });
 ```
 
 ## assert.doesNotThrow(block[, error][, message])
@@ -916,14 +915,17 @@ assert(0);
 <!-- YAML
 added: REPLACEME
 -->
-* `block` {Function}
+* `block` {Function|Promise}
 * `error` {RegExp|Function|Object}
 * `message` {any}
 
-Awaits for promise returned by function `block` to be rejected.
+Awaits the `block` promise or, if `block` is a function, immediately calls the
+function and awaits the returned promise to complete. It will then check that
+the promise is rejected.
 
-When `assert.rejects()` is called, it will immediately call the `block`
-function, and awaits for completion.
+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.
 
 Besides the async nature to await the completion behaves identically to
 [`assert.throws()`][].
@@ -938,22 +940,31 @@ the block fails to reject.
 (async () => {
   await assert.rejects(
     async () => {
-      throw new Error('Wrong value');
+      throw new TypeError('Wrong value');
     },
-    Error
+    {
+      name: 'TypeError',
+      message: 'Wrong value'
+    }
   );
 })();
 ```
 
 ```js
 assert.rejects(
-  () => Promise.reject(new Error('Wrong value')),
+  Promise.reject(new Error('Wrong value')),
   Error
 ).then(() => {
   // ...
 });
 ```
 
+Note that `error` cannot 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. Please read the
+example in [`assert.throws()`][] carefully if using a string as the second
+argument gets considered.
+
 ## assert.strictEqual(actual, expected[, message])
 <!-- YAML
 added: v0.1.21
@@ -1069,7 +1080,7 @@ assert.throws(
 );
 ```
 
-Note that `error` can not be a string. If a string is provided as the second
+Note that `error` cannot 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. Please read the
 example below carefully if using a string as the second argument gets
@@ -1123,7 +1134,6 @@ second argument. This might lead to difficult-to-spot errors.
 [`assert.notDeepStrictEqual()`]: #assert_assert_notdeepstrictequal_actual_expected_message
 [`assert.notStrictEqual()`]: #assert_assert_notstrictequal_actual_expected_message
 [`assert.ok()`]: #assert_assert_ok_value_message
-[`assert.rejects()`]: #assert_assert_rejects_block_error_message
 [`assert.strictEqual()`]: #assert_assert_strictequal_actual_expected_message
 [`assert.throws()`]: #assert_assert_throws_block_error_message
 [`strict mode`]: #assert_strict_mode
diff --git a/lib/assert.js b/lib/assert.js
index 2164c22958df6d..62847e55c3e26f 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -34,7 +34,7 @@ const {
   }
 } = require('internal/errors');
 const { openSync, closeSync, readSync } = require('fs');
-const { inspect } = require('util');
+const { inspect, types: { isPromise } } = require('util');
 const { EOL } = require('os');
 const { NativeModule } = require('internal/bootstrap/loaders');
 
@@ -440,13 +440,27 @@ function getActual(block) {
   return NO_EXCEPTION_SENTINEL;
 }
 
+function checkIsPromise(obj) {
+  // Accept native ES6 promises and promises that are implemented in a similar
+  // way. Do not accept thenables that use a function as `obj` and that have no
+  // `catch` handler.
+  return isPromise(obj) ||
+    obj !== null && typeof obj === 'object' &&
+    typeof obj.then === 'function' &&
+    typeof obj.catch === 'function';
+}
+
 async function waitForActual(block) {
-  if (typeof block !== 'function') {
-    throw new ERR_INVALID_ARG_TYPE('block', 'Function', block);
+  let resultPromise;
+  if (typeof block === 'function') {
+    // Return a rejected promise if `block` throws synchronously.
+    resultPromise = block();
+  } else if (checkIsPromise(block)) {
+    resultPromise = block;
+  } else {
+    throw new ERR_INVALID_ARG_TYPE('block', ['Function', 'Promise'], block);
   }
 
-  // Return a rejected promise if `block` throws synchronously.
-  const resultPromise = block();
   try {
     await resultPromise;
   } catch (e) {
@@ -485,7 +499,7 @@ function expectsError(stackStartFn, actual, error, message) {
       details += ` (${error.name})`;
     }
     details += message ? `: ${message}` : '.';
-    const fnType = stackStartFn === rejects ? 'rejection' : 'exception';
+    const fnType = stackStartFn.name === 'rejects' ? 'rejection' : 'exception';
     innerFail({
       actual: undefined,
       expected: error,
@@ -510,7 +524,8 @@ function expectsNoError(stackStartFn, actual, error, message) {
 
   if (!error || expectedException(actual, error)) {
     const details = message ? `: ${message}` : '.';
-    const fnType = stackStartFn === doesNotReject ? 'rejection' : 'exception';
+    const fnType = stackStartFn.name === 'doesNotReject' ?
+      'rejection' : 'exception';
     innerFail({
       actual,
       expected: error,
@@ -523,29 +538,21 @@ function expectsNoError(stackStartFn, actual, error, message) {
   throw actual;
 }
 
-function throws(block, ...args) {
+assert.throws = function throws(block, ...args) {
   expectsError(throws, getActual(block), ...args);
-}
-
-assert.throws = throws;
+};
 
-async function rejects(block, ...args) {
+assert.rejects = async function rejects(block, ...args) {
   expectsError(rejects, await waitForActual(block), ...args);
-}
-
-assert.rejects = rejects;
+};
 
-function doesNotThrow(block, ...args) {
+assert.doesNotThrow = function doesNotThrow(block, ...args) {
   expectsNoError(doesNotThrow, getActual(block), ...args);
-}
-
-assert.doesNotThrow = doesNotThrow;
+};
 
-async function doesNotReject(block, ...args) {
+assert.doesNotReject = async function doesNotReject(block, ...args) {
   expectsNoError(doesNotReject, await waitForActual(block), ...args);
-}
-
-assert.doesNotReject = doesNotReject;
+};
 
 assert.ifError = function ifError(err) {
   if (err !== null && err !== undefined) {
diff --git a/test/parallel/test-assert-async.js b/test/parallel/test-assert-async.js
index cc557d504ac6f2..2a865c121f36da 100644
--- a/test/parallel/test-assert-async.js
+++ b/test/parallel/test-assert-async.js
@@ -1,74 +1,116 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-const { promisify } = require('util');
-const wait = promisify(setTimeout);
-
-/* eslint-disable prefer-common-expectserror, no-restricted-properties */
 
 // Test assert.rejects() and assert.doesNotReject() by checking their
 // expected output and by verifying that they do not work sync
 
 common.crashOnUnhandledRejection();
 
-(async () => {
-  await assert.rejects(
-    async () => assert.fail(),
-    common.expectsError({
-      code: 'ERR_ASSERTION',
-      type: assert.AssertionError,
-      message: 'Failed'
-    })
-  );
+// Run all tests in parallel and check their outcome at the end.
+const promises = [];
 
-  await assert.doesNotReject(() => {});
+// Check `assert.rejects`.
+{
+  const rejectingFn = async () => assert.fail();
+  const errObj = {
+    code: 'ERR_ASSERTION',
+    name: 'AssertionError [ERR_ASSERTION]',
+    message: 'Failed'
+  };
+  // `assert.rejects` accepts a function or a promise as first argument.
+  promises.push(assert.rejects(rejectingFn, errObj));
+  promises.push(assert.rejects(rejectingFn(), errObj));
+}
 
-  {
-    const promise = assert.doesNotReject(async () => {
-      await wait(1);
-      throw new Error();
-    });
-    await assert.rejects(
-      () => promise,
-      (err) => {
-        assert(err instanceof assert.AssertionError,
-               `${err.name} is not instance of AssertionError`);
-        assert.strictEqual(err.code, 'ERR_ASSERTION');
-        assert.strictEqual(err.message,
-                           'Got unwanted rejection.\nActual message: ""');
-        assert.strictEqual(err.operator, 'doesNotReject');
-        assert.ok(!err.stack.includes('at Function.doesNotReject'));
-        return true;
-      }
-    );
-  }
+{
+  const handler = (err) => {
+    assert(err instanceof assert.AssertionError,
+           `${err.name} is not instance of AssertionError`);
+    assert.strictEqual(err.code, 'ERR_ASSERTION');
+    assert.strictEqual(err.message,
+                       'Missing expected rejection (handler).');
+    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));
+
+  promise = assert.rejects(() => {}, handler);
+  promises.push(assert.rejects(promise, handler));
+
+  promise = assert.rejects(Promise.resolve(), handler);
+  promises.push(assert.rejects(promise, handler));
+}
+
+{
+  const THROWN_ERROR = new Error();
+
+  promises.push(assert.rejects(() => {
+    throw THROWN_ERROR;
+  }).catch(common.mustCall((err) => {
+    assert.strictEqual(err, THROWN_ERROR);
+  })));
+}
 
+promises.push(assert.rejects(
+  assert.rejects('fail', {}),
   {
-    const promise = assert.rejects(() => {});
-    await assert.rejects(
-      () => promise,
-      (err) => {
-        assert(err instanceof assert.AssertionError,
-               `${err.name} is not instance of AssertionError`);
-        assert.strictEqual(err.code, 'ERR_ASSERTION');
-        assert(/^Missing expected rejection\.$/.test(err.message));
-        assert.strictEqual(err.operator, 'rejects');
-        assert.ok(!err.stack.includes('at Function.rejects'));
-        return true;
-      }
-    );
+    code: 'ERR_INVALID_ARG_TYPE',
+    message: 'The "block" argument must be one of type ' +
+             'Function or Promise. Received type string'
   }
+));
 
+// Check `assert.doesNotReject`.
+{
+  // `assert.doesNotReject` accepts a function or a promise as first argument.
+  promises.push(assert.doesNotReject(() => {}));
+  promises.push(assert.doesNotReject(async () => {}));
+  promises.push(assert.doesNotReject(Promise.resolve()));
+}
+
+{
+  const handler1 = (err) => {
+    assert(err instanceof assert.AssertionError,
+           `${err.name} is not instance of AssertionError`);
+    assert.strictEqual(err.code, 'ERR_ASSERTION');
+    assert.strictEqual(err.message, 'Failed');
+    return true;
+  };
+  const handler2 = (err) => {
+    assert(err instanceof assert.AssertionError,
+           `${err.name} is not instance of AssertionError`);
+    assert.strictEqual(err.code, 'ERR_ASSERTION');
+    assert.strictEqual(err.message,
+                       'Got unwanted rejection.\nActual message: "Failed"');
+    assert.strictEqual(err.operator, 'doesNotReject');
+    assert.ok(!err.stack.includes('at Function.doesNotReject'));
+    return true;
+  };
+
+  const rejectingFn = async () => assert.fail();
+
+  let promise = assert.doesNotReject(rejectingFn, handler1);
+  promises.push(assert.rejects(promise, handler2));
+
+  promise = assert.doesNotReject(rejectingFn(), handler1);
+  promises.push(assert.rejects(promise, handler2));
+
+  promise = assert.doesNotReject(() => assert.fail(), common.mustNotCall());
+  promises.push(assert.rejects(promise, handler1));
+}
+
+promises.push(assert.rejects(
+  assert.doesNotReject(123),
   {
-    const THROWN_ERROR = new Error();
-
-    await assert.rejects(() => {
-      throw THROWN_ERROR;
-    }).then(common.mustNotCall())
-      .catch(
-        common.mustCall((err) => {
-          assert.strictEqual(err, THROWN_ERROR);
-        })
-      );
+    code: 'ERR_INVALID_ARG_TYPE',
+    message: 'The "block" argument must be one of type ' +
+             'Function or Promise. Received type number'
   }
-})().then(common.mustCall());
+));
+
+// Make sure all async code gets properly executed.
+Promise.all(promises).then(common.mustCall());