test: refactor common.expectsError

This completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.

PR-URL: #31092
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

test/.eslintrc.yaml

@@ -14,7 +14,6 @@ rules:
   # Custom rules in tools/eslint-rules
   node-core/prefer-assert-iferror: error
   node-core/prefer-assert-methods: error
-  node-core/prefer-common-expectserror: error
   node-core/prefer-common-mustnotcall: error
   node-core/crypto-check: error
   node-core/eslint-check: error

test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js

@@ -5,6 +5,7 @@ const skipMessage = 'intensive toString tests due to memory confinements';
 if (!common.enoughTestMem)
   common.skip(skipMessage);
 
+const assert = require('assert');
 const binding = require(`./build/${common.buildType}/binding`);
 
 // v8 fails silently if string length > v8::String::kMaxLength
@@ -25,11 +26,11 @@ if (!binding.ensureAllocation(2 * kStringMaxLength))
   common.skip(skipMessage);
 
 const stringLengthHex = kStringMaxLength.toString(16);
-common.expectsError(() => {
+assert.throws(() => {
   buf.toString('ascii');
 }, {
   message: `Cannot create a string longer than 0x${stringLengthHex} ` +
            'characters',
   code: 'ERR_STRING_TOO_LONG',
-  type: Error
+  name: 'Error'
 });

test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64.js

@@ -5,6 +5,7 @@ const skipMessage = 'intensive toString tests due to memory confinements';
 if (!common.enoughTestMem)
   common.skip(skipMessage);
 
+const assert = require('assert');
 const binding = require(`./build/${common.buildType}/binding`);
 
 // v8 fails silently if string length > v8::String::kMaxLength
@@ -25,11 +26,11 @@ if (!binding.ensureAllocation(2 * kStringMaxLength))
   common.skip(skipMessage);
 
 const stringLengthHex = kStringMaxLength.toString(16);
-common.expectsError(() => {
+assert.throws(() => {
   buf.toString('base64');
 }, {
   message: `Cannot create a string longer than 0x${stringLengthHex} ` +
            'characters',
   code: 'ERR_STRING_TOO_LONG',
-  type: Error
+  name: 'Error'
 });

test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary.js

@@ -27,13 +27,13 @@ if (!binding.ensureAllocation(2 * kStringMaxLength))
   common.skip(skipMessage);
 
 const stringLengthHex = kStringMaxLength.toString(16);
-common.expectsError(() => {
+assert.throws(() => {
   buf.toString('latin1');
 }, {
   message: `Cannot create a string longer than 0x${stringLengthHex} ` +
            'characters',
   code: 'ERR_STRING_TOO_LONG',
-  type: Error
+  name: 'Error'
 });
 
 // FIXME: Free the memory early to avoid OOM.

test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex.js

@@ -5,6 +5,7 @@ const skipMessage = 'intensive toString tests due to memory confinements';
 if (!common.enoughTestMem)
   common.skip(skipMessage);
 
+const assert = require('assert');
 const binding = require(`./build/${common.buildType}/binding`);
 
 // v8 fails silently if string length > v8::String::kMaxLength
@@ -25,11 +26,11 @@ if (!binding.ensureAllocation(2 * kStringMaxLength))
   common.skip(skipMessage);
 
 const stringLengthHex = kStringMaxLength.toString(16);
-common.expectsError(() => {
+assert.throws(() => {
   buf.toString('hex');
 }, {
   message: `Cannot create a string longer than 0x${stringLengthHex} ` +
            'characters',
   code: 'ERR_STRING_TOO_LONG',
-  type: Error
+  name: 'Error'
 });

test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8.js

@@ -35,19 +35,19 @@ assert.throws(() => {
       message: `Cannot create a string longer than 0x${stringLengthHex} ` +
                'characters',
       code: 'ERR_STRING_TOO_LONG',
-      type: Error
+      name: 'Error'
     })(e);
     return true;
   } else {
     return true;
   }
 });
 
-common.expectsError(() => {
+assert.throws(() => {
   buf.toString('utf8');
 }, {
   message: `Cannot create a string longer than 0x${stringLengthHex} ` +
            'characters',
   code: 'ERR_STRING_TOO_LONG',
-  type: Error
+  name: 'Error'
 });

test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js

@@ -5,6 +5,7 @@ const skipMessage = 'intensive toString tests due to memory confinements';
 if (!common.enoughTestMem)
   common.skip(skipMessage);
 
+const assert = require('assert');
 const binding = require(`./build/${common.buildType}/binding`);
 
 // v8 fails silently if string length > v8::String::kMaxLength
@@ -26,11 +27,11 @@ if (!binding.ensureAllocation(2 * kStringMaxLength))
 
 const stringLengthHex = kStringMaxLength.toString(16);
 
-common.expectsError(() => {
+assert.throws(() => {
   buf.toString('utf16le');
 }, {
   message: `Cannot create a string longer than 0x${stringLengthHex} ` +
            'characters',
   code: 'ERR_STRING_TOO_LONG',
-  type: Error
+  name: 'Error'
 });

test/async-hooks/test-embedder.api.async-resource-no-type.js

@@ -18,9 +18,9 @@ if (process.argv[2] === 'child') {
   }
 
   [null, undefined, 1, Date, {}, []].forEach((i) => {
-    common.expectsError(() => new Foo(i), {
+    assert.throws(() => new Foo(i), {
       code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError
+      name: 'TypeError'
     });
   });
 

test/async-hooks/test-embedder.api.async-resource.js

@@ -12,16 +12,16 @@ const { checkInvocations } = require('./hook-checks');
 const hooks = initHooks();
 hooks.enable();
 
-common.expectsError(
+assert.throws(
   () => new AsyncResource(), {
     code: 'ERR_INVALID_ARG_TYPE',
-    type: TypeError,
+    name: 'TypeError',
   });
-common.expectsError(() => {
+assert.throws(() => {
   new AsyncResource('invalid_trigger_id', { triggerAsyncId: null });
 }, {
   code: 'ERR_INVALID_ASYNC_ID',
-  type: RangeError,
+  name: 'RangeError',
 });
 
 assert.strictEqual(

test/common/README.md

@@ -71,44 +71,17 @@ the `unhandledRejection` hook is directly used by the test.
 
 Indicates if there is more than 1gb of total memory.
 
-### expectsError(\[fn, \]settings\[, exact\])
-* `fn` [<Function>][] a function that should throw.
-* `settings` [<Object>][]
-  that must contain the `code` property plus any of the other following
-  properties (some properties only apply for `AssertionError`):
-  * `code` [<string>][]
-    expected error must have this value for its `code` property.
-  * `type` [<Function>][]
-    expected error must be an instance of `type` and must be an Error subclass.
-  * `message` [<string>][] or [<RegExp>][]
-    if a string is provided for `message`, expected error must have it for its
-    `message` property; if a regular expression is provided for `message`, the
-    regular expression must match the `message` property of the expected error.
-  * `name` [<string>][]
-    expected error must have this value for its `name` property.
-  * `info` <Object> expected error must have the same `info` property
-    that is deeply equal to this value.
-  * `generatedMessage` [<string>][]
-    (`AssertionError` only) expected error must have this value for its
-    `generatedMessage` property.
-  * `actual` <any>
-    (`AssertionError` only) expected error must have this value for its
-    `actual` property.
-  * `expected` <any>
-    (`AssertionError` only) expected error must have this value for its
-    `expected` property.
-  * `operator` <any>
-    (`AssertionError` only) expected error must have this value for its
-    `operator` property.
+### expectsError(validator\[, exact\])
+* `validator` [<Object>][] | [<RegExp>][] | [<Function>][] |
+  [<Error>][] The validator behaves identical to
+  `assert.throws(fn, validator)`.
 * `exact` [<number>][] default = 1
-* return [<Function>][]
+* return [<Function>][] A callback function that expects an error.
 
-  If `fn` is provided, it will be passed to `assert.throws` as first argument
-  and `undefined` will be returned.
-  Otherwise a function suitable as callback or for use as a validation function
-  passed as the second argument to `assert.throws()` will be returned. If the
-  returned function has not been called exactly `exact` number of times when the
-  test is complete, then the test will fail.
+A function suitable as callback to validate callback based errors. The error is
+validated using `assert.throws(() => { throw error; }, validator)`. If the
+returned function has not been called exactly `exact` number of times when the
+test is complete, then the test will fail.
 
 ### expectWarning(name\[, expected\[, code\]\])
 * `name` [<string>][] | [<Object>][]
@@ -929,6 +902,7 @@ See [the WPT tests README][] for details.
 [<ArrayBufferView>]: https://developer.mozilla.org/en-US/docs/Web/API/ArrayBufferView
 [<Buffer>]: https://nodejs.org/api/buffer.html#buffer_class_buffer
 [<BufferSource>]: https://developer.mozilla.org/en-US/docs/Web/API/BufferSource
+[<Error>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error
 [<Function>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function
 [<Object>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object
 [<RegExp>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp

test/common/index.js

@@ -525,92 +525,22 @@ function expectWarning(nameOrMap, expected, code) {
   }
 }
 
-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
-function expectsError(fn, settings, exact) {
-  if (typeof fn !== 'function') {
-    exact = settings;
-    settings = fn;
-    fn = undefined;
-  }
-
-  function innerFn(error) {
-    if (arguments.length !== 1) {
+function expectsError(validator, exact) {
+  return mustCall((...args) => {
+    if (args.length !== 1) {
       // Do not use `assert.strictEqual()` to prevent `util.inspect` from
       // always being called.
-      assert.fail(`Expected one argument, got ${util.inspect(arguments)}`);
+      assert.fail(`Expected one argument, got ${util.inspect(args)}`);
     }
+    const error = args.pop();
     const descriptor = Object.getOwnPropertyDescriptor(error, 'message');
     // The error message should be non-enumerable
     assert.strictEqual(descriptor.enumerable, false);
 
-    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`');
-      }
-      let constructor = error.constructor;
-      if (constructor.name === 'NodeError' && type.name !== 'NodeError') {
-        constructor = Object.getPrototypeOf(error.constructor);
-      }
-      // Add the `type` to the error to properly compare and visualize it.
-      if (!('type' in error))
-        error.type = constructor;
-    }
-
-    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 (!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
-        });
-      }
-
-    }
+    assert.throws(() => { throw error; }, validator);
     return true;
-  }
-  if (fn) {
-    assert.throws(fn, innerFn);
-    return;
-  }
-  return mustCall(innerFn, exact);
+  }, exact);
 }
 
 const suffix = 'This is caused by either a bug in Node.js ' +

test/es-module/test-esm-loader-modulemap.js

@@ -4,8 +4,9 @@
 // This test ensures that the type checking of ModuleMap throws
 // errors appropriately
 
-const common = require('../common');
+require('../common');
 
+const assert = require('assert');
 const { URL } = require('url');
 const { Loader } = require('internal/modules/esm/loader');
 const ModuleMap = require('internal/modules/esm/module_map');
@@ -20,41 +21,41 @@ const moduleMap = new ModuleMap();
 const moduleJob = new ModuleJob(loader, stubModule.module,
                                 () => new Promise(() => {}));
 
-common.expectsError(
+assert.throws(
   () => moduleMap.get(1),
   {
     code: 'ERR_INVALID_ARG_TYPE',
-    type: TypeError,
+    name: 'TypeError',
     message: 'The "url" argument must be of type string. Received type number' +
              ' (1)'
   }
 );
 
-common.expectsError(
+assert.throws(
   () => moduleMap.set(1, moduleJob),
   {
     code: 'ERR_INVALID_ARG_TYPE',
-    type: TypeError,
+    name: 'TypeError',
     message: 'The "url" argument must be of type string. Received type number' +
              ' (1)'
   }
 );
 
-common.expectsError(
+assert.throws(
   () => moduleMap.set('somestring', 'notamodulejob'),
   {
     code: 'ERR_INVALID_ARG_TYPE',
-    type: TypeError,
+    name: 'TypeError',
     message: 'The "job" argument must be an instance of ModuleJob. ' +
              "Received type string ('notamodulejob')"
   }
 );
 
-common.expectsError(
+assert.throws(
   () => moduleMap.has(1),
   {
     code: 'ERR_INVALID_ARG_TYPE',
-    type: TypeError,
+    name: 'TypeError',
     message: 'The "url" argument must be of type string. Received type number' +
              ' (1)'
   }