assert: improve assert.throws

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.

PR-URL: https://github.com/nodejs/node/pull/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>

3 files changed, 92 insertions, 51 deletions

diff --git a/doc/api/assert.md b/doc/api/assert.md
index cf2212c90e..7ad56accc2 100644
--- a/doc/api/assert.md
+++ b/doc/api/assert.md
@@ -767,17 +767,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.
+
 [`Error.captureStackTrace`]: errors.html#errors_error_capturestacktrace_targetobject_constructoropt
 [`Map`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Map
 [`Object.is()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
diff --git a/lib/assert.js b/lib/assert.js
index 00542b53fc..916efcd1ef 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -208,7 +208,11 @@ 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) {
@@ -216,60 +220,61 @@ function tryBlock(block) {
   }
 }
 
-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; };
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index 3078d1ce51..27714e22fc 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -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'
+  }
+);