assert: provide extra context in case of failed error validation checks

doc/api/assert.md

@@ -1171,10 +1171,15 @@ assert.throws(
 assert.throws(
   () => {
     const otherErr = new Error('Not found');
-    otherErr.code = 404;
+    // Copy all enumerable properties from `err` to `otherErr`.
+    for (const [key, value] of Object.entries(err)) {
+      otherErr[key] = value;
+    }
     throw otherErr;
   },
-  err // This tests for `message`, `name` and `code`.
+  // The errors `message` and `name` properties will also be checked when using
+  // an error as validation object.
+  err
 );
 ```
 
@@ -1205,6 +1210,9 @@ assert.throws(
 
 Custom error validation:
 
+The function must return `true` to indicate all internal validations passed.
+It will otherwise fail with an AssertionError.
+
 ```js
 assert.throws(
   () => {
@@ -1214,9 +1222,10 @@ assert.throws(
     assert(err instanceof Error);
     assert(/value/.test(err));
     // Returning anything from validation functions besides `true` is not
-    // recommended. Doing so results in the caught error being thrown again.
-    // That is usually not the desired outcome. Throw an error about the
-    // specific validation that failed instead (as done in this example).
+    // recommended. By doing that it's not clear what part of the validation
+    // failed. Instead, throw an error about the specific validation that failed
+    // (as done in this example) and add as much helpful debugging information
+    // to that error as possible.
     return true;
   },
   'unexpected error'
@@ -1258,11 +1267,9 @@ assert.throws(notThrowing, 'Second');
 // It does not throw because the error messages match.
 assert.throws(throwingSecond, /Second$/);
 
-// If the error message does not match, the error from within the function is
-// not caught.
+// If the error message does not match, an AssertionError is thrown.
 assert.throws(throwingFirst, /Second$/);
-// Error: First
-//     at throwingFirst (repl:2:9)
+// AssertionError [ERR_ASSERTION]
 ```
 
 Due to the confusing notation, it is recommended not to use a string as the

lib/assert.js

@@ -20,7 +20,7 @@
 
 'use strict';
 
-const { Object } = primordials;
+const { Object, ObjectPrototype } = primordials;
 
 const { Buffer } = require('buffer');
 const { codes: {
@@ -36,6 +36,7 @@ const { inspect } = require('internal/util/inspect');
 const { isPromise, isRegExp } = require('internal/util/types');
 const { EOL } = require('internal/constants');
 const { NativeModule } = require('internal/bootstrap/loaders');
+const { isError } = require('internal/util');
 
 const errorCache = new Map();
 
@@ -563,25 +564,24 @@ function compareExceptionKey(actual, expected, key, message, keys, fn) {
 }
 
 function expectedException(actual, expected, message, fn) {
+  let generatedMessage = false;
+  let throwError = false;
+
   if (typeof expected !== 'function') {
     // 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.
-    if (typeof actual !== 'object' || actual === null) {
+      if (!message) {
+        generatedMessage = true;
+        message = 'The input did not match the regular expression ' +
+                  `${inspect(expected)}. Input:\n\n${inspect(str)}\n`;
+      }
+      throwError = true;
+      // Handle primitives properly.
+    } else if (typeof actual !== 'object' || actual === null) {
       const err = new AssertionError({
         actual,
         expected,
@@ -591,43 +591,81 @@ function expectedException(actual, expected, message, fn) {
       });
       err.operator = fn.name;
       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.
-    if (expected instanceof Error) {
-      keys.push('name', 'message');
-    } else if (keys.length === 0) {
-      throw new ERR_INVALID_ARG_VALUE('error',
-                                      expected, 'may not be an empty object');
-    }
-    if (isDeepEqual === undefined) lazyLoadComparison();
-    for (const key of keys) {
-      if (typeof actual[key] === 'string' &&
-          isRegExp(expected[key]) &&
-          expected[key].test(actual[key])) {
-        continue;
+    } else {
+      // Handle validation objects.
+      const keys = Object.keys(expected);
+      // Special handle errors to make sure the name and the message are
+      // compared as well.
+      if (expected instanceof Error) {
+        keys.push('name', 'message');
+      } else if (keys.length === 0) {
+        throw new ERR_INVALID_ARG_VALUE('error',
+                                        expected, 'may not be an empty object');
+      }
+      if (isDeepEqual === undefined) lazyLoadComparison();
+      for (const key of keys) {
+        if (typeof actual[key] === 'string' &&
+            isRegExp(expected[key]) &&
+            expected[key].test(actual[key])) {
+          continue;
+        }
+        compareExceptionKey(actual, expected, key, message, keys, fn);
       }
-      compareExceptionKey(actual, expected, key, message, keys, fn);
+      return;
     }
-    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) {
+  } else if (expected.prototype !== undefined && actual instanceof expected) {
     return;
-  }
-  if (Error.isPrototypeOf(expected)) {
-    throw actual;
+  } else if (ObjectPrototype.isPrototypeOf(Error, expected)) {
+    if (!message) {
+      generatedMessage = true;
+      message = 'The error is expected to be an instance of ' +
+        `"${expected.name}". Received `;
+      if (isError(actual)) {
+        const name = actual.constructor && actual.constructor.name ||
+                     actual.name;
+        if (expected.name === name) {
+          message += 'an error with identical name but a different prototype.';
+        } else {
+          message += `"${name}"`;
+        }
+        if (actual.message) {
+          message += `\n\nError message:\n\n${actual.message}`;
+        }
+      } else {
+        message += `"${inspect(actual, { depth: -1 })}"`;
+      }
+    }
+    throwError = true;
+  } else {
+    // Check validation functions return value.
+    const res = expected.call({}, actual);
+    if (res !== true) {
+      if (!message) {
+        generatedMessage = true;
+        const name = expected.name ? `"${expected.name}" ` : '';
+        message = `The ${name}validation function is expected to return` +
+          ` "true". Received ${inspect(res)}`;
+
+        if (isError(actual)) {
+          message += `\n\nCaught error:\n\n${actual}`;
+        }
+      }
+      throwError = true;
+    }
   }
 
-  // Check validation functions return value.
-  const res = expected.call({}, actual);
-  if (res !== true) {
-    throw actual;
+  if (throwError) {
+    const err = new AssertionError({
+      actual,
+      expected,
+      message,
+      operator: fn.name,
+      stackStartFn: fn
+    });
+    err.generatedMessage = generatedMessage;
+    throw err;
   }
 }
 

test/parallel/test-assert-async.js

@@ -66,6 +66,22 @@ const invalidThenableFunc = () => {
       code: 'ERR_INVALID_RETURN_VALUE'
     })
   );
+
+  const err = new Error('foobar');
+  const validate = () => { return 'baz'; };
+  promises.push(assert.rejects(
+    () => assert.rejects(Promise.reject(err), validate),
+    {
+      message: 'The "validate" validation function is expected to ' +
+               "return \"true\". Received 'baz'\n\nCaught error:\n\n" +
+               'Error: foobar',
+      code: 'ERR_ASSERTION',
+      actual: err,
+      expected: validate,
+      name: 'AssertionError',
+      operator: 'rejects',
+    }
+  ));
 }
 
 {

test/parallel/test-assert.js

@@ -25,6 +25,7 @@
 const common = require('../common');
 const assert = require('assert');
 const { inspect } = require('util');
+const vm = require('vm');
 const { internalBinding } = require('internal/test/binding');
 const a = assert;
 
@@ -110,16 +111,19 @@ assert.throws(() => thrower(a.AssertionError));
 assert.throws(() => thrower(TypeError));
 
 // When passing a type, only catch errors of the appropriate type.
-{
-  let threw = false;
-  try {
-    a.throws(() => thrower(TypeError), a.AssertionError);
-  } catch (e) {
-    threw = true;
-    assert.ok(e instanceof TypeError, 'type');
+assert.throws(
+  () => a.throws(() => thrower(TypeError), a.AssertionError),
+  {
+    generatedMessage: true,
+    actual: new TypeError({}),
+    expected: a.AssertionError,
+    code: 'ERR_ASSERTION',
+    name: 'AssertionError',
+    operator: 'throws',
+    message: 'The error is expected to be an instance of "AssertionError". ' +
+             'Received "TypeError"\n\nError message:\n\n[object Object]'
   }
-  assert.ok(threw, 'a.throws with an explicit error is eating extra errors');
-}
+);
 
 // doesNotThrow should pass through all errors.
 {
@@ -213,20 +217,27 @@ a.throws(() => thrower(TypeError), (err) => {
 
 // https://github.com/nodejs/node/issues/3188
 {
-  let threw = false;
-  let AnotherErrorType;
-  try {
-    const ES6Error = class extends Error {};
-    AnotherErrorType = class extends Error {};
-
-    assert.throws(() => { throw new AnotherErrorType('foo'); }, ES6Error);
-  } catch (e) {
-    threw = true;
-    assert(e instanceof AnotherErrorType,
-           `expected AnotherErrorType, received ${e}`);
-  }
+  let actual;
+  assert.throws(
+    () => {
+      const ES6Error = class extends Error {};
+      const AnotherErrorType = class extends Error {};
 
-  assert.ok(threw);
+      assert.throws(() => {
+        actual = new AnotherErrorType('foo');
+        throw actual;
+      }, ES6Error);
+    },
+    (err) => {
+      assert.strictEqual(
+        err.message,
+        'The error is expected to be an instance of "ES6Error". ' +
+          'Received "AnotherErrorType"\n\nError message:\n\nfoo'
+      );
+      assert.strictEqual(err.actual, actual);
+      return true;
+    }
+  );
 }
 
 // Check messages from assert.throws().
@@ -1254,3 +1265,52 @@ assert.throws(
     assert(!err2.stack.includes('hidden'));
   })();
 }
+
+assert.throws(
+  () => assert.throws(() => { throw Symbol('foo'); }, RangeError),
+  {
+    message: 'The error is expected to be an instance of "RangeError". ' +
+             'Received "Symbol(foo)"'
+  }
+);
+
+assert.throws(
+  // eslint-disable-next-line no-throw-literal
+  () => assert.throws(() => { throw [1, 2]; }, RangeError),
+  {
+    message: 'The error is expected to be an instance of "RangeError". ' +
+             'Received "[Array]"'
+  }
+);
+
+{
+  const err = new TypeError('foo');
+  const validate = (() => () => ({ a: true, b: [ 1, 2, 3 ] }))();
+  assert.throws(
+    () => assert.throws(() => { throw err; }, validate),
+    {
+      message: 'The validation function is expected to ' +
+              `return "true". Received ${inspect(validate())}\n\nCaught ` +
+              `error:\n\n${err}`,
+      code: 'ERR_ASSERTION',
+      actual: err,
+      expected: validate,
+      name: 'AssertionError',
+      operator: 'throws',
+    }
+  );
+}
+
+assert.throws(
+  () => {
+    const script = new vm.Script('new RangeError("foobar");');
+    const context = vm.createContext();
+    const err = script.runInContext(context);
+    assert.throws(() => { throw err; }, RangeError);
+  },
+  {
+    message: 'The error is expected to be an instance of "RangeError". ' +
+             'Received an error with identical name but a different ' +
+             'prototype.\n\nError message:\n\nfoobar'
+  }
+);