Skip to content

Commit

Permalink
test: improve logged errors
Browse files Browse the repository at this point in the history
To indicate which lines are test lines and which from Node.js core,
it's good to rely on `util.inspect()` while inspecting errors.

The stack was accessed directly instead in multiple cases and logging
that does not provide as much information as using `util.inspect()`.

PR-URL: #31425
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
  • Loading branch information
BridgeAR authored and codebytere committed Feb 17, 2020
1 parent 1484f5a commit 89dcf73
Show file tree
Hide file tree
Showing 14 changed files with 35 additions and 30 deletions.
17 changes: 10 additions & 7 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,19 +139,22 @@ if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) {
process._rawDebug();
throw new Error(`same id added to destroy list twice (${id})`);
}
destroyListList[id] = new Error().stack;
destroyListList[id] = util.inspect(new Error());
_queueDestroyAsyncId(id);
};

require('async_hooks').createHook({
init(id, ty, tr, r) {
init(id, ty, tr, resource) {
if (initHandles[id]) {
process._rawDebug(
`Is same resource: ${r === initHandles[id].resource}`);
`Is same resource: ${resource === initHandles[id].resource}`);
process._rawDebug(`Previous stack:\n${initHandles[id].stack}\n`);
throw new Error(`init called twice for same id (${id})`);
}
initHandles[id] = { resource: r, stack: new Error().stack.substr(6) };
initHandles[id] = {
resource,
stack: util.inspect(new Error()).substr(6)
};
},
before() { },
after() { },
Expand All @@ -161,7 +164,7 @@ if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) {
process._rawDebug();
throw new Error(`destroy called for same id (${id})`);
}
destroydIdsList[id] = new Error().stack;
destroydIdsList[id] = util.inspect(new Error());
},
}).enable();
}
Expand Down Expand Up @@ -345,7 +348,7 @@ function _mustCallInner(fn, criteria = 1, field) {
const context = {
[field]: criteria,
actual: 0,
stack: (new Error()).stack,
stack: util.inspect(new Error()),
name: fn.name || '<anonymous>'
};

Expand Down Expand Up @@ -530,7 +533,7 @@ function expectWarning(nameOrMap, expected, code) {
function expectsError(validator, exact) {
return mustCall((...args) => {
if (args.length !== 1) {
// Do not use `assert.strictEqual()` to prevent `util.inspect` from
// Do not use `assert.strictEqual()` to prevent `inspect` from
// always being called.
assert.fail(`Expected one argument, got ${util.inspect(args)}`);
}
Expand Down
3 changes: 2 additions & 1 deletion test/common/wpt.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const fs = require('fs');
const fsPromises = fs.promises;
const path = require('path');
const vm = require('vm');
const { inspect } = require('util');

// https://github.com/w3c/testharness.js/blob/master/testharness.js
// TODO: get rid of this half-baked harness in favor of the one
Expand Down Expand Up @@ -354,7 +355,7 @@ class WPTRunner {
this.fail(filename, {
name: '',
message: err.message,
stack: err.stack
stack: inspect(err)
}, 'UNCAUGHT');
this.inProgress.delete(filename);
}
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/uncaught-exceptions/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const domain = require('domain');

var d = domain.create();
d.on('error', function(err) {
console.log('[ignored]', err.stack);
console.log('[ignored]', err);
});

d.run(function() {
Expand Down
2 changes: 1 addition & 1 deletion test/message/vm_display_runtime_error.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ console.error('beginning');
try {
vm.runInThisContext('throw new Error("boo!")', { filename: 'test.vm' });
} catch (err) {
console.error(err.stack);
console.error(err);
}

vm.runInThisContext('throw new Error("spooky!")', { filename: 'test.vm' });
Expand Down
2 changes: 1 addition & 1 deletion test/message/vm_display_syntax_error.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ console.error('beginning');
try {
vm.runInThisContext('var 4;', { filename: 'foo.vm', displayErrors: true });
} catch (err) {
console.error(err.stack);
console.error(err);
}

vm.runInThisContext('var 5;', { filename: 'test.vm' });
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-assert-if-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ assert.ifError(undefined);
} catch (e) {
threw = true;
assert.strictEqual(e.message, 'Missing expected exception.');
assert(!e.stack.includes('throws'), e.stack);
assert(!e.stack.includes('throws'), e);
}
assert(threw);
}
2 changes: 1 addition & 1 deletion test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ assert.throws(
threw = true;
assert.ok(e.message.includes(rangeError.message));
assert.ok(e instanceof assert.AssertionError);
assert.ok(!e.stack.includes('doesNotThrow'), e.stack);
assert.ok(!e.stack.includes('doesNotThrow'), e);
}
assert.ok(threw);
}
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http-request-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ server.listen(0, common.mustCall(function() {
res.resume();
server.close();
})).on('error', function(e) {
console.error(e.stack);
console.error(e);
process.exit(1);
});
}));
2 changes: 1 addition & 1 deletion test/parallel/test-http-unix-socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ server.listen(common.PIPE, common.mustCall(function() {
}));

req.on('error', function(e) {
assert.fail(e.stack);
assert.fail(e);
});

req.end();
Expand Down
11 changes: 6 additions & 5 deletions test/parallel/test-promises-unhandled-rejections.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const common = require('../common');
const assert = require('assert');
const domain = require('domain');
const { inspect } = require('util');

common.disableCrashOnUnhandledRejection();

Expand All @@ -14,8 +15,8 @@ const asyncTest = (function() {

function fail(error) {
const stack = currentTest ?
`${error.stack}\nFrom previous event:\n${currentTest.stack}` :
error.stack;
`${inspect(error)}\nFrom previous event:\n${currentTest.stack}` :
inspect(error);

if (currentTest)
process.stderr.write(`'${currentTest.description}' failed\n\n`);
Expand Down Expand Up @@ -44,11 +45,11 @@ const asyncTest = (function() {
}

return function asyncTest(description, fn) {
const stack = new Error().stack.split('\n').slice(1).join('\n');
const stack = inspect(new Error()).split('\n').slice(1).join('\n');
asyncTestQueue.push({
action: fn,
stack: stack,
description: description
stack,
description
});
if (!asyncTestsEnabled) {
asyncTestsEnabled = true;
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-tls-min-max-version.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';
const common = require('../common');
const fixtures = require('../common/fixtures');
const { inspect } = require('util');

// Check min/max protocol versions.

Expand All @@ -16,7 +17,7 @@ function test(cmin, cmax, cprot, smin, smax, sprot, proto, cerr, serr) {
// Report where test was called from. Strip leading garbage from
// at Object.<anonymous> (file:line)
// from the stack location, we only want the file:line part.
const where = (new Error()).stack.split('\n')[2].replace(/[^(]*/, '');
const where = inspect(new Error()).split('\n')[2].replace(/[^(]*/, '');
connect({
client: {
checkServerIdentity: (servername, cert) => { },
Expand Down
6 changes: 1 addition & 5 deletions test/parallel/test-tls-securepair-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const key = fixtures.readKey('rsa_private.pem');
const cert = fixtures.readKey('rsa_cert.crt');

function log(a) {
console.error(`***server*** ${a}`);
console.error('***server***', a);
}

const server = net.createServer(common.mustCall(function(socket) {
Expand Down Expand Up @@ -74,21 +74,18 @@ const server = net.createServer(common.mustCall(function(socket) {
pair.cleartext.on('error', function(err) {
log('got error: ');
log(err);
log(err.stack);
socket.destroy();
});

pair.encrypted.on('error', function(err) {
log('encrypted error: ');
log(err);
log(err.stack);
socket.destroy();
});

socket.on('error', function(err) {
log('socket error: ');
log(err);
log(err.stack);
socket.destroy();
});

Expand All @@ -99,7 +96,6 @@ const server = net.createServer(common.mustCall(function(socket) {
pair.on('error', function(err) {
log('secure error: ');
log(err);
log(err.stack);
socket.destroy();
});
}));
Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-tls-set-ciphers.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
'use strict';
const common = require('../common');
if (!common.hasCrypto) common.skip('missing crypto');
if (!common.hasCrypto)
common.skip('missing crypto');

const fixtures = require('../common/fixtures');
const { inspect } = require('util');

// Test cipher: option for TLS.

Expand All @@ -12,7 +15,7 @@ const {

function test(cciphers, sciphers, cipher, cerr, serr) {
assert(cipher || cerr || serr, 'test missing any expectations');
const where = (new Error()).stack.split('\n')[2].replace(/[^(]*/, '');
const where = inspect(new Error()).split('\n')[2].replace(/[^(]*/, '');
connect({
client: {
checkServerIdentity: (servername, cert) => { },
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-vm-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
'use strict';
require('../common');
const assert = require('assert');

const { inspect } = require('util');
const vm = require('vm');
const Script = vm.Script;
let script = new Script('"passed";');
Expand Down Expand Up @@ -59,7 +59,7 @@ try {
} catch (e) {
gh1140Exception = e;
assert.ok(/expected-filename/.test(e.stack),
`expected appearance of filename in Error stack: ${e.stack}`);
`expected appearance of filename in Error stack: ${inspect(e)}`);
}
// This is outside of catch block to confirm catch block ran.
assert.strictEqual(gh1140Exception.toString(), 'Error');
Expand Down

0 comments on commit 89dcf73

Please sign in to comment.