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 all 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
11 changes: 2 additions & 9 deletions integration_tests/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,16 +156,9 @@ const extractSummary = (stdout: string) => {

// different versions of Node print different stack traces. This function
// unifies their output to make it possible to snapshot them.
// TODO: Remove when we drop support for node 4
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, '')
.replace(/\n.*at (new )?Promise \(<anonymous>\).*$/gm, '')
.replace(/\n.*at <anonymous>.*$/gm, '')
.replace(/\n.*at Generator.next \(<anonymous>\).*$/gm, '')
.replace(/^.*at.*[\s][\(]?(\S*\:\d*\:\d*).*$/gm, ' at $1');
return output.replace(/^.*at.*[\s][\(]?(\S*\:\d*\:\d*).*$/gm, ' at $1');
};

module.exports = {
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
43 changes: 42 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,27 @@ 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)
at internal/process/next_tick.js:188:7
Copy link
Member Author

Choose a reason for hiding this comment

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

this last one I just saw in the integration tests. It should be the same as the one above it. Ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to come from the remaining replace in cleanupStackTrace in integration_tests/utils.js. Not sure how to handle it

Copy link
Member Author

Choose a reason for hiding this comment

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

If we really want to do it, I can move this next_tick part down there, no reason to have it in message-util

Copy link
Member Author

Choose a reason for hiding this comment

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

Figured it out, this is node 4 missing the calling function's name.

`;

it('should exclude jasmine from stack trace for Unix paths.', () => {
const messages = formatResultsErrors(
[
Expand Down Expand Up @@ -62,3 +83,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();
});
48 changes: 44 additions & 4 deletions packages/jest-message-util/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ 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()
// this is to have the tests be the same in node 4 and node 6.
// TODO: Remove when we drop support for node 4
.concat(new RegExp('internal/process/next_tick.js'));
} 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 @@ -26,7 +39,10 @@ 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 JEST_INTERNALS_IGNORE = /^\s+at.*?jest(-.*?)?(\/|\\)(build|node_modules|packages)(\/|\\)/;
const ANONYMOUS_FN_IGNORE = /^\s+at <anonymous>.*$/;
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 +122,26 @@ const removeInternalStackEntries = (lines, options: StackTraceOptions) => {
let pathCounter = 0;

return lines.filter(line => {
const isPath = STACK_PATH_REGEXP.test(line);
if (!isPath) {
if (ANONYMOUS_FN_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 +150,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 (JEST_INTERNALS_IGNORE.test(line)) {
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