Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
Make use of jest-leak-detector to detect tests leaking memory
  • Loading branch information
Miguel Jimenez Esun committed Nov 25, 2017
commit 7817781773a3d7928deb936cb12e78c6e4689cc8
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ exports[`--showConfig outputs config info and exits 1`] = `
\\"coveragePathIgnorePatterns\\": [
\\"/node_modules/\\"
],
\\"detectLeaks\\": false,
\\"globals\\": {},
\\"haste\\": {
\\"providesModuleNodeModules\\": []
Expand Down Expand Up @@ -66,6 +67,7 @@ exports[`--showConfig outputs config info and exits 1`] = `
\\"lcov\\",
\\"clover\\"
],
\\"detectLeaks\\": false,
\\"expand\\": false,
\\"listTests\\": false,
\\"mapCoverage\\": false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export const runAndTransformResultsToJestFormat = async ({
console: null,
displayName: config.displayName,
failureMessage,
leaks: false, // That's legacy code, just adding it so Flow is happy.
numFailingTests,
numPassingTests,
numPendingTests,
Expand Down
8 changes: 8 additions & 0 deletions packages/jest-cli/src/cli/args.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,14 @@ export const options = {
description: 'Print debugging info about your jest config.',
type: 'boolean',
},
detectLeaks: {
default: false,
description:
'**EXPERIMENTAL**: Detect memory leaks in tests. After executing a ' +
'test, it will try to garbage collect the global object used, and fail ' +
'if it was leaked',
type: 'boolean',
},
env: {
description:
'The test environment used for all tests. This can point to ' +
Expand Down
1 change: 1 addition & 0 deletions packages/jest-cli/src/test_result_helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export const buildFailureTestResult = (
console: null,
displayName: '',
failureMessage: null,
leaks: false,
numFailingTests: 0,
numPassingTests: 0,
numPendingTests: 0,
Expand Down
17 changes: 17 additions & 0 deletions packages/jest-cli/src/test_scheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,23 @@ export default class TestScheduler {
});
return Promise.resolve();
}
if (testResult.leaks) {
const message = [
'Your test suite is leaking memory! Please ensure all references are cleaned.',
Copy link
Member

Choose a reason for hiding this comment

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

I would put a bold statement on top and say "Experimental" etc. Nobody reads jest --help.

'',
'There is a number of things that can leak memory:',
' - Async operations that have not finished (e.g. fs.readFile).',
' - Timers not properly mocked (e.g. setInterval, setTimeout).',
' - Missed global listeners (e.g. process.on).',
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: "Global event listeners weren't cleaned up (e.g. process.on)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With #4904 that one is no longer necessary 😃

' - Keeping references to the global scope.',
].join('\n');
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 not use an array and just use backticks + string concatenation like in the rest of Jest? :)


await onFailure(test, {
message,
stack: new Error(message).stack,
});
return Promise.resolve();
Copy link
Member

Choose a reason for hiding this comment

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

return is enough, no? It's already an async function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this Promise.resolve() (not only mine, but all others) are there because of the eslint consistent-return rule 😞, but TBH I prefer keeping them and not adding an eslint-disable.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's sufficient to just return onFailure(...) since it's already a promise (same thing can be fixed in testResult.testResults.length block above)

}
addResult(aggregatedResults, testResult);
await this._dispatcher.onTestResult(test, testResult, aggregatedResults);
return this._bailIfNeeded(contexts, aggregatedResults, watcher);
Expand Down
1 change: 1 addition & 0 deletions packages/jest-config/src/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export default ({
clearMocks: false,
coveragePathIgnorePatterns: [NODE_MODULES_REGEXP],
coverageReporters: ['json', 'text', 'lcov', 'clover'],
detectLeaks: false,
expand: false,
globals: {},
haste: {
Expand Down
2 changes: 2 additions & 0 deletions packages/jest-config/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const getConfigs = (
coverageDirectory: options.coverageDirectory,
coverageReporters: options.coverageReporters,
coverageThreshold: options.coverageThreshold,
detectLeaks: options.detectLeaks,
expand: options.expand,
findRelatedTests: options.findRelatedTests,
forceExit: options.forceExit,
Expand Down Expand Up @@ -123,6 +124,7 @@ const getConfigs = (
clearMocks: options.clearMocks,
coveragePathIgnorePatterns: options.coveragePathIgnorePatterns,
cwd: options.cwd,
detectLeaks: options.detectLeaks,
displayName: options.displayName,
globals: options.globals,
haste: options.haste,
Expand Down
1 change: 1 addition & 0 deletions packages/jest-config/src/normalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ export default function normalize(options: InitialOptions, argv: Argv) {
case 'collectCoverage':
case 'coverageReporters':
case 'coverageThreshold':
case 'detectLeaks':
case 'displayName':
case 'expand':
case 'globals':
Expand Down
1 change: 1 addition & 0 deletions packages/jest-runner/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"jest-docblock": "^21.2.0",
"jest-haste-map": "^21.2.0",
"jest-jasmine2": "^21.2.1",
"jest-leak-detector": "^21.2.1",
"jest-message-util": "^21.2.1",
"jest-runtime": "^21.2.1",
"jest-util": "^21.2.1",
Expand Down
69 changes: 52 additions & 17 deletions packages/jest-runner/src/run_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,32 @@ import {
setGlobal,
} from 'jest-util';
import jasmine2 from 'jest-jasmine2';
import LeakDetector from 'jest-leak-detector';
import {getTestEnvironment} from 'jest-config';
import * as docblock from 'jest-docblock';

// The default jest-runner is required because it is the default test runner
// and required implicitly through the `testRunner` ProjectConfig option.
jasmine2;

export default (async function runTest(
// Keeping the core of "runTest" as a separate function (as "runTestHelper") is
// key to be able to detect memory leaks. Since all variables are local to the
// function, when "runTestHelper" finishes its execution, they can all be freed,
// UNLESS someone else is leaking them (and that's why we can detect the leak!).
Copy link
Member

Choose a reason for hiding this comment

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

"UNLESS something else" not someone.

//
// If we had all the code in a single function, we should manually nullify all
// references to verify if there is a leak, which is not maintainable and error
// prone. That's why "runTestHelper" CANNOT be inlined inside "runTest".
async function runTestHelper(
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 call it _runTest or runTestInternal? I don't like "util" or "helper" in general :D

path: Path,
globalConfig: GlobalConfig,
config: ProjectConfig,
resolver: Resolver,
) {
let testSource;

try {
testSource = fs.readFileSync(path, 'utf8');
} catch (e) {
return Promise.reject(e);
Copy link
Member

Choose a reason for hiding this comment

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

Such clowny code.

Copy link
Member

Choose a reason for hiding this comment

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

The function wasn't async at the point it was written 🙂

}

): Promise<TestResult> {
const testSource = fs.readFileSync(path, 'utf8');
const parsedDocblock = docblock.parse(docblock.extract(testSource));
const customEnvironment = parsedDocblock['jest-environment'];

let testEnvironment = config.testEnvironment;

if (customEnvironment) {
Expand All @@ -66,6 +69,10 @@ export default (async function runTest(
>);

const environment = new TestEnvironment(config);
const environmentLeakDetector = config.detectLeaks
Copy link
Member

Choose a reason for hiding this comment

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

Can you call this variable just leakDetector? It's soooo long.

? new LeakDetector(environment)
: false;
Copy link
Member

Choose a reason for hiding this comment

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

null?


const consoleOut = globalConfig.useStderr ? process.stderr : process.stdout;
const consoleFormatter = (type, message) =>
getConsoleOutput(
Expand All @@ -76,24 +83,25 @@ export default (async function runTest(
);

let testConsole;

if (globalConfig.silent) {
testConsole = new NullConsole(consoleOut, process.stderr, consoleFormatter);
} else if (globalConfig.verbose) {
testConsole = new Console(consoleOut, process.stderr, consoleFormatter);
} else {
if (globalConfig.verbose) {
testConsole = new Console(consoleOut, process.stderr, consoleFormatter);
} else {
testConsole = new BufferedConsole();
}
testConsole = new BufferedConsole();
}

const cacheFS = {[path]: testSource};
setGlobal(environment.global, 'console', testConsole);

const runtime = new Runtime(config, environment, resolver, cacheFS, {
collectCoverage: globalConfig.collectCoverage,
collectCoverageFrom: globalConfig.collectCoverageFrom,
collectCoverageOnlyFrom: globalConfig.collectCoverageOnlyFrom,
mapCoverage: globalConfig.mapCoverage,
});

const start = Date.now();
await environment.setup();
try {
Expand All @@ -106,22 +114,49 @@ export default (async function runTest(
);
const testCount =
result.numPassingTests + result.numFailingTests + result.numPendingTests;

result.leaks = environmentLeakDetector;
result.perfStats = {end: Date.now(), start};
result.testFilePath = path;
result.coverage = runtime.getAllCoverageInfo();
result.sourceMaps = runtime.getSourceMapInfo();
result.console = testConsole.getBuffer();
result.skipped = testCount === result.numPendingTests;
result.displayName = config.displayName;

if (globalConfig.logHeapUsage) {
if (global.gc) {
global.gc();
}
result.memoryUsage = process.memoryUsage().heapUsed;
}

// Delay the resolution to allow log messages to be output.
return new Promise(resolve => setImmediate(() => resolve(result)));
await new Promise(resolve => setImmediate(resolve));

return result;
Copy link
Member

Choose a reason for hiding this comment

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

Is this just simplifying the code or is there another reason you made this change?

} finally {
await environment.teardown();
}
});
}

export default async function runTest(
path: Path,
globalConfig: GlobalConfig,
config: ProjectConfig,
resolver: Resolver,
): Promise<TestResult> {
const result: TestResult = await runTestHelper(
path,
globalConfig,
config,
resolver,
);

// Resolve leak detector, so that the object is JSON serializable.
if (result.leaks instanceof LeakDetector) {
result.leaks = result.leaks.isLeaking();
Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I get it. This seems super hacky. How about instead of doing this we change runTestHelper to return {leakDetector, result} and turn result.leaks into a boolean?

}

return result;
}
2 changes: 2 additions & 0 deletions test_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const DEFAULT_GLOBAL_CONFIG: GlobalConfig = {
coverageDirectory: 'coverage',
coverageReporters: [],
coverageThreshold: {global: {}},
detectLeaks: false,
expand: false,
findRelatedTests: false,
forceExit: false,
Expand Down Expand Up @@ -63,6 +64,7 @@ const DEFAULT_PROJECT_CONFIG: ProjectConfig = {
clearMocks: false,
coveragePathIgnorePatterns: [],
cwd: '/test_root_dir/',
detectLeaks: false,
displayName: undefined,
globals: {},
haste: {
Expand Down
4 changes: 4 additions & 0 deletions types/Config.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export type DefaultOptions = {|
expand: boolean,
globals: ConfigGlobals,
haste: HasteConfig,
detectLeaks: boolean,
mapCoverage: boolean,
moduleDirectories: Array<string>,
moduleFileExtensions: Array<string>,
Expand Down Expand Up @@ -78,6 +79,7 @@ export type InitialOptions = {
coveragePathIgnorePatterns?: Array<string>,
coverageReporters?: Array<string>,
coverageThreshold?: {global: {[key: string]: number}},
detectLeaks?: boolean,
displayName?: string,
expand?: boolean,
findRelatedTests?: boolean,
Expand Down Expand Up @@ -154,6 +156,7 @@ export type GlobalConfig = {|
coverageDirectory: string,
coverageReporters: Array<string>,
coverageThreshold: {global: {[key: string]: number}},
detectLeaks: boolean,
expand: boolean,
findRelatedTests: boolean,
forceExit: boolean,
Expand Down Expand Up @@ -197,6 +200,7 @@ export type ProjectConfig = {|
clearMocks: boolean,
coveragePathIgnorePatterns: Array<string>,
cwd: Path,
detectLeaks: boolean,
displayName: ?string,
globals: ConfigGlobals,
haste: HasteConfig,
Expand Down
2 changes: 2 additions & 0 deletions types/TestResult.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* @flow
*/

import LeakDetector from 'jest-leak-detector';
import type {ConsoleBuffer} from './Console';

export type RawFileCoverage = {|
Expand Down Expand Up @@ -138,6 +139,7 @@ export type TestResult = {|
console: ?ConsoleBuffer,
coverage?: RawCoverage,
displayName: ?string,
leaks: LeakDetector | boolean,
Copy link
Member

Choose a reason for hiding this comment

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

Woah, this doesn't seem like a good type. What does it represent?

memoryUsage?: Bytes,
failureMessage: ?string,
numFailingTests: number,
Expand Down