Use yarn nodeLinker: node-modules to make tests faster #652
Conversation
This change almost halves the execution time of `main.test.ts` (from 57s to 35s on my machine using NOCK_ENV=replay). The end-to-end tests run corepack in a new node process about 150 times. With the pnp linker, node is asked to require `.pnp.cjs` on every startup which significantly contributes to test time.
|
Yeah, for the Node 22 test on Ubuntu total job time drops by 40s to 1min. And on Windows it drops by 3:40 to 8min. |
| - "main": "lib/advanced/index", | ||
| + "main": "lib/advanced/index.js", |
There was a problem hiding this comment.
With the original patch I get the following error:
Error: Directory import '.../corepack/node_modules/clipanion/lib/platform' is not supported resolving ES modules imported from ...corepack/node_modules/clipanion/lib/advanced/Cli.mjs
In fact, Cli.mjs uses import ... from './platform' but Node requires relative import specifiers to always point to files. I guess this worked before because yarn’s resolver is implemented differently and supports relative package imports.
This comment was marked as resolved.
This comment was marked as resolved.
|
Will you merge this? It seems like a good improvement. |
|
I would prefer to keep PnP, if only to have a Node.js project dogfooding the default yarn install strategy. The perf impact is unfortunate, but 30% doesn't feel too dramatic an increase that this is a must-do. |
This change almost halves the execution time of
main.test.ts(from 57s to 35s on my machine using NOCK_ENV=replay). I expect similar results on CI.The end-to-end tests run corepack in a new node process about 150 times. With the pnp linker, node is asked to require
.pnp.cjson every startup which significantly contributes to test time.As an alternative, I tried removing
--require=.pnp.cjsfromNODE_OPTIONSinrunCli. After all, we’re running the bundled version of corepack. But this did not work becausetests/recordRequests.jsis also required which in turn needsbetter-sqlite3.