From f85d5996db11107edb4d078f5eafcd4cc588cd9c Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 4 Apr 2018 17:03:58 +0200
Subject: [PATCH] test: improve common.expectsError

The output is now improved by showing most properties all at once.
Besides that this adds a warning to use `assert.throws` instead
due to a better output.

PR-URL: https://github.com/nodejs/node/pull/19797
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
---
 test/common/index.js                          | 73 +++++++++++++------
 .../parallel/test-console-assign-undefined.js |  8 +-
 test/parallel/test-internal-errors.js         |  4 +-
 3 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/test/common/index.js b/test/common/index.js
index 7df3907f0e7174..95bb8dd804881f 100644
--- a/test/common/index.js
+++ b/test/common/index.js
@@ -690,6 +690,15 @@ Object.defineProperty(exports, 'hasSmallICU', {
   }
 });
 
+class Comparison {
+  constructor(obj, keys) {
+    for (const key of keys) {
+      if (key in obj)
+        this[key] = obj[key];
+    }
+  }
+}
+
 // Useful for testing expected internal/error objects
 exports.expectsError = function expectsError(fn, settings, exact) {
   if (typeof fn !== 'function') {
@@ -697,45 +706,63 @@ exports.expectsError = function expectsError(fn, settings, exact) {
     settings = fn;
     fn = undefined;
   }
+
   function innerFn(error) {
     const descriptor = Object.getOwnPropertyDescriptor(error, 'message');
     assert.strictEqual(descriptor.enumerable,
                        false, 'The error message should be non-enumerable');
+
+    let innerSettings = settings;
     if ('type' in settings) {
       const type = settings.type;
       if (type !== Error && !Error.isPrototypeOf(type)) {
         throw new TypeError('`settings.type` must inherit from `Error`');
       }
-      assert(error instanceof type,
-             `${error.name} is not instance of ${type.name}`);
-      let typeName = error.constructor.name;
-      if (typeName === 'NodeError' && type.name !== 'NodeError') {
-        typeName = Object.getPrototypeOf(error.constructor).name;
+      let constructor = error.constructor;
+      if (constructor.name === 'NodeError' && type.name !== 'NodeError') {
+        constructor = Object.getPrototypeOf(error.constructor);
       }
-      assert.strictEqual(typeName, type.name);
+      // Add the `type` to the error to properly compare and visualize it.
+      if (!('type' in error))
+        error.type = constructor;
     }
-    if ('info' in settings) {
-      assert.deepStrictEqual(error.info, settings.info);
-    }
-    if ('message' in settings) {
-      const message = settings.message;
-      if (typeof message === 'string') {
-        assert.strictEqual(error.message, message);
-      } else {
-        assert(message.test(error.message),
-               `${error.message} does not match ${message}`);
-      }
+
+    if ('message' in settings &&
+        typeof settings.message === 'object' &&
+        settings.message.test(error.message)) {
+      // Make a copy so we are able to modify the settings.
+      innerSettings = Object.create(
+        settings, Object.getOwnPropertyDescriptors(settings));
+      // Visualize the message as identical in case of other errors.
+      innerSettings.message = error.message;
     }
 
     // Check all error properties.
     const keys = Object.keys(settings);
     for (const key of keys) {
-      if (key === 'message' || key === 'type' || key === 'info')
-        continue;
-      const actual = error[key];
-      const expected = settings[key];
-      assert.strictEqual(actual, expected,
-                         `${key}: expected ${expected}, not ${actual}`);
+      if (!util.isDeepStrictEqual(error[key], innerSettings[key])) {
+        // Create placeholder objects to create a nice output.
+        const a = new Comparison(error, keys);
+        const b = new Comparison(innerSettings, keys);
+
+        const tmpLimit = Error.stackTraceLimit;
+        Error.stackTraceLimit = 0;
+        const err = new assert.AssertionError({
+          actual: a,
+          expected: b,
+          operator: 'strictEqual',
+          stackStartFn: assert.throws
+        });
+        Error.stackTraceLimit = tmpLimit;
+
+        throw new assert.AssertionError({
+          actual: error,
+          expected: settings,
+          operator: 'common.expectsError',
+          message: err.message
+        });
+      }
+
     }
     return true;
   }
diff --git a/test/parallel/test-console-assign-undefined.js b/test/parallel/test-console-assign-undefined.js
index 1c47b3bda784bb..1021307b3c5f22 100644
--- a/test/parallel/test-console-assign-undefined.js
+++ b/test/parallel/test-console-assign-undefined.js
@@ -6,7 +6,7 @@
 const tmp = global.console;
 global.console = 42;
 
-const common = require('../common');
+require('../common');
 const assert = require('assert');
 
 // Originally the console had a getter. Test twice to verify it had no side
@@ -14,11 +14,9 @@ const assert = require('assert');
 assert.strictEqual(global.console, 42);
 assert.strictEqual(global.console, 42);
 
-common.expectsError(
+assert.throws(
   () => console.log('foo'),
-  {
-    type: TypeError
-  }
+  { name: 'TypeError' }
 );
 
 global.console = 1;
diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js
index a161a1db69e66d..cd2028c0f29b95 100644
--- a/test/parallel/test-internal-errors.js
+++ b/test/parallel/test-internal-errors.js
@@ -77,7 +77,7 @@ common.expectsError(() => {
   }, { code: 'TEST_ERROR_1', type: RangeError });
 }, {
   code: 'ERR_ASSERTION',
-  message: /^.+ is not instance of \S/
+  message: /-   type: \[Function: TypeError]\n\+   type: \[Function: RangeError]/
 });
 
 common.expectsError(() => {
@@ -89,7 +89,7 @@ common.expectsError(() => {
 }, {
   code: 'ERR_ASSERTION',
   type: assert.AssertionError,
-  message: /.+ does not match \S/
+  message: /-   message: 'Error for testing purposes: a'\n\+   message: \/\^Error/
 });
 
 // Test ERR_INVALID_FD_TYPE