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

Add --detectOpenHandles flag #6130

Merged
merged 21 commits into from
May 6, 2018
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

### Features

* `[jest-cli]` Add `--detectOpenHandles` flag which enables Jest to potentially
track down handles keeping it open after tests are complete.
([#6130](https://github.com/facebook/jest/pull/6130))
* `[jest-jasmine2]` Add data driven testing based on `jest-each`
([#6102](https://github.com/facebook/jest/pull/6102))
* `[jest-matcher-utils]` Change "suggest to equal" message to be more advisory
Expand Down
2 changes: 2 additions & 0 deletions TestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const DEFAULT_GLOBAL_CONFIG: GlobalConfig = {
coverageReporters: [],
coverageThreshold: {global: {}},
detectLeaks: false,
detectOpenHandles: false,
enabledTestsMap: null,
expand: false,
filter: null,
Expand Down Expand Up @@ -72,6 +73,7 @@ const DEFAULT_PROJECT_CONFIG: ProjectConfig = {
coveragePathIgnorePatterns: [],
cwd: '/test_root_dir/',
detectLeaks: false,
detectOpenHandles: false,
displayName: undefined,
filter: null,
forceCoverageMatch: [],
Expand Down
11 changes: 10 additions & 1 deletion docs/CLI.md
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,14 @@ output.

Print debugging info about your Jest config.

### `--detectOpenHandles`

Attempt to collect and print open handles preventing Jest from exiting cleanly.
Use this in cases where you need to use `--forceExit` in order for Jest to exit
to potentially track down the reason. Implemented using
[`async_hooks`](https://nodejs.org/api/async_hooks.html), so it only works in
Node 8 and newer.

### `--env=<environment>`

The test environment used for all tests. This can point to any file or node
Expand All @@ -196,7 +204,8 @@ resources set up by test code cannot be adequately cleaned up. _Note: This
feature is an escape-hatch. If Jest doesn't exit at the end of a test run, it
means external resources are still being held on to or timers are still pending
in your code. It is advised to tear down external resources after each test to
make sure Jest can shut down cleanly._
make sure Jest can shut down cleanly. You can use `--detectOpenHandles` to help
track it down._

### `--help`

Expand Down
1 change: 1 addition & 0 deletions docs/Configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,7 @@ structure as the first argument and return it:
"numPassedTests": number,
"numFailedTests": number,
"numPendingTests": number,
"openHandles": Array<Error>,
"testResults": [{
"numFailingTests": number,
"numPassingTests": number,
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/Utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ const extractSummary = (

let rest = cleanupStackTrace(
// remove all timestamps
stdout.slice(0, -match[0].length).replace(/\s*\(\d*\.?\d+m?s\)$/gm, ''),
stdout.replace(match[0], '').replace(/\s*\(\d*\.?\d+m?s\)$/gm, ''),
Copy link
Member Author

Choose a reason for hiding this comment

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

now that there is sometimes output after the summary, this needs to be more precise

);

if (stripLocation) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ exports[`CLI accepts exact file names if matchers matched 1`] = `
"PASS foo/bar.spec.js
✓ foo

"

Force exiting Jest - have you considered using \`--detectOpenHandles\`?"
`;

exports[`CLI accepts exact file names if matchers matched 2`] = `
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ exports[`prints console.logs when run with forceExit 1`] = `
"PASS __tests__/a-banana.js
✓ banana

"

Force exiting Jest - have you considered using \`--detectOpenHandles\`?"
`;

exports[`prints console.logs when run with forceExit 2`] = `
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`prints message about flag on forceExit 1`] = `"Force exiting Jest - have you considered using \`--detectOpenHandles\`?"`;

exports[`prints message about flag on slow tests 1`] = `
"Jest has not exited 1000ms after the test run finished

Have you considered using \`--detectOpenHandles\`?"
`;

exports[`prints out info about open handlers 1`] = `
"Jest has detected the following 1 open handle potentially keeping Jest from exiting:

● GETADDRINFOREQWRAP

5 | const app = new http.Server();
6 |
> 7 | app.listen({host: 'localhost', port: 0});
| ^
8 |

at Object.<anonymous> (server.js:7:5)
at Object.<anonymous> (__tests__/test.js:3:1)"
`;
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ exports[`--showConfig outputs config info and exits 1`] = `
\\"/node_modules/\\"
],
\\"detectLeaks\\": false,
\\"detectOpenHandles\\": false,
\\"filter\\": null,
\\"forceCoverageMatch\\": [],
\\"globals\\": {},
Expand Down Expand Up @@ -79,6 +80,7 @@ exports[`--showConfig outputs config info and exits 1`] = `
\\"clover\\"
],
\\"detectLeaks\\": false,
\\"detectOpenHandles\\": false,
\\"expand\\": false,
\\"filter\\": null,
\\"globalSetup\\": null,
Expand Down
62 changes: 62 additions & 0 deletions integration-tests/__tests__/detect_open_handles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/
'use strict';

const runJest = require('../runJest');

try {
// $FlowFixMe: Node core
require('async_hooks');
} catch (e) {
if (e.code === 'MODULE_NOT_FOUND') {
// eslint-disable-next-line jest/no-focused-tests
fit('skip test for unsupported nodes', () => {
console.warn('Skipping test for node ' + process.version);
});
} else {
throw e;
}
}

function getTextAfterTest(stderr) {
return stderr.split('Ran all test suites.')[1].trim();
}

it('prints message about flag on slow tests', async () => {
const {stderr} = await runJest.until(
'detect-open-handles',
[],
'Jest has not exited 1000ms after the test run finished',
);
const textAfterTest = getTextAfterTest(stderr);

expect(textAfterTest).toMatchSnapshot();
});

it('prints message about flag on forceExit', async () => {
const {stderr} = await runJest.until(
'detect-open-handles',
['--forceExit'],
'Force exiting Jest',
);
const textAfterTest = getTextAfterTest(stderr);

expect(textAfterTest).toMatchSnapshot();
});

it('prints out info about open handlers', async () => {
const {stderr} = await runJest.until(
'detect-open-handles',
['--detectOpenHandles'],
'Jest has detected',
);
const textAfterTest = getTextAfterTest(stderr);

expect(textAfterTest).toMatchSnapshot();
});
5 changes: 5 additions & 0 deletions integration-tests/detect-open-handles/__tests__/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
require('../server');

test('something', () => {
expect(true).toBe(true);
});
5 changes: 5 additions & 0 deletions integration-tests/detect-open-handles/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"jest": {
"testEnvironment": "node"
}
}
7 changes: 7 additions & 0 deletions integration-tests/detect-open-handles/server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

const http = require('http');

const app = new http.Server();

app.listen({host: 'localhost', port: 0});
69 changes: 66 additions & 3 deletions integration-tests/runJest.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@
'use strict';

const path = require('path');
const {sync: spawnSync} = require('execa');
const execa = require('execa');
const {Writable} = require('readable-stream');
const {fileExists} = require('./Utils');

const {sync: spawnSync} = execa;

const JEST_PATH = path.resolve(__dirname, '../packages/jest-cli/bin/jest.js');

type RunJestOptions = {
Expand Down Expand Up @@ -67,9 +70,9 @@ function runJest(
// 'success', 'startTime', 'numTotalTests', 'numTotalTestSuites',
// 'numRuntimeErrorTestSuites', 'numPassedTests', 'numFailedTests',
// 'numPendingTests', 'testResults'
runJest.json = function(dir: string, args?: Array<string>) {
runJest.json = function(dir: string, args?: Array<string>, ...rest) {
args = [...(args || []), '--json'];
const result = runJest(dir, args);
const result = runJest(dir, args, ...rest);
try {
result.json = JSON.parse((result.stdout || '').toString());
} catch (e) {
Expand All @@ -85,4 +88,64 @@ runJest.json = function(dir: string, args?: Array<string>) {
return result;
};

// Runs `jest` until a given output is achieved, then kills it with `SIGTERM`
runJest.until = async function(
dir: string,
args?: Array<string>,
text: string,
options: RunJestOptions = {},
) {
const isRelative = dir[0] !== '/';

if (isRelative) {
dir = path.resolve(__dirname, dir);
}

const localPackageJson = path.resolve(dir, 'package.json');
if (!options.skipPkgJsonCheck && !fileExists(localPackageJson)) {
throw new Error(
`
Make sure you have a local package.json file at
"${localPackageJson}".
Otherwise Jest will try to traverse the directory tree and find the
the global package.json, which will send Jest into infinite loop.
`,
);
}

const env = options.nodePath
? Object.assign({}, process.env, {
FORCE_COLOR: 0,
NODE_PATH: options.nodePath,
})
: process.env;

const jestPromise = execa(JEST_PATH, args || [], {
cwd: dir,
env,
reject: false,
});

jestPromise.stderr.pipe(
new Writable({
write(chunk, encoding, callback) {
const output = chunk.toString('utf8');

if (output.includes(text)) {
jestPromise.kill();
}

callback();
},
}),
);

const result = await jestPromise;

// For compat with cross-spawn
result.status = result.code;

return result;
};

module.exports = runJest;
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"prettier": "^1.12.1",
"prettylint": "^1.0.0",
"progress": "^2.0.0",
"readable-stream": "^2.3.6",
"regenerator-runtime": "^0.11.0",
"resolve": "^1.4.0",
"rimraf": "^2.6.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ export const runAndTransformResultsToJestFormat = async ({
numFailingTests,
numPassingTests,
numPendingTests,
openHandles: [],
perfStats: {
// populated outside
end: 0,
Expand Down
7 changes: 7 additions & 0 deletions packages/jest-cli/src/cli/args.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,13 @@ export const options = {
'if it was leaked',
type: 'boolean',
},
detectOpenHandles: {
default: false,
description:
'Print out remaining open handles preventing Jest from exiting at the ' +
'end of a test run.',
type: 'boolean',
},
env: {
description:
'The test environment used for all tests. This can point to ' +
Expand Down
34 changes: 34 additions & 0 deletions packages/jest-cli/src/cli/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import chalk from 'chalk';
import createContext from '../lib/create_context';
import exit from 'exit';
import getChangedFilesPromise from '../get_changed_files_promise';
import {formatHandleErrors} from '../get_node_handles';
import fs from 'fs';
import handleDeprecationWarnings from '../lib/handle_deprecation_warnings';
import logDebugMessages from '../lib/log_debug_messages';
Expand All @@ -28,6 +29,7 @@ import runJest from '../run_jest';
import Runtime from 'jest-runtime';
import TestWatcher from '../test_watcher';
import watch from '../watch';
import pluralize from '../pluralize';
import yargs from 'yargs';
import rimraf from 'rimraf';
import {sync as realpath} from 'realpath-native';
Expand Down Expand Up @@ -101,6 +103,19 @@ export const runCLI = async (
);
}

const {openHandles} = results;

if (openHandles && openHandles.length) {
const openHandlesString = pluralize('open handle', openHandles.length, 's');

const message =
chalk.red(
`\nJest has detected the following ${openHandlesString} potentially keeping Jest from exiting:\n\n`,
) + formatHandleErrors(openHandles, configs[0]).join('\n\n');

console.error(message);
}

return Promise.resolve({globalConfig, results});
};

Expand All @@ -113,7 +128,26 @@ const readResultsAndExit = (
process.on('exit', () => (process.exitCode = code));

if (globalConfig.forceExit) {
if (!globalConfig.detectOpenHandles) {
console.error(
chalk.red(
'Force exiting Jest - have you considered using `--detectOpenHandles`?',
Copy link
Member

Choose a reason for hiding this comment

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

How about ending with […] to detect async operations that kept running after all tests finished?

),
);
}

exit(code);
} else if (!globalConfig.detectOpenHandles) {
setTimeout(() => {
const lines = [
chalk.red.bold(
'Jest has not exited 1000ms after the test run finished',
Copy link
Member

Choose a reason for hiding this comment

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

Can we do "Jest did not exit one second after the test run has completed. This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with --detectOpenHandles to troubleshoot this issue." or something similar?

),
chalk.red('Have you considered using `--detectOpenHandles`?'),
];
console.error(lines.join('\n\n'));
// $FlowFixMe: `unref` exists in Node
}, 1000).unref();
Copy link
Member

Choose a reason for hiding this comment

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

nice

}
};

Expand Down