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

Re-inject native Node modules #4970

Merged
merged 4 commits into from
Nov 29, 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
17 changes: 17 additions & 0 deletions integration_tests/__tests__/leak_detection.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* 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');

it('Makes sure that Jest does not leak the environment', () => {
const result = runJest.json('leak-detection', ['--detectLeaks']).json;

expect(result.success).toBe(true);
});
21 changes: 21 additions & 0 deletions integration_tests/__tests__/require_all_modules.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* 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');

it('Makes sure that no native module makes Jest crash', () => {
const result = runJest.json('require-all-modules').json;

if (!result.success) {
console.warn(result);
Copy link
Member

Choose a reason for hiding this comment

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

for debugging? should it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it so that if a module throws when required, we can know who was based on the console 🙂

}

expect(result.success).toBe(true);
});
5 changes: 5 additions & 0 deletions integration_tests/leak-detection/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"jest": {
"testEnvironment": "node"
}
}
27 changes: 27 additions & 0 deletions integration_tests/leak-detection/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* 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.
*/

const fs = require('fs');
const http = require('http');

it('expands a native module', () => {
fs.expandingNativeObject = () => {
console.log(global);
};
});

it('expands the prototype of a native constructor', () => {
http.ServerResponse.prototype.expandingNativePrototype = () => {
console.log(global);
};
});

it('adds listeners to process', () => {
process.on('foo', () => {
console.log(global);
});
});
5 changes: 5 additions & 0 deletions integration_tests/require-all-modules/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"jest": {
"testEnvironment": "node"
}
}
18 changes: 18 additions & 0 deletions integration_tests/require-all-modules/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* 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.
*/

it('requires all native modules to check they all work', () => {
const modules = Object.keys(process.binding('natives')).filter(module =>
/^[^_][^\/]*$/.test(module)
);

// Node 6 has 34 native modules; so the total value has to be >= than 34.
expect(modules.length).not.toBeLessThan(34);

// Require all modules to verify they don't throw.
modules.forEach(module => require(module));
});
9 changes: 9 additions & 0 deletions jest-inspect
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/usr/bin/env node --inspect-brk
Copy link
Member

Choose a reason for hiding this comment

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

fancy :D

/**
* 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.
*/

require('./packages/jest-cli/bin/jest');
4 changes: 4 additions & 0 deletions packages/jest-editor-support/src/__tests__/runner.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ const {readFileSync} = require('fs');
const fixtures = path.resolve(__dirname, '../../../../fixtures');
import ProjectWorkspace from '../project_workspace';

// Win32 requires to spawn a process to kill the first one, by using "taskkill".
// Mocking "child_process" avoids the async spawn.
jest.mock('child_process');

// Replace `readFile` with `readFileSync` so we don't get multiple threads
jest.doMock('fs', () => {
return {
Expand Down
20 changes: 17 additions & 3 deletions packages/jest-jasmine2/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,17 @@ const JASMINE = require.resolve('./jasmine/jasmine_light.js');
async function jasmine2(
globalConfig: GlobalConfig,
config: ProjectConfig,
environment: Environment,
environment: ?Environment,
Copy link
Member

Choose a reason for hiding this comment

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

woah, let's not do this please. For example by renaming the environment variable below?

Copy link
Contributor Author

@mjesun mjesun Nov 28, 2017

Choose a reason for hiding this comment

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

We have to; I need to kill all environment references.

runtime: Runtime,
testPath: string,
): Promise<TestResult> {
// The "environment" parameter is nullable just so that we can clean its
Copy link
Member

Choose a reason for hiding this comment

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

will this help pinpointing #4710?

// reference after adding some variables to it; but you still need to pass
// it when calling "jasmine2".
if (!environment) {
throw new ReferenceError('Please pass a valid Jest Environment object');
}

const reporter = new JasmineReporter(
globalConfig,
config,
Expand Down Expand Up @@ -85,12 +92,17 @@ async function jasmine2(
if (config.resetMocks) {
runtime.resetAllMocks();

if (config.timers === 'fake') {
if (environment && config.timers === 'fake') {
environment.fakeTimers.useFakeTimers();
}
}
});

// Free references to environment to avoid leaks.
env.afterAll(() => {
environment = null;
Copy link
Member

Choose a reason for hiding this comment

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

Who is holding on to environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The beforeAll closure, through environment.fakeTimers.useFakeTimers().

});

env.addReporter(reporter);

runtime
Expand All @@ -114,7 +126,9 @@ async function jasmine2(

if (config.setupTestFramework && config.setupTestFramework.length) {
config.setupTestFramework.forEach(module => {
require(module)(environment.global);
if (environment) {
require(module)(environment.global);
}
});
}

Expand Down
6 changes: 6 additions & 0 deletions packages/jest-message-util/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ type StackTraceOptions = {
// filter for noisy stack trace lines
const JASMINE_IGNORE = /^\s+at(?:(?:.*?vendor\/|jasmine\-)|\s+jasmine\.buildExpectationResult)/;
const JEST_INTERNALS_IGNORE = /^\s+at.*?jest(-.*?)?(\/|\\)(build|node_modules|packages)(\/|\\)/;

const JEST_NODE_NATIVE_IGNORE = /^\s+at.*?jest-node-native-/;
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>\).*$/;
Expand Down Expand Up @@ -138,6 +140,10 @@ const removeInternalStackEntries = (lines, options: StackTraceOptions) => {
return false;
}

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

if (!STACK_PATH_REGEXP.test(line)) {
return true;
}
Expand Down
43 changes: 31 additions & 12 deletions packages/jest-runner/src/run_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ async function runTestInternal(
RuntimeClass,
>);

const environment = new TestEnvironment(config);
let environment = new TestEnvironment(config);

const leakDetector = config.detectLeaks
? new LeakDetector(environment)
: null;
Expand All @@ -98,15 +99,24 @@ async function runTestInternal(
testConsole = new BufferedConsole();
}

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

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

let runtime = new Runtime(
config,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's time to turn this constructor into something that takes an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we make this change now? This would break Runtime's interface; that's why I thought it was better to keep it as-is.

Copy link
Member

Choose a reason for hiding this comment

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

No, that should be a follow-up but we could do it for Jest 22.

environment,
resolver,
cacheFS,
coverageOptions,
path,
);

const start = Date.now();
await environment.setup();
Expand All @@ -129,19 +139,23 @@ async function runTestInternal(
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({leakDetector, result}));
});
} finally {
await environment.teardown();
if (environment.teardown) {
await environment.teardown();
}

if (runtime.reset) {
await runtime.reset();
}

// Free references to environment to avoid leaks.
cacheFS = null;
environment = null;
runtime = null;
}
}

Expand All @@ -158,6 +172,11 @@ export default async function runTest(
resolver,
);

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

// Resolve leak detector, outside the "runTestInternal" closure.
result.leaks = leakDetector ? leakDetector.isLeaking() : false;

Expand Down
Loading