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

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

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.

@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.

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.

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())).

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

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 😄

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.

@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.

@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).

@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

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.)

Sorry, what bug does this fix?

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.

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 fixes nodejs/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 fixes nodejs/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 thread test/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 f3a2e1c Compare 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 thread test/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 thread test/es-module/helper.spawnAsPromised.mjs Outdated

Comment thread test/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 d7a2541 Compare July 26, 2022 18:32

MoLow reviewed Jul 26, 2022

View reviewed changes

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

MoLow reviewed Jul 26, 2022

View reviewed changes

Comment thread test/common/index.js

MoLow reviewed Jul 26, 2022

View reviewed changes

Comment thread test/common/index.js

GeoffreyBooth approved these changes Jul 27, 2022

View reviewed changes

Comment thread test/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-actions Bot 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>

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

Uh oh!

Uh oh!

esm: improve performance & tidy tests - #43784

esm: improve performance & tidy tests#43784aduh95 merged 15 commits intonodejs:mainnodejs/node:mainfrom JakobJingleheimer:esm/tidy-esm-testsJakobJingleheimer/node:esm/tidy-esm-testsCopy head branch name to clipboard

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

esm: improve performance & tidy tests#43784
aduh95 merged 15 commits into
nodejs:mainfrom
JakobJingleheimer:esm/tidy-esm-tests

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