From ef8f147b7e7d53d73ce7bc9f39294a1a83e686d3 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sun, 19 May 2019 23:23:18 +0200
Subject: [PATCH] assert: improve regular expression validation

This makes sure `assert.throws()` and `assert.rejects()` result in
an easy to understand error message instead of rethrowing the actual
error. This should significantly improve the debugging experience in
case people use an regular expression to validate their errors.

This also adds support for primitive errors that would have caused
runtime errors using the mentioned functions. The input is now
stringified before it's passed to the RegExp to circumvent that.

As drive-by change this also adds some further comments and renames
a variable for clarity.

PR-URL: https://github.com/nodejs/node/pull/27781
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 lib/assert.js                | 71 +++++++++++++++++++++++++++---------
 test/parallel/test-assert.js | 22 ++++++++++-
 2 files changed, 75 insertions(+), 18 deletions(-)

diff --git a/lib/assert.js b/lib/assert.js
index 6e0b850b40c6a7..fadc3ad530f974 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -549,15 +549,22 @@ function compareExceptionKey(actual, expected, key, message, keys, fn) {
   }
 }
 
-function expectedException(actual, expected, msg, fn) {
+function expectedException(actual, expected, message, fn) {
   if (typeof expected !== 'function') {
-    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
-      );
+    // Handle regular expressions.
+    if (isRegExp(expected)) {
+      const str = String(actual);
+      if (expected.test(str))
+        return;
+
+      throw new AssertionError({
+        actual,
+        expected,
+        message: message || 'The input did not match the regular expression ' +
+                            `${inspect(expected)}. Input:\n\n${inspect(str)}\n`,
+        operator: fn.name,
+        stackStartFn: fn
+      });
     }
 
     // Handle primitives properly.
@@ -565,7 +572,7 @@ function expectedException(actual, expected, msg, fn) {
       const err = new AssertionError({
         actual,
         expected,
-        message: msg,
+        message,
         operator: 'deepStrictEqual',
         stackStartFn: fn
       });
@@ -573,6 +580,7 @@ function expectedException(actual, expected, msg, fn) {
       throw err;
     }
 
+    // Handle validation objects.
     const keys = Object.keys(expected);
     // Special handle errors to make sure the name and the message are compared
     // as well.
@@ -589,18 +597,25 @@ function expectedException(actual, expected, msg, fn) {
           expected[key].test(actual[key])) {
         continue;
       }
-      compareExceptionKey(actual, expected, key, msg, keys, fn);
+      compareExceptionKey(actual, expected, key, message, keys, fn);
     }
-    return true;
+    return;
   }
+
   // Guard instanceof against arrow functions as they don't have a prototype.
+  // Check for matching Error classes.
   if (expected.prototype !== undefined && actual instanceof expected) {
-    return true;
+    return;
   }
   if (Error.isPrototypeOf(expected)) {
-    return false;
+    throw actual;
+  }
+
+  // Check validation functions return value.
+  const res = expected.call({}, actual);
+  if (res !== true) {
+    throw actual;
   }
-  return expected.call({}, actual) === true;
 }
 
 function getActual(fn) {
@@ -695,9 +710,31 @@ function expectsError(stackStartFn, actual, error, message) {
       stackStartFn
     });
   }
-  if (error && !expectedException(actual, error, message, stackStartFn)) {
-    throw actual;
+
+  if (!error)
+    return;
+
+  expectedException(actual, error, message, stackStartFn);
+}
+
+function hasMatchingError(actual, expected) {
+  if (typeof expected !== 'function') {
+    if (isRegExp(expected)) {
+      const str = String(actual);
+      return expected.test(str);
+    }
+    throw new ERR_INVALID_ARG_TYPE(
+      'expected', ['Function', 'RegExp'], expected
+    );
   }
+  // Guard instanceof against arrow functions as they don't have a prototype.
+  if (expected.prototype !== undefined && actual instanceof expected) {
+    return true;
+  }
+  if (Error.isPrototypeOf(expected)) {
+    return false;
+  }
+  return expected.call({}, actual) === true;
 }
 
 function expectsNoError(stackStartFn, actual, error, message) {
@@ -709,7 +746,7 @@ function expectsNoError(stackStartFn, actual, error, message) {
     error = undefined;
   }
 
-  if (!error || expectedException(actual, error)) {
+  if (!error || hasMatchingError(actual, error)) {
     const details = message ? `: ${message}` : '.';
     const fnType = stackStartFn.name === 'doesNotReject' ?
       'rejection' : 'exception';
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index 025800eb8b9a9c..f0641430f76c03 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -182,7 +182,27 @@ assert.throws(
 }
 
 // Use a RegExp to validate the error message.
-a.throws(() => thrower(TypeError), /\[object Object\]/);
+{
+  a.throws(() => thrower(TypeError), /\[object Object\]/);
+
+  const symbol = Symbol('foo');
+  a.throws(() => {
+    throw symbol;
+  }, /foo/);
+
+  a.throws(() => {
+    a.throws(() => {
+      throw symbol;
+    }, /abc/);
+  }, {
+    message: 'The input did not match the regular expression /abc/. ' +
+             "Input:\n\n'Symbol(foo)'\n",
+    code: 'ERR_ASSERTION',
+    operator: 'throws',
+    actual: symbol,
+    expected: /abc/
+  });
+}
 
 // Use a fn to validate the error object.
 a.throws(() => thrower(TypeError), (err) => {