esm: improve performance & tidy tests by JakobJingleheimer · Pull Request #43784 · nodejs/node

JakobJingleheimer · 2022-07-11T19:07:37Z

This standardises the setup of ESM tests and improves performance by parallelising case runs using a promise wrapper around async spawn.

I originally started out using chaining for parallelisation, but switched to node:test to avoid a bajillion .then()s. As .then()s, the performance improvement was obvious.

I'm not sure how to get a truly apples-to-apples comparison: ./tools/test.py -J es-module doesn't benefit from node:test's parallelisation. I manually checked one of the largest tests, and the improvement is quite stark:

$> time ./node ./test/es-module/test-esm-loader-chaining.mjs

When	Duration
Before	`0m1.168s`
After	`0m0.442s`

Note to reviewers: enabling Hide whitespace reduces the diff by about a third.

cc @nodejs/loaders

aduh95

I would suggest to not refactor all the tests to ESM, that makes the PR needlessly harder to review, instead you can use dynamic import.

JakobJingleheimer · 2022-07-11T21:29:39Z

@aduh95 I'm also hoping to standardise how the tests are written. Making them all ESM instead of a mix avoids someone wondering why some are one and others are another (there's no real reason—the CJS ones are just older).

How about I tackle that in a separate PR? I think if this lands first, then the CJS → ESM PR will have nothing to be reviewed.

PS My phone was sleeping when your comment came in, so I didn't see your message for the commit I just pushed.

JakobJingleheimer · 2022-07-24T22:28:08Z

There are some lint errors in the commonjs test files that I don't know how to fix:

node-core/require-common-first Mandatory module "common" must be loaded before any other modules

The files are already requiring common at the very top (immediately below 'use strict';),

node-core/async-iife-no-unused-result The result of an immediately-invoked async function needs to be used (e.g. with .then(common.mustCall()))

It's complaining about the (async () => {})() wrapper I added to the CJS tests in order to dynamic import the new spawnAsPromised helper.

aduh95 · 2022-07-24T22:50:57Z

The files are already requiring common at the very top

It expects require('../common'), not require('../common/index.js').

It's complaining about the (async () => {})() wrapper I added to the CJS tests in order to dynamic import the new spawnAsPromised helper.

As the error message says, you need to use its result (i.e. add .then(common.mustCall())).

JakobJingleheimer · 2022-07-25T08:04:03Z

This test is redundant to test/es-module/test-esm-unknown-or-no-extension.js

aduh95 · 2022-07-25T22:07:05Z

Wouldn't it be easier to refactor this to ESM to get TLA/static imports?

I did and you said to not, so I backed it out lol

The issue is that GitHub UI doesn't give an option to ignore the whitespace/indentation changes (I think?), if the only diff was adding the await import that would be much easier to review to keep it as CJS. As it is now, I doubt it makes much difference. Oh well, let's keep it as is, it can always be re-refactored later.

The issue is that GitHub UI doesn’t give an option to ignore the whitespace/indentation changes (I think?)
Add ?w=1 to the end of the URL. Or use one of the other methods: https://stackoverflow.com/a/37145215/223225.

"Hide whitespace" via the GitHub UI (on the file view screen):
Damn, sorry I had added a note to my PR description about hiding whitespace changes (it reduces the diff by 36%). I dunno what happened to that note 😖

🤦‍♂️ foiled by my own cleverness: who can spot the problem

Ah, that explains why I was never pinged about this 😄

GeoffreyBooth

This is too much to review with all the async function wrappers because of spawn. Please refactor that so that it has CommonJS and ESM versions available so that you can import it into all these CommonJS test files without needing to turn them into async functions, then I’ll re-review once the diff has shrunk.

targos · 2022-08-02T17:29:37Z

@GeoffreyBooth I assume you would like this to be included in a future v16.x release. The correct label for this is lts-watch-v16.x. Releasers/backporters will add backport-requested-v16.x if they have trouble cherry-picking the commit when a release is being prepared.

GeoffreyBooth · 2022-08-02T17:31:49Z

@targos Sorry, thanks for the correction 😄 FYI this fixes a bug introduced by an earlier PR that hasn’t been released on 16 yet; see https://openjs-foundation.slack.com/archives/C01810KG1EF/p1659460515402819?thread_ts=1658853609.909249&cid=C01810KG1EF. @cspotcode will add a comment so that hopefully this fix goes out in the same release as the bug (so that the bug never actually gets released in 16.x).

targos · 2022-08-02T17:35:23Z

@GeoffreyBooth There is a v16.x release proposal at #44098. This commit would normally not be eligible for inclusion in LTS because it's not been in a Current release yet. If there is a good reason to include it, please add a comment to #44098

GeoffreyBooth · 2022-08-02T17:39:09Z

I think even better would be to just pull the commit that introduces the bug from the current release proposal, if it’s in there, and then that commit could go out in the same release as this one according to the usual schedule. (Waiting on which commit includes the bug.)

JakobJingleheimer · 2022-08-02T17:43:02Z

Sorry, what bug does this fix?

cspotcode · 2022-08-02T17:44:52Z

I think there's a bit of confusion, this PR doesn't fix it, it's the thing you said you'd address in a PR after this PR lands. If everyone's ok waiting a few hours for me to post all the details, I can do that later today. Won't be able to right this moment.

JakobJingleheimer · 2022-08-02T17:46:59Z

Ah, yes indeed. I shall put that bugfix together tomorrow (it's a simple fix). I wanted to wait for this to land to avoid a merge conflict in with the new test case for that bug.

PR-URL: #43784 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs/node#43784

* chore: bump node in DEPS to v16.17.0 * chore: fixup asar patch * lib: use null-prototype objects for property descriptors nodejs/node#43270 * src: make SecureContext fields private nodejs/node#43173 * crypto: remove Node.js-specific webcrypto extensions nodejs/node#43310 * test: refactor to top-level await nodejs/node#43500 * deps: cherry-pick two libuv fixesnodejs/node#43950 * src: slim down env-inl.h nodejs/node#43745 * util: add AggregateError.prototype.errors to inspect output nodejs/node#43646 * esm: improve performance & tidy tests nodejs/node#43784 * src: NodeArrayBufferAllocator delegates to v8's allocator nodejs/node#43594 * chore: update patch indices * chore: update filenames * src: refactor IsSupportedAuthenticatedMode nodejs/node#42368 * src: add --openssl-legacy-provider option nodejs/node#40478 * lib,src: add source map support for global eval nodejs/node#43428 * trace_events: trace net connect event nodejs/node#43903 * deps: update ICU to 71.1 nodejs/node#42655 This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093 * lib: give names to promisified exists() and question() nodejs/node#43218 * crypto: add CFRG curves to Web Crypto API nodejs/node#42507 * src: fix memory leak for v8.serialize nodejs/node#42695 This test does not work for Electron as they do not use V8's ArrayBufferAllocator. * buffer: fix atob input validation nodejs/node#42539 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

PR-URL: nodejs/node#43784 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

* chore: bump node in DEPS to v16.17.0 * chore: fixup asar patch * lib: use null-prototype objects for property descriptors nodejs/node#43270 * src: make SecureContext fields private nodejs/node#43173 * crypto: remove Node.js-specific webcrypto extensions nodejs/node#43310 * test: refactor to top-level await nodejs/node#43500 * deps: cherry-pick two libuv fixesnodejs/node#43950 * src: slim down env-inl.h nodejs/node#43745 * util: add AggregateError.prototype.errors to inspect output nodejs/node#43646 * esm: improve performance & tidy tests nodejs/node#43784 * src: NodeArrayBufferAllocator delegates to v8's allocator nodejs/node#43594 * chore: update patch indices * chore: update filenames * src: refactor IsSupportedAuthenticatedMode nodejs/node#42368 * src: add --openssl-legacy-provider option nodejs/node#40478 * lib,src: add source map support for global eval nodejs/node#43428 * trace_events: trace net connect event nodejs/node#43903 * deps: update ICU to 71.1 nodejs/node#42655 This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093 * lib: give names to promisified exists() and question() nodejs/node#43218 * crypto: add CFRG curves to Web Crypto API nodejs/node#42507 * src: fix memory leak for v8.serialize nodejs/node#42695 This test does not work for Electron as they do not use V8's ArrayBufferAllocator. * buffer: fix atob input validation nodejs/node#42539 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

JakobJingleheimer added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Jul 11, 2022

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 11, 2022

aduh95 reviewed Jul 11, 2022
View reviewed changes

Comment threadtest/es-module/test-cjs-esm-warn.mjs Outdated

JakobJingleheimer mentioned this pull request Jul 14, 2022
test_runner: fix it concurrency #43757
Merged

JakobJingleheimer mentioned this pull request Jul 24, 2022
esm: add --import flag #43942
Merged
1 task

JakobJingleheimer force-pushed the esm/tidy-esm-tests branch 2 times, most recently from efb8000 to f3a2e1cCompare July 24, 2022 22:22

JakobJingleheimer marked this pull request as ready for review July 24, 2022 22:22

JakobJingleheimer requested a review from GeoffreyBooth July 25, 2022 07:56

aduh95 reviewed Jul 25, 2022
View reviewed changes

Comment threadtest/es-module/test-cjs-esm-warn.js Outdated

JakobJingleheimer commented Jul 25, 2022
View reviewed changes

JakobJingleheimer requested a review from aduh95 July 25, 2022 21:57

aduh95 reviewed Jul 25, 2022
View reviewed changes

GeoffreyBooth reviewed Jul 26, 2022
View reviewed changes

Comment threadtest/es-module/helper.spawnAsPromised.mjs Outdated
Comment threadtest/es-module/test-esm-cjs-load-error-note.mjs Outdated

JakobJingleheimer added 6 commits July 26, 2022 20:22

esm: tidy tests
03d1f1d

switch to node:test
24157ab

move spawnAsPromised to test/common as spawnPromisified
a53214b

tidy shouldIncludeNote
e9fcef0

replaceAll regex → string
41980ee

REPL done()
d7a2541

JakobJingleheimer force-pushed the esm/tidy-esm-tests branch from 98aebc9 to d7a2541Compare July 26, 2022 18:32

MoLow reviewed Jul 26, 2022
View reviewed changes

Comment threadtest/es-module/test-esm-cjs-load-error-note.mjs Outdated

MoLow reviewed Jul 26, 2022
View reviewed changes

Comment threadtest/common/index.js

MoLow reviewed Jul 26, 2022
View reviewed changes

Comment threadtest/common/index.js

GeoffreyBooth approved these changes Jul 27, 2022
View reviewed changes

Comment threadtest/common/index.js Outdated

JakobJingleheimer deleted the esm/tidy-esm-tests branch July 31, 2022 18:36

aduh95 removed the test_runner Issues and PRs related to the test runner subsystem. label Jul 31, 2022

This was referenced Aug 1, 2022
CI Reliability 2022-08-01 nodejs/reliability#334
Open
CI Reliability 2022-08-02 nodejs/reliability#335
Open

GeoffreyBooth added the backport-requested-v16.x label Aug 2, 2022

targos added lts-watch-v16.x and removed backport-requested-v16.x labels Aug 2, 2022

github-actionsBot mentioned this pull request Aug 3, 2022
CI Reliability 2022-08-03 nodejs/reliability#336
Open
15 tasks

targos removed the lts-watch-v16.x label Aug 6, 2022

targos pushed a commit that referenced this pull request Aug 6, 2022
test: refactor ESM tests to improve performance
e6db809
PR-URL: #43784 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos mentioned this pull request Aug 6, 2022
v16.17.0 release proposal #44098
Merged

danielleadams pushed a commit that referenced this pull request Aug 16, 2022
test: refactor ESM tests to improve performance
cbc1189
PR-URL: #43784 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

danielleadams mentioned this pull request Aug 16, 2022
v18.8.0 release proposal #44246
Closed

ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
test: refactor ESM tests to improve performance
b19564b
PR-URL: #43784 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

ruyadorno mentioned this pull request Aug 23, 2022
v18.8.0 release proposal #44353
Merged

codebytere added a commit to electron/electron that referenced this pull request Aug 23, 2022
esm: improve performance & tidy tests
8b3177b
nodejs/node#43784

codebytere added a commit to electron/electron that referenced this pull request Aug 23, 2022
esm: improve performance & tidy tests
16f2d2f
nodejs/node#43784

codebytere added a commit to electron/electron that referenced this pull request Aug 24, 2022
esm: improve performance & tidy tests
4d7a56e
nodejs/node#43784

codebytere added a commit to electron/electron that referenced this pull request Aug 25, 2022
esm: improve performance & tidy tests
e1bbdd2
nodejs/node#43784

guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
test: refactor ESM tests to improve performance
7ed24ae
PR-URL: nodejs/node#43784 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Uh oh!

Uh oh!

Conversation

JakobJingleheimer commented Jul 11, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JakobJingleheimer commented Jul 11, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakobJingleheimer commented Jul 24, 2022

Uh oh!

aduh95 commented Jul 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

targos commented Aug 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoffreyBooth commented Aug 2, 2022

Uh oh!

targos commented Aug 2, 2022

Uh oh!

GeoffreyBooth commented Aug 2, 2022

Uh oh!

JakobJingleheimer commented Aug 2, 2022

Uh oh!

cspotcode commented Aug 2, 2022

Uh oh!

JakobJingleheimer commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JakobJingleheimer commented Jul 11, 2022•
edited
Loading

JakobJingleheimer commented Jul 11, 2022•
edited
Loading

aduh95 commented Jul 24, 2022•
edited
Loading

targos commented Aug 2, 2022•
edited
Loading