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

Run tests in a native Node.js ESM environment #13966

Merged
merged 9 commits into from Dec 3, 2021

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Nov 14, 2021

Q                       A
Any Dependency Changes?
License MIT

With this PR I'm trying to run our tests using the native Node.js ESM implementation, rather than Jest's one.

I created a custom jest runner still uses jest-circus/jest-each/jest-mock/jest-expect/jest-snapshot to provide a Jest-like environment, but runs them (and the tests) in the current context rather than spawning a new vm.Context. By doing so, we don't need to wait for Node.js to stabilize the ESM vm api so that Jest can properly use it (it's currently behind an experimental Node.js flag, and Jest's support isn't complete yet also because of some V8 bugs). Also, this approach is faster (2x on my machine).

However, there are some downsides:

  1. We don't have the strong isolation that Jest gives anymore, so one test could set some global state which would be accessible by other tests
  2. We can only mock files manually

(1) doesn't affect us since Babel doesn't rely on global state. I am working on a solution for (2).

This PR contains many changes to align our tests with an ESM-compatible implementation, but I'll extract them to separate PRs:

TODO:

  • Either avoid mocking in @babel/register tests, or make it work Made it work!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 14, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ac23359:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 17, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50072/

@nicolo-ribaudo

This comment has been minimized.

@nicolo-ribaudo

This comment has been minimized.

@nicolo-ribaudo nicolo-ribaudo force-pushed the jest-light branch 2 times, most recently from 6b1e33d to d9a4733 Compare November 18, 2021 21:35
@nicolo-ribaudo

This comment has been minimized.

@JLHwung

This comment has been minimized.

@nicolo-ribaudo

This comment has been minimized.

{
test,
port: mc.port1,
updateSnapshot: this.#config.updateSnapshot,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: updateSnapshot can be hoisted.


const piscina = new Piscina({
filename: new URL("./worker-runner.js", import.meta.url).href,
maxThreads: os.cpus().length / 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Test on my local machine is faster when I use the default maxThreads (cpus().length * 1.5) (29s vs 26s), but

it("should create the cache after dirty", () => {
load();
setDirty();
return new Promise(resolve => {
process.nextTick(() => {
expect(fs.existsSync(testCacheFilename)).toBe(true);
expect(get()).toEqual({});
resolve();
});
});
});
is randomly failing. Guess process.nextTick has racing conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see that failure 🤔 Also, all the tests in a single file are run serially, so it shouldn't have any race conditions.

Btw, for me it takes way longer 😬 33s with / 2 vs 50s with * 1.5.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is an

error output
    - Expected  -  1
    + Received  + 45

    - Object {}
    + Object {
    +   "{\"assumptions\":{},\"sourceRoot\":\"/path/to/babel/packages/babel-register/test/fixtures/babelrc/\",\"cwd\":\"/path/to/babel/packages/babel-register/test/fixtures/babelrc\",\"babelrc\":false,\"sourceMaps\":false,\"caller\":{\"name\":\"@babel/register\"},\"filename\":\"/path/to/babel/packages/babel-register/test/fixtures/babelrc/es2015.js\",\"targets\":{},\"cloneInputAst\":true,\"configFile\":false,\"browserslistConfigFile\":false,\"passPerPreset\":false,\"envName\":\"undefined\",\"root\":\"/path/to/babel/packages/babel-register/test/fixtures/babelrc\",\"rootMode\":\"root\",\"plugins\":[],\"presets\":[]}:7.16.0:undefined": Object {
    +     "ast": null,
    +     "code": "const a = 1;",
    +     "map": null,
    +     "metadata": Object {},
    +     "mtime": 1637340521216,
    +     "options": Object {
    +       "assumptions": Object {},
    +       "ast": false,
    +       "babelrc": false,
    +       "browserslistConfigFile": false,
    +       "caller": Object {
    +         "name": "@babel/register",
    +       },
    +       "cloneInputAst": true,
    +       "configFile": false,
    +       "cwd": "/path/to/babel/packages/babel-register/test/fixtures/babelrc",
    +       "envName": "undefined",
    +       "filename": "/path/to/babel/packages/babel-register/test/fixtures/babelrc/es2015.js",
    +       "generatorOpts": Object {
    +         "comments": true,
    +         "compact": "auto",
    +         "filename": "/path/to/babel/packages/babel-register/test/fixtures/babelrc/es2015.js",
    +         "sourceFileName": "es2015.js",
    +         "sourceMaps": false,
    +         "sourceRoot": "/path/to/babel/packages/babel-register/test/fixtures/babelrc/",
    +       },
    +       "parserOpts": Object {
    +         "plugins": Array [],
    +         "sourceFileName": "/path/to/babel/packages/babel-register/test/fixtures/babelrc/es2015.js",
    +         "sourceType": "module",
    +       },
    +       "passPerPreset": false,
    +       "plugins": Array [],
    +       "presets": Array [],
    +       "root": "/path/to/babel/packages/babel-register/test/fixtures/babelrc",
    +       "rootMode": "root",
    +       "sourceMaps": false,
    +       "sourceRoot": "/path/to/babel/packages/babel-register/test/fixtures/babelrc/",
    +       "targets": Object {},
    +     },
    +     "sourceType": "module",
    +   },
    + }

      at file:/path/to/babel/packages/babel-register/test/cache.js:121:25

The test cache seems to be created by another test test/index.js but then read by test/cache.js, however I can't reproduce it consistently.

Copy link
Contributor

Choose a reason for hiding this comment

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

33s with / 2 vs 50s with * 1.5.

Hmm, interesting. I think it is related to the Intel hyper-threading, which doubles the available processor count... Anyway the current setting is good enough for me, don't bother change that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you check if c385768 solves the race condition?


import "./global-setup.js";

/** @typedef {import("@jest/test-result").Test} Test */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the typedef used?


/**
*
* @param {*} stats
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we supplement the shape of stats here?

numPassingTests: stats.passes,
numPendingTests: stats.pending,
perfStats: {
end: +new Date(stats.end),
Copy link
Contributor

@JLHwung JLHwung Nov 30, 2021

Choose a reason for hiding this comment

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

end and start seem to be not yet implemented. They are always undefined on my local machine.

We can implement using the performance API available since Node 8.5.

The perfStats also includes slow and runtime, which can be accordingly implemented

https://github.com/facebook/jest/blob/3a85065fe5604655e1337ffc1631f9999722c821/packages/jest-runner/src/runTest.ts#L293-L299

@@ -31,7 +31,7 @@ export default function verifyAndAssertMessages(
"../../../babel-eslint-shared-fixtures/config/babel.config.js",
),
},
...overrideConfig?.parserOptions,
...(overrideConfig && overrideConfig.parserOptions),
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we skip jest script transformer, should we add https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unsupported-features/es-syntax.md to non-fixture test files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh thanks! I was looking for this rule but didn't remember the plugin name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have enabled it, but support for ?? and ?. hasn't been released yet (so it doesn't warn about them).

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Awesome work! This PR replaces the default jest-runner by jest-light-runner, which removes the test environment (an isolated VM context) and the script transforms (babel-jest). Although the test runner is backed by worker pools slower than the process-based approach used in jest-worker, we still see significant CI runtime improvement: This branch on Node 16(2m3s) vs main on Node 16(4m57s). Bravo!

@nicolo-ribaudo
Copy link
Member Author

I squashed the various review commits into the relative main ones to make it easier for the next reviewer.

@nicolo-ribaudo
Copy link
Member Author

🎉

The next steps I'm working on to release an ESM Babel versions are:

  • Make @babel/register work
  • Use exports in package.json files

@nicolo-ribaudo nicolo-ribaudo merged commit 2d989a9 into babel:main Dec 3, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the jest-light branch December 3, 2021 14:32
@nicolo-ribaudo nicolo-ribaudo added this to In progress in Move to native ES modules via automation Jan 14, 2022
@nicolo-ribaudo nicolo-ribaudo moved this from In progress to Done in Move to native ES modules Jan 14, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Development

Successfully merging this pull request may close these issues.

None yet

4 participants