test: change common.expectsError() signature

One downside to `common.expectsError()` is that it increases the abstractions people have to learn about in order to work with even simple tests. Whereas before, all they had to know about is `assert.throws()`, now they have to *also* know about `common.expectsError()`. This is very different (IMO) from `common.mustCall()` in that the latter has an intuitively understandable name, accepts arguments as one would expect, and (in most cases) doesn't actually require reading documentation or code to figure out what it's doing. With `common.expectsError()`, there's a fair bit of magic. Like, it's not obvious what the first argument would be. Or the second. Or the third. You just have to know. This PR changes the arguments accepted by `common.expectsError()` to a single settings object. Someone coming across this has a hope of understanding what's going on without reading source or docs: ```js const validatorFunction = common.expectsError({code: 'ELOOP', type: Error, message: 'foo'}); ``` This, by comparison, is harder to grok: ```js const validatorFunction = common.expectsError('ELOOP', Error, 'foo'); ``` And this is especially wat-inducing: ```js common.expectsError(undefined, undefined, 'looped doodad found'); ``` It's likely that only people who work with tests frequently can be expected to remember the three arguments and their order. By comparison, remembering that the error code is `code` and the message is `message` might be manageable.
nodejs · Trott · Feb 22, 2017 · Feb 25, 2017 · cd0b3217dafb0faf5061d6f6e4fdee0dd36fa4f8
commit cd0b3217dafb0faf5061d6f6e4fdee0dd36fa4f8
diff --git a/test/README.md b/test/README.md
@@ -187,16 +187,18 @@ Platform normalizes the `dd` command
 
 Check if there is more than 1gb of total memory.
 
-### expectsError(code[, type[, message]])
-* `code` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
-  expected error must have this value for its `code` property
-* `type` [&lt;Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function)
-  expected error must be an instance of `type`
-* `message` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
-  or [&lt;RegExp>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/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
+### expectsError(settings)
+* `settings` [&lt;Object>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)
+  with the following optional properties:
+  * `code` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
+    expected error must have this value for its `code` property
+  * `type` [&lt;Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function)
+    expected error must be an instance of `type`
+  * `message` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
+    or [&lt;RegExp>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/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
 
 * return function suitable for use as a validation function passed as the second
   argument to `assert.throws()`

diff --git a/test/common.js b/test/common.js
@@ -625,7 +625,7 @@ exports.WPT = {
 };
 
 // Useful for testing expected internal/error objects
-exports.expectsError = function expectsError(code, type, message) {
+exports.expectsError = function expectsError({code, type, message}) {
   return function(error) {
     assert.strictEqual(error.code, code);
     if (type !== undefined)

diff --git a/test/parallel/test-debug-agent.js b/test/parallel/test-debug-agent.js
@@ -5,9 +5,6 @@ const debug = require('_debug_agent');
 
 assert.throws(
   () => { debug.start(); },
-  common.expectsError(
-    undefined,
-    assert.AssertionError,
-    'Debugger agent running without bindings!'
-  )
+  common.expectsError({ type: assert.AssertionError,
+                        message: 'Debugger agent running without bindings!' })
 );
diff --git a/test/parallel/test-http-request-invalid-method-error.js b/test/parallel/test-http-request-invalid-method-error.js
@@ -5,5 +5,6 @@ const http = require('http');
 
 assert.throws(
   () => { http.request({method: '\0'}); },
-  common.expectsError(undefined, TypeError, 'Method must be a valid HTTP token')
+  common.expectsError({ type: TypeError,
+                        message: 'Method must be a valid HTTP token' })
 );
diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js
@@ -82,35 +82,39 @@ assert.throws(
 assert.doesNotThrow(() => {
   assert.throws(() => {
     throw new errors.TypeError('TEST_ERROR_1', 'a');
-  }, common.expectsError('TEST_ERROR_1'));
+  }, common.expectsError({ code: 'TEST_ERROR_1' }));
 });
 
 assert.doesNotThrow(() => {
   assert.throws(() => {
     throw new errors.TypeError('TEST_ERROR_1', 'a');
-  }, common.expectsError('TEST_ERROR_1', TypeError, /^Error for testing/));
+  }, common.expectsError({ code: 'TEST_ERROR_1',
+                           type: TypeError,
+                           message: /^Error for testing/ }));
 });
 
 assert.doesNotThrow(() => {
   assert.throws(() => {
     throw new errors.TypeError('TEST_ERROR_1', 'a');
-  }, common.expectsError('TEST_ERROR_1', TypeError));
+  }, common.expectsError({ code: 'TEST_ERROR_1', type: TypeError }));
 });
 
 assert.doesNotThrow(() => {
   assert.throws(() => {
     throw new errors.TypeError('TEST_ERROR_1', 'a');
-  }, common.expectsError('TEST_ERROR_1', Error));
+  }, common.expectsError({ code: 'TEST_ERROR_1', type: Error }));
 });
 
 assert.throws(() => {
   assert.throws(() => {
     throw new errors.TypeError('TEST_ERROR_1', 'a');
-  }, common.expectsError('TEST_ERROR_1', RangeError));
+  }, common.expectsError({ code: 'TEST_ERROR_1', type: RangeError }));
 }, /^AssertionError: .+ is not the expected type \S/);
 
 assert.throws(() => {
   assert.throws(() => {
     throw new errors.TypeError('TEST_ERROR_1', 'a');
-  }, common.expectsError('TEST_ERROR_1', TypeError, /^Error for testing 2/));
+  }, common.expectsError({ code: 'TEST_ERROR_1',
+                           type: TypeError,
+                           message: /^Error for testing 2/ }));
 }, /AssertionError: .+ does not match \S/);
diff --git a/test/parallel/test-require-invalid-package.js b/test/parallel/test-require-invalid-package.js
@@ -5,5 +5,5 @@ const assert = require('assert');
 
 // Should be an invalid package path.
 assert.throws(() => require('package.json'),
-              common.expectsError('MODULE_NOT_FOUND')
+              common.expectsError({ code: 'MODULE_NOT_FOUND' })
 );