Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: include .cause stacks in the error stack traces #4829

Merged
merged 9 commits into from
Mar 4, 2024
Merged

feat: include .cause stacks in the error stack traces #4829

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
62671e9
Append the cause stacks to the main stack trace
voxpelli Feb 9, 2022
86f0560
Fix tests
voxpelli Feb 9, 2022
a627c6a
Skip some string concatenation
voxpelli Feb 19, 2024
58300c4
Don't export needlessly + improve docs
voxpelli Feb 19, 2024
de2dfe4
Improved recursive filtering
voxpelli Feb 19, 2024
ba45e65
Added loop protection
voxpelli Feb 19, 2024
0ec8b7d
Same logic for "message" in cause trail as in top
voxpelli Feb 19, 2024
04f7008
Apply suggestions from code review
voxpelli Feb 20, 2024
571ac7c
Revert "Apply suggestions from code review"
voxpelli Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 51 additions & 19 deletions lib/reporters/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,56 @@ var generateDiff = (exports.generateDiff = function (actual, expected) {
}
});

/**
* Traverses err.cause and returns all stack traces
*
* @private
* @param {Error} err
* @param {Set<Error>} [seen]
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
* @return {{ message: string, msg: string, stack: string }}
*/
var getFullErrorStack = function (err, seen) {
voxpelli marked this conversation as resolved.
Show resolved Hide resolved
if (seen?.has(err)) {
return { message: '', msg: '<circular>', stack: '' };
}

var message;

if (typeof err.inspect === 'function') {
message = err.inspect() + '';
} else if (err.message && typeof err.message.toString === 'function') {
message = err.message + '';
} else {
message = '';
}

var msg;
var stack = err.stack || message;
var index = message ? stack.indexOf(message) : -1;

if (index === -1) {
msg = message;
} else {
index += message.length;
msg = stack.slice(0, index);
// remove msg from stack
stack = stack.slice(index + 1);

if (err.cause) {
seen = seen || new Set();
seen.add(err);
const causeStack = getFullErrorStack(err.cause, seen)
stack += '\n Caused by: ' + causeStack.msg + (causeStack.stack ? '\n' + causeStack.stack : '');
}
}

return {
message,
msg,
stack
};
};

/**
* Outputs the given `failures` as a list.
*
Expand All @@ -241,7 +291,6 @@ exports.list = function (failures) {
color('error stack', '\n%s\n');

// msg
var msg;
var err;
if (test.err && test.err.multiple) {
if (multipleTest !== test) {
Expand All @@ -252,25 +301,8 @@ exports.list = function (failures) {
} else {
err = test.err;
}
var message;
if (typeof err.inspect === 'function') {
message = err.inspect() + '';
} else if (err.message && typeof err.message.toString === 'function') {
message = err.message + '';
} else {
message = '';
}
var stack = err.stack || message;
var index = message ? stack.indexOf(message) : -1;

if (index === -1) {
msg = message;
} else {
index += message.length;
msg = stack.slice(0, index);
// remove msg from stack
stack = stack.slice(index + 1);
}
var { message, msg, stack } = getFullErrorStack(err);

// uncaught
if (err.uncaught) {
Expand Down
21 changes: 16 additions & 5 deletions lib/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,11 +443,22 @@ Runner.prototype.fail = function (test, err, force) {
err = thrown2Error(err);
}

try {
err.stack =
this.fullStackTrace || !err.stack ? err.stack : stackFilter(err.stack);
} catch (ignore) {
// some environments do not take kindly to monkeying with the stack
// Filter the stack traces
if (!this.fullStackTrace) {
const alreadyFiltered = new Set();
let currentErr = err;

while (currentErr?.stack && !alreadyFiltered.has(currentErr)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?.

From https://github.com/mochajs/mocha/actions/runs/8068123423/job/22040196920?pr=4829:

nps is executing `build` : rollup -c ./rollup.config.js

./browser-entry.js → ./mocha.js...
[!] (plugin at position 4) SyntaxError: Unexpected token (467:22) in /home/runner/work/mocha/mocha/lib/runner.js
lib/runner.js (467:22)
SyntaxError: Unexpected token (467:22) in /home/runner/work/mocha/mocha/lib/runner.js
    at Parser.pp$4.raise (/home/runner/work/mocha/mocha/node_modules/rollup-plugin-node-globals/node_modules/acorn/dist/acorn.js:2757:13)

@voxpelli what do you want to do here? My intuition is to delay using ?. until we redo the rollup builds / etc..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, removing it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now 0 for 2 in making suggestions on this PR. Not great for me. 😂

alreadyFiltered.add(currentErr);

try {
currentErr.stack = stackFilter(currentErr.stack);
} catch (ignore) {
// some environments do not take kindly to monkeying with the stack
}

currentErr = currentErr.cause;
}
}

this.emit(constants.EVENT_TEST_FAIL, test, err);
Expand Down
102 changes: 102 additions & 0 deletions test/reporters/base.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,108 @@ describe('Base reporter', function () {
expect(errOut, 'to be', '1) test title:\n Error\n foo\n bar');
});

describe('error causes', function () {
it('should append any error cause trail to stack traces', function () {
var err = {
message: 'Error',
stack: 'Error\nfoo\nbar',
showDiff: false,
cause: {
message: 'Cause1',
stack: 'Cause1\nbar\nfoo',
showDiff: false,
cause: {
message: 'Cause2',
stack: 'Cause2\nabc\nxyz',
showDiff: false
}
}
};
var test = makeTest(err);

list([test]);

var errOut = stdout.join('\n').trim();
expect(
errOut,
'to be',
'1) test title:\n Error\n foo\n bar\n Caused by: Cause1\n bar\n foo\n Caused by: Cause2\n abc\n xyz'
);
});

it('should not get stuck in a hypothetical circular error cause trail', function () {
var err1 = {
message: 'Error',
stack: 'Error\nfoo\nbar',
showDiff: false,
};
var err2 = {
message: 'Cause1',
stack: 'Cause1\nbar\nfoo',
showDiff: false,
cause: err1
}
err1.cause = err2;

var test = makeTest(err1);

list([test]);

var errOut = stdout.join('\n').trim();
expect(
errOut,
'to be',
'1) test title:\n Error\n foo\n bar\n Caused by: Cause1\n bar\n foo\n Caused by: <circular>'
);
});

it("should set an empty cause if neither 'inspect' nor 'message' is set", function () {
var err = {
message: 'Error',
stack: 'Error\nfoo\nbar',
showDiff: false,
cause: {
showDiff: false,
}
};

var test = makeTest(err);

list([test]);

var errOut = stdout.join('\n').trim();
expect(
errOut,
'to be',
'1) test title:\n Error\n foo\n bar\n Caused by:'
);
});

it('should not add cause trail if error does not contain message', function () {
var err = {
message: 'Error',
stack: 'foo\nbar',
showDiff: false,
cause: {
message: 'Cause1',
stack: 'Cause1\nbar\nfoo',
showDiff: false,
cause: {
message: 'Cause2',
stack: 'Cause2\nabc\nxyz',
showDiff: false
}
}
};
var test = makeTest(err);

list([test]);

var errOut = stdout.join('\n').trim();
expect(errOut, 'to be', '1) test title:\n Error\n foo\n bar');
});
});

it('should list multiple Errors per test', function () {
var err = new Error('First Error');
err.multiple = [new Error('Second Error - same test')];
Expand Down
34 changes: 34 additions & 0 deletions test/unit/runner.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,22 @@ describe('Runner', function () {
});
runner.fail(hook, err);
});

it('should prettify stack-traces in error cause trail', function (done) {
var hook = new Hook();
hook.parent = suite;
var causeErr = new Error();
// Fake stack-trace
causeErr.stack = stack.join('\n');
var err = new Error();
err.cause = causeErr;

runner.on(EVENT_TEST_FAIL, function (_hook, _err) {
expect(_err.cause.stack, 'to be', stack.slice(0, 3).join('\n'));
done();
});
runner.fail(hook, err);
});
});

describe('long', function () {
Expand All @@ -647,6 +663,24 @@ describe('Runner', function () {
});
runner.fail(hook, err);
});

it('should display full stack-traces in error cause trail', function (done) {
var hook = new Hook();
hook.parent = suite;
var causeErr = new Error();
// Fake stack-trace
causeErr.stack = stack.join('\n');
var err = new Error();
err.cause = causeErr;
// Add --stack-trace option
runner.fullStackTrace = true;

runner.on(EVENT_TEST_FAIL, function (_hook, _err) {
expect(_err.cause.stack, 'to be', stack.join('\n'));
done();
});
runner.fail(hook, err);
});
});

describe('ginormous', function () {
Expand Down
Loading