-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Conversation
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:
|
1c17894
to
debfaee
Compare
e09c850
to
73968df
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50072/ |
73968df
to
f99c11b
Compare
This comment has been minimized.
This comment has been minimized.
5e272c3
to
fcc5f37
Compare
This comment has been minimized.
This comment has been minimized.
6b1e33d
to
d9a4733
Compare
This comment has been minimized.
This comment has been minimized.
d9a4733
to
67f4458
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7730462
to
6f9abcd
Compare
39df24a
to
db81f8c
Compare
test/jest-light-runner/src/index.js
Outdated
{ | ||
test, | ||
port: mc.port1, | ||
updateSnapshot: this.#config.updateSnapshot, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
babel/packages/babel-register/test/cache.js
Lines 96 to 106 in 8936501
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(); | |
}); | |
}); | |
}); |
process.nextTick
has racing conditions?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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
@@ -31,7 +31,7 @@ export default function verifyAndAssertMessages( | |||
"../../../babel-eslint-shared-fixtures/config/babel.config.js", | |||
), | |||
}, | |||
...overrideConfig?.parserOptions, | |||
...(overrideConfig && overrideConfig.parserOptions), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this 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!
70fc2e7
to
c7bd3a6
Compare
I squashed the various review commits into the relative main ones to make it easier for the next reviewer. |
🎉 The next steps I'm working on to release an ESM Babel versions are:
|
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 newvm.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) 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:
src
in tests #13978)lib
intest
#13995)require
d files in@babel/helpers
tests #13996)TODO:
Either avoid mocking inMade it work!@babel/register
tests, or make it work