From 4a3af65a88122307a86b28bf385b8f87767803e6 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 6e0b850b40c..fadc3ad530f 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 52e85724797..af2b683bfa8 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) => {