From e72a0287233230ee77e4123b66e9cf365da4b20f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 4 Apr 2018 17:03:58 +0200 Subject: [PATCH 1/2] 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. --- test/common/index.js | 70 +++++++++++++------ .../parallel/test-console-assign-undefined.js | 8 +-- test/parallel/test-internal-errors.js | 2 +- 3 files changed, 54 insertions(+), 26 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 7df3907f0e7174..53b97e5cb63423 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,10 +706,13 @@ 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)) { @@ -708,34 +720,52 @@ exports.expectsError = function expectsError(fn, settings, exact) { } 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); - } - if ('info' in settings) { - assert.deepStrictEqual(error.info, settings.info); + // Add the `type` to the error to properly compare and visualize it. + if (!('type' in error)) + error.type = constructor; } - 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') { + if (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, + errorDiff: 2 + }); + 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..64e81190952487 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -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 From 8a76c1a1fffaf343b175c78e8f55a619ae71140b Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 19 Apr 2018 19:34:28 +0200 Subject: [PATCH 2/2] fixup: remove now obsolete code --- test/common/index.js | 21 +++++++++------------ test/parallel/test-internal-errors.js | 2 +- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 53b97e5cb63423..95bb8dd804881f 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -718,8 +718,6 @@ exports.expectsError = function expectsError(fn, settings, exact) { 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 constructor = error.constructor; if (constructor.name === 'NodeError' && type.name !== 'NodeError') { constructor = Object.getPrototypeOf(error.constructor); @@ -729,14 +727,14 @@ exports.expectsError = function expectsError(fn, settings, exact) { error.type = constructor; } - if ('message' in settings && typeof settings.message === 'object') { - if (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; - } + 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. @@ -753,8 +751,7 @@ exports.expectsError = function expectsError(fn, settings, exact) { actual: a, expected: b, operator: 'strictEqual', - stackStartFn: assert.throws, - errorDiff: 2 + stackStartFn: assert.throws }); Error.stackTraceLimit = tmpLimit; diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 64e81190952487..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(() => {