Use babel transform to inline all requires

[cpojer]

Rebased version of #3493

[cpojer]

There is one failing test: the Symbol test, and it shows a real issue (as well as the flaky utils-test that overwrote fs): With this change, all the code within a vm context that comes from Jest (see https://github.com/cpojer/jest/blob/b254715310d800330479aad6996b4bc0716feaa2/packages/jest-jasmine2/src/index.js#L75-L80 and their transitive dependencies) is lazily executed, which means that Jest's code will behave differently based on the test environment at the time Jest code is being required, which can be at any time during a test with this PR applied rather than before running any user code like we do now.

I can think of two ways to fix this:

• We use require for all modules and dependencies that we need to eagerly evaluate that is part of a test context.
• We eagerly evaluate a set amount of modules that we know depends on the test environment.

We should analyze what gets required in a test context and what environment side-effects they depend on. Maybe we could isolate them all and pass them in from the adapter I posted above, like:

runtime.requireInternal('./jest-module.js')({
  Symbol: global.Symbol,
});

I think that may actually be the best choice – it makes it more explicit what side-effects Jest's test code can depend on during a test run.

Also, the speedup for Jest's own test run on this diff is ridiculous. @SimenB, thank you!

[SimenB]

Taking some sort of snapshot of the global environment before running user code might make sense? Or do you want to explicitly pass just the ones needed?

[cpojer]

I think the first step could be to log all modules that are required through requireModuleInternal (without this diff applied so it uses eager imports) and then see what global state/side-effects they rely on. Once we have that analysis, we can make a decision :)

[SimenB]

Sticking a console.log here yields this on master and this on this branch.

It doesn't dedupe dependencies like a proper install would, but still

[thymikee]

So thanks to @SimenB data digging I've been able to find that jest-matcher-utils required from jest-matchers needs to be evaluated eagerly, but I'm still not sure what's the root cause. No time to get through it today.

[SimenB]

Woo, should be ready to merge now!
We can use excludeModules instead of a manual require. I'm not allowed to push to this branch, so I opened up a PR for it: cpojer#2

[SimenB]

I'm still not sure what's the root cause. No time to get through it today.

Is just eagerly evaluating it enough for now? I an alpha could be released it'd be interesting to run that version on e.g. React and see if stuff is still green. And probably at FB, if that's feasible? And other large test suites, of course.

[SimenB]

this should be removed

[aaronabramov]

i messed around with this issue this week. My opinion that we should make everything that is not a test execute in the outer VM scope.

I tried updating jest at fb internally, and i couldn't even count how many ways there are to break the test runner.

one of the examples: if any of the test modify

Array.prototype[Symbol.iterator] = null;

any code inside any files that were requireInternalModule'd that have any runtime for (const a of b) { ... } loops will break.

I spent almost a week trying to catch all of these cases and eventually gave up (one of the test modified Error object, and nothing was failing/throwing. the process just hung forever).

To avoid requiring anything in the inner VM scope i was thinking about two things that we'll need to do:

1. Avoid using singletons in our testing framework/runner/jest-circus and pass framework instances to every test:

const circusContext = jestCurcus.createContext();
// circusContext has {it, describe, setState, getState} and other globals

Object.assign(innerVMScopeGlobal, circusContext);

2. Write an adapter that will take certain globals from innerVM scope and make them available in the outer scope.

We need this for some specific cases. For example, when we throw an Error when a jest-matcher dose not pass, we need to throw an Error that is from the inner context, otherwise we'll have this:

try {
  expect(1).toBe(2);
} catch (error) {
  error instanceOf Error; // false, since the error throw is from the outerVM scope `Error` class.
}

i'm not sure what would be the cleanest way to do this, but i'm thinkig about something like this:

const innerVMScopeGlobals = extractGlobals(innerVMScopeGlobal);
const expectWithInnerVMScopeGlobals = expect.createWithGlobals(innerVMScopeGlobals);
innerVMScopeGlobal.expect = expectWithInnerVMScopeGlobals;
runtime.requireModule(testPath); // require the actual test file, now there's no way for it to modify anything global that will conflict with Jest's code
circusContext.run(); // run the test suite

after the test runs, both circus instance and expect instance are disposed (garbage collected)

[aaronabramov]

i tried to reproduce one of those potential bugs and did it successfully. here's some context:

i added a new matcher that uses for...of loop to jest-matchers:

and a test that uses it:

that passes successfully:

but if we modify the test to overwrite Array.prototype[Symbol.iterator]

the whole thing breaks:

[cpojer]

@aaronabramov: I'm fine with this if you can make it work reliably. Anything that does an instanceof check or needs the child built-in types will need to be able to take the "environment".

[cpojer]

See #4340

[github-actions]

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.