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.

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

Always remove node internals from stacktraces #4695

Merged
merged 7 commits into from
Oct 15, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
* `[jest-editor-support]` Fix editor support test for node 4 ([#4640](https://github.com/facebook/jest/pull/4640))
* `[jest-mock]` Support mocking constructor in `mockImplementationOnce` ([#4599](https://github.com/facebook/jest/pull/4599))
* `[jest-runtime]` Fix manual user mocks not working with custom resolver ([#4489](https://github.com/facebook/jest/pull/4489))
* `[jest-runtime]` Move `babel-core` to peer dependenies so it works with Babel 7 ([#4557](https://github.com/facebook/jest/pull/4557))
* `[jest-runtime]` Move `babel-core` to peer dependencies so it works with Babel 7 ([#4557](https://github.com/facebook/jest/pull/4557))
* `[jest-util]` Fix `runOnlyPendingTimers` for `setTimeout` inside `setImmediate` ([#4608](https://github.com/facebook/jest/pull/4608))
* `[jest-message-util]` Always remove node internals from stacktraces ([#4695](https://github.com/facebook/jest/pull/4695))

### Features
* `[jest-environment-*]` [**BREAKING**] Add Async Test Environment APIs, dispose is now teardown ([#4506](https://github.com/facebook/jest/pull/4506))
Expand Down
7 changes: 0 additions & 7 deletions integration_tests/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,19 +158,12 @@ const extractSummary = (stdout: string) => {
// unifies their output to make it possible to snapshot them.
const cleanupStackTrace = (output: string) => {
return output
.replace(/\n.*at.*timers\.js.*$/gm, '')
.replace(/\n.*at.*assert\.js.*$/gm, '')
.replace(/\n.*at.*node\.js.*$/gm, '')
.replace(/\n.*at.*next_tick\.js.*$/gm, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this needs to be updated on Node 4? Not sure why it fails now though.

Copy link
Member Author

Choose a reason for hiding this comment

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

next_tick is not under internal/ on node 4...

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not the integration test which fails, it's the new test I added in message-utils

.replace(/\n.*at (new )?Promise \(<anonymous>\).*$/gm, '')
.replace(/\n.*at <anonymous>.*$/gm, '')
.replace(/\n.*at Generator.next \(<anonymous>\).*$/gm, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

.replace(/^.*at.*[\s][\(]?(\S*\:\d*\:\d*).*$/gm, ' at $1');
};

module.exports = {
cleanup,
cleanupStackTrace,
copyDir,
createEmptyPackage,
extractSummary,
Expand Down
3 changes: 2 additions & 1 deletion packages/jest-message-util/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"dependencies": {
"chalk": "^2.0.1",
"micromatch": "^2.3.11",
"slash": "^1.0.0"
"slash": "^1.0.0",
"stack-utils": "^1.0.1"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,23 @@ exports[`.formatExecError() 1`] = `
"
`;

exports[`formatStackTrace should strip node internals 1`] = `
"<bold><red> <bold>● <bold>Unix test</></>


Expected value to be of type:
\\"number\\"
Received:
\\"\\"
type:
\\"string\\"
<dim> </>
<dim> </>
<dim> <dim>at Object.it (<dim>__tests__/test.js<dim>:8:14)<dim></>
<dim> </>
"
`;

exports[`should exclude jasmine from stack trace for Unix paths. 1`] = `
"<bold><red> <bold>● <bold>Unix test</></>

Expand Down
42 changes: 41 additions & 1 deletion packages/jest-message-util/src/__tests__/messages.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

'use strict';

const {formatResultsErrors, formatExecError} = require('../');
const {formatResultsErrors, formatExecError} = require('..');

const unixStackTrace =
` ` +
Expand All @@ -23,6 +23,26 @@ const unixStackTrace =
at Object.<anonymous> (../jest-jasmine2/build/jasmine-pit.js:35:32)
at attemptAsync (../jest-jasmine2/build/jasmine-2.4.1.js:1919:24)`;

const assertionStack =
' ' +
`
Expected value to be of type:
"number"
Received:
""
type:
"string"

at Object.it (__tests__/test.js:8:14)
at Object.asyncFn (node_modules/jest-jasmine2/build/jasmine_async.js:124:345)
at resolve (node_modules/jest-jasmine2/build/queue_runner.js:46:12)
at Promise (<anonymous>)
at mapper (node_modules/jest-jasmine2/build/queue_runner.js:34:499)
at promise.then (node_modules/jest-jasmine2/build/queue_runner.js:74:39)
at <anonymous>
at process._tickCallback (internal/process/next_tick.js:188:7)
`;

it('should exclude jasmine from stack trace for Unix paths.', () => {
const messages = formatResultsErrors(
[
Expand Down Expand Up @@ -62,3 +82,23 @@ it('.formatExecError()', () => {

expect(message).toMatchSnapshot();
});

it('formatStackTrace should strip node internals', () => {
const messages = formatResultsErrors(
[
{
ancestorTitles: [],
failureMessages: [assertionStack],
title: 'Unix test',
},
],
{
rootDir: '',
},
{
noStackTrace: false,
},
);

expect(messages).toMatchSnapshot();
});
43 changes: 40 additions & 3 deletions packages/jest-message-util/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ import path from 'path';
import chalk from 'chalk';
import micromatch from 'micromatch';
import slash from 'slash';
import StackUtils from 'stack-utils';

let nodeInternals = [];

try {
nodeInternals = StackUtils.nodeInternals();
} catch (e) {
// `StackUtils.nodeInternals()` fails in browsers. We don't need to remove
// node internals in the browser though, so no issue.
}

type StackTraceConfig = {
rootDir: string,
Expand All @@ -27,6 +37,9 @@ type StackTraceOptions = {
// filter for noisy stack trace lines
const JASMINE_IGNORE = /^\s+at(?:(?:.*?vendor\/|jasmine\-)|\s+jasmine\.buildExpectationResult)/;
const STACK_TRACE_IGNORE = /^\s+at.*?jest(-.*?)?(\/|\\)(build|node_modules|packages)(\/|\\)/;
const ANONYMOUS_TRACE_IGNORE = /^\s+at <anonymous>.*$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ANONYMOUS_FN_IGNORE?

const ANONYMOUS_PROMISE_IGNORE = /^\s+at (new )?Promise \(<anonymous>\).*$/;
const ANONYMOUS_GENERATOR_IGNORE = /^\s+at Generator.next \(<anonymous>\).*$/;
const TITLE_INDENT = ' ';
const MESSAGE_INDENT = ' ';
const STACK_INDENT = ' ';
Expand Down Expand Up @@ -106,10 +119,26 @@ const removeInternalStackEntries = (lines, options: StackTraceOptions) => {
let pathCounter = 0;

return lines.filter(line => {
const isPath = STACK_PATH_REGEXP.test(line);
if (!isPath) {
if (ANONYMOUS_TRACE_IGNORE.test(line)) {
return false;
}

if (ANONYMOUS_PROMISE_IGNORE.test(line)) {
return false;
}

if (ANONYMOUS_GENERATOR_IGNORE.test(line)) {
return false;
}

if (nodeInternals.some(internal => internal.test(line))) {
return false;
}

if (!STACK_PATH_REGEXP.test(line)) {
return true;
}

if (JASMINE_IGNORE.test(line)) {
return false;
}
Expand All @@ -118,7 +147,15 @@ const removeInternalStackEntries = (lines, options: StackTraceOptions) => {
return true; // always keep the first line even if it's from Jest
}

return !(STACK_TRACE_IGNORE.test(line) || options.noStackTrace);
if (options.noStackTrace) {
return false;
}

if (STACK_TRACE_IGNORE.test(line)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about renaming this to JEST_INTERNALS_IGNORE?

return false;
}

return true;
});
};

Expand Down
4 changes: 4 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5444,6 +5444,10 @@ sshpk@^1.7.0:
jsbn "~0.1.0"
tweetnacl "~0.14.0"

stack-utils@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/stack-utils/-/stack-utils-1.0.1.tgz#d4f33ab54e8e38778b0ca5cfd3b3afb12db68620"

state-toggle@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/state-toggle/-/state-toggle-1.0.0.tgz#d20f9a616bb4f0c3b98b91922d25b640aa2bc425"
Expand Down