From 72bb4445c64af98f3e481c11320afbfb995f010c Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 19 Jan 2018 10:35:39 +0100 Subject: [PATCH] assert: wrap original error in ifError It is hard to know where ifError is actually triggered due to the original error being thrown. This changes it by wrapping the original error in a AssertionError. This has the positive effect of also making clear that it is indeed a assertion function that triggered that error. The original stack can still be accessed by checking the `actual` property. PR-URL: https://github.com/nodejs/node/pull/18247 Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- doc/api/assert.md | 11 ++- lib/assert.js | 48 ++++++++++- .../uncaught-exceptions/callbackify2.js | 6 +- test/message/if-error-has-good-stack.js | 22 +++++ test/message/if-error-has-good-stack.out | 19 +++++ test/parallel/test-assert-if-error.js | 82 +++++++++++++++++++ test/parallel/test-assert.js | 21 ----- test/parallel/test-domain-implicit-fs.js | 6 +- test/parallel/test-util-callbackify.js | 4 +- 9 files changed, 187 insertions(+), 32 deletions(-) create mode 100644 test/message/if-error-has-good-stack.js create mode 100644 test/message/if-error-has-good-stack.out create mode 100644 test/parallel/test-assert-if-error.js diff --git a/doc/api/assert.md b/doc/api/assert.md index 339351db8b22b1..80b3019ad8a1ee 100644 --- a/doc/api/assert.md +++ b/doc/api/assert.md @@ -469,6 +469,11 @@ suppressFrame(); ## assert.ifError(value) * `value` {any} @@ -483,11 +488,11 @@ assert.ifError(null); assert.ifError(0); // OK assert.ifError(1); -// Throws 1 +// AssertionError [ERR_ASSERTION]: ifError got unwanted exception: 1 assert.ifError('error'); -// Throws 'error' +// AssertionError [ERR_ASSERTION]: ifError got unwanted exception: 'error' assert.ifError(new Error()); -// Throws Error +// AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Error ``` ## assert.notDeepEqual(actual, expected[, message]) diff --git a/lib/assert.js b/lib/assert.js index 9931ce9c222e6e..6ac3cf38ea99ae 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -452,7 +452,53 @@ assert.doesNotThrow = function doesNotThrow(block, error, message) { throw actual; }; -assert.ifError = function ifError(err) { if (err) throw err; }; +assert.ifError = function ifError(err) { + if (err) { + let message = 'ifError got unwanted exception: '; + if (typeof err === 'object' && typeof err.message === 'string') { + if (err.message.length === 0 && err.constructor) { + message += err.constructor.name; + } else { + message += err.message; + } + } else { + message += inspect(err); + } + + const newErr = new assert.AssertionError({ + actual: err, + expected: null, + operator: 'ifError', + message, + stackStartFn: ifError + }); + + // Make sure we actually have a stack trace! + const origStack = err.stack; + + if (typeof origStack === 'string') { + // This will remove any duplicated frames from the error frames taken + // from within `ifError` and add the original error frames to the newly + // created ones. + const tmp2 = origStack.split('\n'); + tmp2.shift(); + // Filter all frames existing in err.stack. + let tmp1 = newErr.stack.split('\n'); + for (var i = 0; i < tmp2.length; i++) { + // Find the first occurrence of the frame. + const pos = tmp1.indexOf(tmp2[i]); + if (pos !== -1) { + // Only keep new frames. + tmp1 = tmp1.slice(0, pos); + break; + } + } + newErr.stack = `${tmp1.join('\n')}\n${tmp2.join('\n')}`; + } + + throw newErr; + } +}; // Expose a strict only variant of assert function strict(...args) { diff --git a/test/fixtures/uncaught-exceptions/callbackify2.js b/test/fixtures/uncaught-exceptions/callbackify2.js index 9080da234f3c04..7b4ee4f565596d 100644 --- a/test/fixtures/uncaught-exceptions/callbackify2.js +++ b/test/fixtures/uncaught-exceptions/callbackify2.js @@ -8,13 +8,13 @@ const { callbackify } = require('util'); { const sentinel = new Error(__filename); process.once('uncaughtException', (err) => { - assert.strictEqual(err, sentinel); + assert.notStrictEqual(err, sentinel); // Calling test will use `stdout` to assert value of `err.message` console.log(err.message); }); - async function fn() { - return await Promise.reject(sentinel); + function fn() { + return Promise.reject(sentinel); } const cbFn = callbackify(fn); diff --git a/test/message/if-error-has-good-stack.js b/test/message/if-error-has-good-stack.js new file mode 100644 index 00000000000000..1db25d2fa55a1b --- /dev/null +++ b/test/message/if-error-has-good-stack.js @@ -0,0 +1,22 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); + +let err; +// Create some random error frames. +(function a() { + (function b() { + (function c() { + err = new Error('test error'); + })(); + })(); +})(); + +(function x() { + (function y() { + (function z() { + assert.ifError(err); + })(); + })(); +})(); diff --git a/test/message/if-error-has-good-stack.out b/test/message/if-error-has-good-stack.out new file mode 100644 index 00000000000000..fa72322b446f6a --- /dev/null +++ b/test/message/if-error-has-good-stack.out @@ -0,0 +1,19 @@ +assert.js:* + throw newErr; + ^ + +AssertionError [ERR_ASSERTION]: ifError got unwanted exception: test error + at z (*/if-error-has-good-stack.js:*:* + at y (*/if-error-has-good-stack.js:*:*) + at x (*/if-error-has-good-stack.js:*:*) + at Object. (*/if-error-has-good-stack.js:*:*) + at c (*/if-error-has-good-stack.js:*:*) + at b (*/if-error-has-good-stack.js:*:*) + at a (*/if-error-has-good-stack.js:*:*) + at Object. (*/if-error-has-good-stack.js:*:*) + at Module._compile (module.js:*:*) + at Object.Module._extensions..js (module.js:*:*) + at Module.load (module.js:*:*) + at tryModuleLoad (module.js:*:*) + at Function.Module._load (module.js:*:*) + at Function.Module.runMain (module.js:*:*) diff --git a/test/parallel/test-assert-if-error.js b/test/parallel/test-assert-if-error.js new file mode 100644 index 00000000000000..bcee765f9afc17 --- /dev/null +++ b/test/parallel/test-assert-if-error.js @@ -0,0 +1,82 @@ +'use strict'; + +require('../common'); +const assert = require('assert').strict; +/* eslint-disable no-restricted-properties */ + +// Test that assert.ifError has the correct stack trace of both stacks. + +let err; +// Create some random error frames. +(function a() { + (function b() { + (function c() { + err = new Error('test error'); + })(); + })(); +})(); + +const msg = err.message; +const stack = err.stack; + +(function x() { + (function y() { + (function z() { + let threw = false; + try { + assert.ifError(err); + } catch (e) { + assert.equal(e.message, 'ifError got unwanted exception: test error'); + assert.equal(err.message, msg); + assert.equal(e.actual, err); + assert.equal(e.actual.stack, stack); + assert.equal(e.expected, null); + assert.equal(e.operator, 'ifError'); + threw = true; + } + assert(threw); + })(); + })(); +})(); + +assert.throws( + () => assert.ifError(new TypeError()), + { + message: 'ifError got unwanted exception: TypeError' + } +); + +assert.throws( + () => assert.ifError({ stack: false }), + { + message: 'ifError got unwanted exception: { stack: false }' + } +); + +assert.throws( + () => assert.ifError({ constructor: null, message: '' }), + { + message: 'ifError got unwanted exception: ' + } +); + +assert.doesNotThrow(() => { assert.ifError(null); }); +assert.doesNotThrow(() => { assert.ifError(); }); +assert.doesNotThrow(() => { assert.ifError(undefined); }); +assert.doesNotThrow(() => { assert.ifError(false); }); + +// https://github.com/nodejs/node-v0.x-archive/issues/2893 +{ + let threw = false; + try { + // eslint-disable-next-line no-restricted-syntax + assert.throws(() => { + assert.ifError(null); + }); + } catch (e) { + threw = true; + assert.strictEqual(e.message, 'Missing expected exception.'); + assert(!e.stack.includes('throws'), e.stack); + } + assert(threw); +} diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index e8bf7949322279..70ba887fee54e1 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -452,11 +452,6 @@ assert.throws(makeBlock(thrower, TypeError)); 'a.doesNotThrow is not catching type matching errors'); } -assert.throws(() => { assert.ifError(new Error('test error')); }, - /^Error: test error$/); -assert.doesNotThrow(() => { assert.ifError(null); }); -assert.doesNotThrow(() => { assert.ifError(); }); - common.expectsError( () => assert.doesNotThrow(makeBlock(thrower, Error), 'user message'), { @@ -602,22 +597,6 @@ testAssertionMessage({ a: undefined, b: null }, '{ a: undefined, b: null }'); testAssertionMessage({ a: NaN, b: Infinity, c: -Infinity }, '{ a: NaN, b: Infinity, c: -Infinity }'); -// https://github.com/nodejs/node-v0.x-archive/issues/2893 -{ - let threw = false; - try { - // eslint-disable-next-line no-restricted-syntax - assert.throws(() => { - assert.ifError(null); - }); - } catch (e) { - threw = true; - assert.strictEqual(e.message, 'Missing expected exception.'); - assert.ok(!e.stack.includes('throws'), e.stack); - } - assert.ok(threw); -} - // https://github.com/nodejs/node-v0.x-archive/issues/5292 try { assert.strictEqual(1, 2); diff --git a/test/parallel/test-domain-implicit-fs.js b/test/parallel/test-domain-implicit-fs.js index 6e4127763ce51c..f3695989502a77 100644 --- a/test/parallel/test-domain-implicit-fs.js +++ b/test/parallel/test-domain-implicit-fs.js @@ -34,9 +34,9 @@ d.on('error', common.mustCall(function(er) { assert.strictEqual(er.domain, d); assert.strictEqual(er.domainThrown, true); assert.ok(!er.domainEmitter); - assert.strictEqual(er.code, 'ENOENT'); - assert.ok(/\bthis file does not exist\b/i.test(er.path)); - assert.strictEqual(typeof er.errno, 'number'); + assert.strictEqual(er.actual.code, 'ENOENT'); + assert.ok(/\bthis file does not exist\b/i.test(er.actual.path)); + assert.strictEqual(typeof er.actual.errno, 'number'); })); diff --git a/test/parallel/test-util-callbackify.js b/test/parallel/test-util-callbackify.js index bc2154940cb405..d8b2de314c9c49 100644 --- a/test/parallel/test-util-callbackify.js +++ b/test/parallel/test-util-callbackify.js @@ -220,7 +220,9 @@ const values = [ [fixture], common.mustCall((err, stdout, stderr) => { assert.ifError(err); - assert.strictEqual(stdout.trim(), fixture); + assert.strictEqual( + stdout.trim(), + `ifError got unwanted exception: ${fixture}`); assert.strictEqual(stderr, ''); }) );