-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Allow running tests in child processes #17486
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
base: main
Are you sure you want to change the base?
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/60721 |
|
commit: |
05d1543 to
d59e4bf
Compare
f7a8c25 to
0630f73
Compare
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.
Nice!
09eb690 to
0f6d4ed
Compare
| return new Promise((resolve, reject) => { | ||
| if (!(globalThis as any).worker) { | ||
| const workerFile = path.join(dirname, "worker.js"); | ||
| const worker = ((globalThis as any).worker = spawn("fnm", [ |
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 am on the fence with the introduction of a new node version manager for the test purpose, if the fnm website is down the whole CI pipeline will be blocked, too. And it is not very efficient that every babel-8-exec test pipeline has to install a version manager.
Would it be possible to make EXEC_TESTS_NODE independent of the node version manager? For example let EXEC_TESTS_NODE_BIN be the path to the exec test node binary, then we can directly pass it to spawn. Doing so should reduce the runtime overhead of the version manager since we have thousands of exec tests.
The path to the node binary should be easily obtained from any node version manager. With this approach, we may just use the node 6 installed from the setup-node action, it is likely provided in the CI runner toolchain, too.
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.
Doing so should reduce the runtime overhead of the version manager since we have thousands of exec tests.
We only initialize one worker for each child process, and the total number of workers in CI should be 2 or 4. :)
Whether EXEC_TESTS_NODE_BIN is supported depends on whether setup-node caches compressed, which is a bit more complicated if they are compressed.
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.
There seems to be no good way to get the path to other versions of setup-node.
Initially, I also didn't want to rely on a package manager, but unfortunately, tools like esvu don't provide NodeJS support.
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.
What if we used n, which can be installed through npm?
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.
There seems to be no good way to get the path to other versions of
setup-node.
After we checked out the legacy node version via setup-node, could we write the result of whic
8000
h node to the github output? Then we can enable the latest node version and pass that output to the test step.
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.
What if we used n, which can be installed through npm?
Unfortunately it doesn't support Windows.
6095b2b to
3c40069
Compare
|
One issue I've noticed is that this PR contains breaking changes (using |
6685301 to
ea73f0c
Compare
ea73f0c to
017fec7
Compare
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| name: babel-7 | ||
| name: babel-8 |
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.
We should probably do this.
Fixes #1, Fixes #2This is in preparation for Babel 8.
After we clean up our Babel 7 code, we still want to test compatibility with older Node.js versions.
This also makes it easier for local development, as we can now quickly test compatibility with older versions.
Usage:
cross-env EXEC_TESTS_NODE=16 yarn jest