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

Re-inject native Node modules #4970

merged 4 commits into from
Nov 29, 2017

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Nov 27, 2017

This PR improves the sandboxing for native modules. Every time a native require is detected, Jest will:

  • introduce it inside the sandbox by re-evaluating its source code, exposed through process.binding('natives'); which differs from the previous approach where require(moduleName) was done on the main context; and
  • switch the require logic to know it's inside a native module (thus, internal/util will not resolve to the NPM internal module, but to the native Node one).

This kills all memory leaks related to process and native module modification. I have added an integration test with the most common use cases. Executing the test (./jest leak_detection) on a previous commit will fail because of detected leaks.

It also makes the jest test suite leak free itself (tried with ./jest -i --detectLeaks packages).

@SimenB
Copy link
Member

SimenB commented Nov 27, 2017

Does this fix #4630?

@mjesun
Copy link
Contributor Author

mjesun commented Nov 27, 2017

Nope; but I'll work on the child one as soon as I can. I'm very curious to see why this test breaks 🙂

// 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 environment object');
Copy link
Collaborator

Choose a reason for hiding this comment

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

- valid environment object
+ valid Jest Environment

@@ -85,12 +92,21 @@ async function jasmine2(
if (config.resetMocks) {
runtime.resetAllMocks();

if (!environment) {
throw new ReferenceError('Environment should be cleaned at the end');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be more descriptive?

moduleRegistry = this._moduleRegistry;
}

let modulePath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this to the top?

switch (moduleName) {
case 'buffer': // Causes issues when passing buffers to another context.
case 'module': // Calls into native_module, which is not mockable.
case 'os': // Pure native module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we maintain this whitelist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way 😄 . There is no way of knowing that os is a native module, and that module cannot be re-evaluated. Essentially trial/error.

Copy link
Contributor Author

@mjesun mjesun Nov 27, 2017

Choose a reason for hiding this comment

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

Actually, os module should probably be wrapped with an object on top. Once we get this merged I will open a task for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I did a bunch of things in the last commit:

  • I added a test suite that requires all native modules, so this will help us to semi-automate this list.
  • I also deep copy os and v8 modules so that they are sandboxed as well (i.e. if someone modifies them, they won't leak).
  • And finally I fixed the V8 crash for Node 8 (async_hooks can't be re-required).

import createProcessObject from '../create_process_object';

it('creates a process object that looks like the original one', () => {
const fakeProcess = createProcessObject();

// "process" inherits from EventEmitter through the prototype chain.
expect(fakeProcess instanceof EventEmitter).toBe(true);
expect(typeof fakeProcess.on).toBe('function');
Copy link
Collaborator

Choose a reason for hiding this comment

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

So fakeProcess instanceof EventEmitter returns false now? Maybe worth to update the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unfortunate, but yes. Because EventEmitter is re-evaluated now, it is not the parent EventEmitter anymore. In order to get that check right we should build the process object inside the VM context. However, this requires being able to do require('events') which overcomplicates all the current flow.

The comment is still valid, since we check the existence of the on & various methods; but I can clarify it.

@@ -314,6 +316,8 @@ exports[`works with node assert 1`] = `
Message:
Missing expected exception.

at assert:1:1
Copy link
Member

Choose a reason for hiding this comment

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

we don't want this in the stack at all, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these modules are now re-injected, they will appear in the stack. I don't think it's critical, though.

Copy link
Member

Choose a reason for hiding this comment

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

it's not useful though, so why not remove it? ref #4695

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you decide what comes from an internal and what not? Notice that now internals are not internals anymore, because we re-evaluate them 😉

Copy link
Member

@SimenB SimenB Nov 27, 2017

Choose a reason for hiding this comment

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

IMO internal === jest or node core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but I'm unsure on how can we remove an arbitrary number of stack frames from the stack. How are we doing that now? I can maybe adapt my filenames to the existing way.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah these guys have to go. We need a way to annotate node libs so they don't come up in stack traces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB this looks promising! I can inject native modules with native on the file name, and I think I can get rid of them.

case 'os': // Pure native module.
case 'v8': // Contains invalid references.
// $FlowFixMe: dynamic require needed.
localModule.exports = deepCyclicCopy(require(moduleName));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it have a noticeable impact on perf? And are we sure there's nothing to blacklist there?

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 checked both; v8 is just a set of functions, and os has some additional properties (os.constants), but it simple enough to be copied.

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 🙂

@@ -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

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?

@SimenB
Copy link
Member

SimenB commented Nov 27, 2017

Failing on appveyor 🙁

@mjesun
Copy link
Contributor Author

mjesun commented Nov 27, 2017

Yes, I just debugged it. What is happening is that jasmine-reporters/src/appveyor_reporter.js is injected inside the VM context (see here), but it is called after the test finishes. By then, the environment has been cleaned and the reporter fails.

To be precise, what fails is the execution of any require within the VM context after the test suite has finished. The appveyor_reporter.js has an inline require to http, which is the one breaking.

IMO this reporters should be injected outside the VM context (i.e. on the main context); but I wonder if doing so can create other undesirable side effects. cc @cpojer, @SimenB, @thymikee.

@thymikee
Copy link
Collaborator

I guess it makes sense to create another hook which works outside of VM context and put jasmine reporters there.

@cpojer
Copy link
Member

cpojer commented Nov 28, 2017

Yeah, we'll need to fix that issue first before we can merge this PR.

@@ -14,7 +14,8 @@ const runJest = require('../runJest');

const dir = path.resolve(__dirname, '../failures');

const normalizeDots = text => text.replace(/\.{1,}$/gm, '.');
const normalize = text =>
text.replace(/\.{1,}$/gm, '.').replace(/\bassert:\d+:\d+/g, 'assert:1:1');
Copy link
Member

Choose a reason for hiding this comment

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

what is this change for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert can change from one version to another, and since now we re-inject the native module, line numbers can change, thus not making the snapshot to match. This ensures that line numbers are transformed into something consistent across Node versions.

Since I'm going to try make them disappear, that doesn't make sense anymore.

*/

it('requires all native modules to check they all work', () => {
Object.keys(process.binding('natives'))
Copy link
Member

Choose a reason for hiding this comment

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

If in a version of node process.binding changes to not return anything, this test will still pass but the sandboxing is broken. Can we change this to make sure it requires the right modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything would be broken then, because we rely on process.binding already (e.g. is_builtin_module from jest-resolve 😄

What I'm going to do is to check that the length is greater than the current amount of modules for Node 6, which should be good signaling.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'm fine with that. If they ever remove core modules our test will fail, but that's fine and something we'd want to know about anyway.

@@ -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.

@@ -85,12 +92,24 @@ async function jasmine2(
if (config.resetMocks) {
runtime.resetAllMocks();

if (!environment) {
throw new ReferenceError(
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way that this can actually happen? How about just doing `if (environment && config.timers === 'fake') { … } ?

if (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().

@@ -142,6 +143,11 @@ async function runTestInternal(
});
} finally {
await environment.teardown();
await runtime.teardown();

// Free references to enviroment to avoid leaks.
Copy link
Member

Choose a reason for hiding this comment

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

environment.

import createProcessObject from '../create_process_object';

it('creates a process object that looks like the original one', () => {
const fakeProcess = createProcessObject();

// "process" inherits from EventEmitter through the prototype chain.
expect(fakeProcess instanceof EventEmitter).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

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

Hah, I think this is a bug actually. You are creating the process object with EventEmitter from the parent but it should use runtime._requireNativeModule('events') so that this test still passes. Changing this test isn't the right fix here imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly; I explained that to @thymikee before. The issue here is that in order to get the child EventEmitter I need to execute in the context require('events').EventEmitter. Unfortunately at this point neither the context, nor the require implementation are ready; so it's too complex to add the change in this PR.

Since I just re-did the process object about a week ago, and before it did not inherit from EventEmitter either, I think we are more or less safe to do this.

However, at the core of the issue, you are both right, the check should pass 🙂

Copy link
Member

Choose a reason for hiding this comment

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

In that case let's change where we are initializing process then? It seems like that should happen later or lazily. You could make process a lazy getter that, when first accessed, overwrites itself by doing global.process = process. What do you think about that? We just need to make sure there is nothing in events or its dependencies that depends on process which would produce a circular dependency.

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.

I tried making this work, but I ended up with a V8 crash (see below). I think we should revisit it later; but it should be kept out of the scope of this PR.

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node::Abort() [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 3: v8::V8::ToLocalEmpty() [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 4: node::(anonymous namespace)::ContextifyScript::RunInContext(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 6: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 7: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 8: 0x2fbc95e0463d
 9: 0x2fbc95ef4048

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 file a task to fix this?

const isNativeModule =
moduleName &&
((options && options.isNativeModule) ||
this._resolver.isCoreModule(moduleName));
Copy link
Member

Choose a reason for hiding this comment

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

woah.. pretty :P

if (isNativeModule) {
moduleRegistry = this._nativeModuleRegistry;
} else if (options && options.isInternalModule) {
moduleRegistry = this._internalModuleRegistry;
Copy link
Member

Choose a reason for hiding this comment

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

The internal module registry is used in Jest for Jest's own core modules like pretty-format or chalk. Any reason you can't reuse that one? Why do you need a third one?

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.

Because of clashes. Once you start requiring inside a Jest internal, all the child modules are required inside Jest are internal modules as well (which is fine, because you prevent clashes between test and Jest code).

If Jest requires internal/util, internal refers to NPM's module; but if you require it from the Native util module, it refers to the internal file; so you need a separate require implementation (as well as a separate registry) to avoid the conflict.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that makes sense.

Object.defineProperty(localModule, 'require', {
value: this._createRequireImplementation(filename, options),
});

const fileSource = isNativeModule
? // $FlowFixMe: process.binding exists.
process.binding('natives')[filename]
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 call fast or slow?

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.

Really fast. The following code:

console.time('binding');

for (let i = 0; i < 100000; i++) {
  global.fsSourceCode = process.binding('natives').fs;
}

console.timeEnd('binding');

Reports an average of 16ms per 100,000 calls.

moduleRegistry: ModuleRegistry,
from: Path,
) {
switch (moduleName) {
Copy link
Member

Choose a reason for hiding this comment

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

I like it.

// use native resolution. Native resolution differs from standard
// resolution on how requires are resolved (e.g. the "util" module
// requires "internal/util", which is not the NPM "internal" module,
// but something else).
Copy link
Member

Choose a reason for hiding this comment

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

For this comment, how about:

Use a special resolution when requiring node's internal modules. For example the "util" module requires "internal/util" which isn't a physical directory.

It's shorter and conveys the same message.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

This is great work, I'm so excited for better sandboxing and fewer memory leaks in Jest. Can you work on some of the inline comments that I left?

Before merging this, here are some more high-level questions that would be great to answer:

  • Can you show results of --logHeapUsage -i before and after this change?
  • How does this affect performance of Jest?
  • Can we reduce this to only two module registries (internal and user) vs. three?

@@ -33,7 +33,7 @@ export type Options = {|
collectCoverage: boolean,
collectCoverageFrom: Array<Glob>,
collectCoverageOnlyFrom: ?{[key: string]: boolean, __proto__: null},
isCoreModule?: boolean,
isNativeModule?: boolean,
Copy link
Member

Choose a reason for hiding this comment

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

why change the name? "native module" in my mind is native addons (https://nodejs.org/api/addons.html), not node core modules.

I suppose it's not public API, but still

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already use a isCoreModule in packages/jest-resolve/src/index.js, which does not quite match the meaning of isNativeModule. When I noticed it I decided to rename all the new additions.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 28, 2017

  • Can you show results of --logHeapUsage -i before and after this change?
    Yes, heap consumption grows by approx. a 30%. This is expected, since heap measurement is done before we clean the environment; and now all native modules are re-evaluated.

  • How does this affect performance of Jest?
    Time wise, I could see some inconsistent performance regression of around 10%. Second runs with the change take longer (measured on the same machine). Maybe @thymikee could also run against his test suite, so we get other numbers 🙂

  • Can we reduce this to only two module registries (internal and user) vs. three?
    Unfortunately no 😞, Jest's and Node's internal/util mean different things, so we have to address that to avoid key collisions).

@cpojer
Copy link
Member

cpojer commented Nov 28, 2017

Huh, can you try to run again with node --expose-gc? That will run the garbage collector before logging heap usage. A 30% increase in memory usage goes counter to what this diff is aiming to solve.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 28, 2017

As explained, it is expected based on where the memory is being measured.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 28, 2017

Added a new commit (also rebased over master); modifying where memory is measured. Memory consumption went down from 156 to 143 MB (i.e ~10% less memory 🙂) while measured in Jest repository.

However, the memory still grows within the first tests, up until it stabilizes around 143 MB. Some simple snapshots between tests (running with -i) show that part of the increase comes from objects storing compiled source code (i.e. cacheFS), but there are many others.

@codecov-io
Copy link

codecov-io commented Nov 28, 2017

Codecov Report

Merging #4970 into master will increase coverage by 0.01%.
The diff coverage is 61.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4970      +/-   ##
==========================================
+ Coverage   60.25%   60.27%   +0.01%     
==========================================
  Files         198      198              
  Lines        6623     6659      +36     
  Branches        4        3       -1     
==========================================
+ Hits         3991     4014      +23     
- Misses       2632     2645      +13
Impacted Files Coverage Δ
packages/jest-runner/src/run_test.js 1.96% <0%> (-0.27%) ⬇️
packages/jest-jasmine2/src/index.js 5.47% <0%> (-0.41%) ⬇️
packages/jest-runtime/src/script_transformer.js 86.5% <100%> (ø) ⬆️
packages/jest-util/src/index.js 100% <100%> (+11.76%) ⬆️
packages/jest-util/src/install_common_globals.js 100% <100%> (ø) ⬆️
packages/jest-message-util/src/index.js 86.13% <66.66%> (-0.6%) ⬇️
packages/jest-runtime/src/index.js 74.83% <86.04%> (+1.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0338a82...cf83dff. Read the comment docs.

};

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.

// Delay the resolution to allow log messages to be output.
return new Promise(resolve => {
setImmediate(() => resolve({leakDetector, result}));
});
} finally {
await environment.teardown();
environment.teardown && (await environment.teardown());
runtime.teardown && (await runtime.teardown());
Copy link
Member

Choose a reason for hiding this comment

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

No way. Can your turn these into if statements?

@@ -130,6 +134,8 @@ class Runtime {
this._mockRegistry = Object.create(null);
this._moduleMocker = this._environment.moduleMocker;
this._moduleRegistry = Object.create(null);
this._nativeModuleRegistry = Object.create(null);
this._path = path || '<unknown test file>';
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 change this to "unknown entry point"? jest-runtime can be used as standalone and doesn't have to run test files.

@@ -267,40 +273,68 @@ class Runtime {
return cliOptions;
}

teardown() {
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 this reset and call it in the constructor also for less duplication?

if (!this._environment) {
throw new Error(
`A module was required after the test suite ${this._path} finished.\n` +
`This usually means that you forgot to clean or mock an async operation`,
Copy link
Member

Choose a reason for hiding this comment

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

nit: "In most cases this is because an async operation wasn't cleaned up or mocked properly."

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Awesome. Please rebase + remove the merged PR + fix some of the nits before merging. I'm still awaiting the memory difference of this on the two large codebases internally, mind sending me that within FB? :)

@cpojer cpojer merged commit ef55e89 into jestjs:master Nov 29, 2017
@SimenB
Copy link
Member

SimenB commented Nov 29, 2017

This should probably have an entry in the changelog

@mjesun mjesun deleted the re-inject-native-modules branch December 4, 2017 11:15
mjesun added a commit that referenced this pull request Dec 4, 2017
mjesun added a commit that referenced this pull request Dec 4, 2017
cpojer pushed a commit that referenced this pull request Dec 4, 2017
* Revert "docs: update expect.anything() example (#5007)"

This reverts commit ea3fabc.

* Revert "Emphasise required return (#4999)"

This reverts commit 4f1113c.

* Revert "Makes NPM a link like Yarn is (#4998)"

This reverts commit aa4f09d.

* Revert "Update Timeout error message to `jest.timeout` and display current timeout value (#4990)"

This reverts commit 08f8394.

* Revert "fix: jest-util should not depend on jest-mock (#4992)"

This reverts commit 4e2f41f.

* Revert "Update Troubleshooting.md (#4988)"

This reverts commit 6414f28.

* Revert "Revert "Add the Yammer logo to the 'who is using this' section of the website." (#4987)"

This reverts commit 91b104f.

* Revert "Make "weak" optional dependency and check it at runtime (#4984)"

This reverts commit e00529d.

* Revert "Re-inject native Node modules (#4970)"

This reverts commit ef55e89.
@mjesun mjesun mentioned this pull request Jan 2, 2018
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants