Skip to content

Commit d0483ee

Browse files
TrottItalo A. Casas
Trott
authored and
Italo A. Casas
committed
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. PR-URL: #11512 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
1 parent 3afe90d commit d0483ee

File tree

6 files changed

+28
-24
lines changed

6 files changed

+28
-24
lines changed

‎test/README.md‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -187,16 +187,18 @@ Platform normalizes the `dd` command
187187

188188
Check if there is more than 1gb of total memory.
189189

190-
### expectsError(code[, type[, message]])
191-
* `code` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
192-
expected error must have this value for its `code` property
193-
* `type` [&lt;Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function)
194-
expected error must be an instance of `type`
195-
* `message` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
196-
or [&lt;RegExp>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp)
197-
if a string is provided for `message`, expected error must have it for its
198-
`message` property; if a regular expression is provided for `message`, the
199-
regular expression must match the `message` property of the expected error
190+
### expectsError(settings)
191+
* `settings` [&lt;Object>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)
192+
with the following optional properties:
193+
* `code` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
194+
expected error must have this value for its `code` property
195+
* `type` [&lt;Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function)
196+
expected error must be an instance of `type`
197+
* `message` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
198+
or [&lt;RegExp>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp)
199+
if a string is provided for `message`, expected error must have it for its
200+
`message` property; if a regular expression is provided for `message`, the
201+
regular expression must match the `message` property of the expected error
200202

201203
* return function suitable for use as a validation function passed as the second
202204
argument to `assert.throws()`

‎test/common.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -621,7 +621,7 @@ exports.WPT = {
621621
};
622622

623623
// Useful for testing expected internal/error objects
624-
exports.expectsError = function expectsError(code, type, message) {
624+
exports.expectsError = function expectsError({code, type, message}) {
625625
return function(error) {
626626
assert.strictEqual(error.code, code);
627627
if (type !== undefined)

‎test/parallel/test-debug-agent.js‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,6 @@ const debug = require('_debug_agent');
55

66
assert.throws(
77
() => { debug.start(); },
8-
common.expectsError(
9-
undefined,
10-
assert.AssertionError,
11-
'Debugger agent running without bindings!'
12-
)
8+
common.expectsError({ type: assert.AssertionError,
9+
message: 'Debugger agent running without bindings!' })
1310
);

‎test/parallel/test-http-request-invalid-method-error.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,6 @@ const http = require('http');
55

66
assert.throws(
77
() => { http.request({method: '\0'}); },
8-
common.expectsError(undefined, TypeError, 'Method must be a valid HTTP token')
8+
common.expectsError({ type: TypeError,
9+
message: 'Method must be a valid HTTP token' })
910
);

‎test/parallel/test-internal-errors.js‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -82,35 +82,39 @@ assert.throws(
8282
assert.doesNotThrow(() => {
8383
assert.throws(() => {
8484
throw new errors.TypeError('TEST_ERROR_1', 'a');
85-
}, common.expectsError('TEST_ERROR_1'));
85+
}, common.expectsError({ code: 'TEST_ERROR_1' }));
8686
});
8787

8888
assert.doesNotThrow(() => {
8989
assert.throws(() => {
9090
throw new errors.TypeError('TEST_ERROR_1', 'a');
91-
}, common.expectsError('TEST_ERROR_1', TypeError, /^Error for testing/));
91+
}, common.expectsError({ code: 'TEST_ERROR_1',
92+
type: TypeError,
93+
message: /^Error for testing/ }));
9294
});
9395

9496
assert.doesNotThrow(() => {
9597
assert.throws(() => {
9698
throw new errors.TypeError('TEST_ERROR_1', 'a');
97-
}, common.expectsError('TEST_ERROR_1', TypeError));
99+
}, common.expectsError({ code: 'TEST_ERROR_1', type: TypeError }));
98100
});
99101

100102
assert.doesNotThrow(() => {
101103
assert.throws(() => {
102104
throw new errors.TypeError('TEST_ERROR_1', 'a');
103-
}, common.expectsError('TEST_ERROR_1', Error));
105+
}, common.expectsError({ code: 'TEST_ERROR_1', type: Error }));
104106
});
105107

106108
assert.throws(() => {
107109
assert.throws(() => {
108110
throw new errors.TypeError('TEST_ERROR_1', 'a');
109-
}, common.expectsError('TEST_ERROR_1', RangeError));
111+
}, common.expectsError({ code: 'TEST_ERROR_1', type: RangeError }));
110112
}, /^AssertionError: .+ is not the expected type \S/);
111113

112114
assert.throws(() => {
113115
assert.throws(() => {
114116
throw new errors.TypeError('TEST_ERROR_1', 'a');
115-
}, common.expectsError('TEST_ERROR_1', TypeError, /^Error for testing 2/));
117+
}, common.expectsError({ code: 'TEST_ERROR_1',
118+
type: TypeError,
119+
message: /^Error for testing 2/ }));
116120
}, /AssertionError: .+ does not match \S/);

‎test/parallel/test-require-invalid-package.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,5 @@ const assert = require('assert');
55

66
// Should be an invalid package path.
77
assert.throws(() => require('package.json'),
8-
common.expectsError('MODULE_NOT_FOUND')
8+
common.expectsError({ code: 'MODULE_NOT_FOUND' })
99
);

0 commit comments

Comments
 (0)