esm: misc test refactors by GeoffreyBooth · Pull Request #46631 · nodejs/node · GitHub

This PR contains miscellaneous test refactors that were done as part of #44710 but can live on their own. Specifically:

Add a new test specific to the event loop
Move parallel/ tests into es-module/
Refactor hooks-custom.mjs fixture to put the resolve hook first and add braces for if blocks
Use os instead of fs as a dummy placeholder, since other loaders override fs as part of their tests
Spelling, line lengths

This PR also adds test/es-module/test-esm-loader-spawn-promisified.mjs, which is a rewrite of test-esm-loader.mjs to use the test runner and spawnPromisified. Personally I like this version better, but if people prefer test-esm-loader.mjs I can delete the new file; I don’t think we should keep both. The new test test-esm-loader-event-loop.mjs covers a scenario that the old test-esm-loader.mjs was inadvertently testing, of many rejected promises in the same file loaded through a custom loader.

Move parallel/ tests into es-module/

There are no changes to parallel/ in this PR.

Move parallel/ tests into es-module/

There are no changes to parallel/ in this PR.

Yes there are, see https://github.com/nodejs/node/pull/46631/files#diff-fc0306c592a1d4956d08f363252348f6c94a3408ba564068e94d121c1f648755. (maybe your GitHub UI is configured to hide this kind of changes?)

Move parallel/ tests into es-module/

There are no changes to parallel/ in this PR.

Yes there are, see https://github.com/nodejs/node/pull/46631/files#diff-fc0306c592a1d4956d08f363252348f6c94a3408ba564068e94d121c1f648755. (maybe your GitHub UI is configured to hide this kind of changes?)

Ok my bad, I looked at the tree on the left which only shows the destination folders.

@GeoffreyBooth please consider the other tests as well: 5d7a2b35a31ce2d0b5bc09a398f97413e7be3348 (unless they are already somewhere else?) (EDIT: I misread the GH UI, all the files are already there). Note that test/es-module/test-loaders-this-value-inside-hook-functions.mjs would benefit from being included to test/es-module/test-esm-loader-hooks.mjs as a "spawnPromisified" test rather than being a standalone file.

- add test specific to the event loop - move parallel tests into es-module folder - refactor fixture to add braces for if blocks - use 'os' instead of 'fs' as placeholder - spelling

Note that test/es-module/test-loaders-this-value-inside-hook-functions.mjs would benefit from being included to test/es-module/test-esm-loader-hooks.mjs as a “spawnPromisified” test rather than being a standalone file.

Done, along with all your other notes.

Can I delete test/es-module/test-esm-loader.mjs now? I think everything it tests is now covered by test/es-module/test-esm-loader-spawn-promisified.mjs and test/es-module/test-esm-loader-event-loop.mjs.

I think everything it tests is now covered by test/es-module/test-esm-loader-spawn-promisified.mjs and test/es-module/test-esm-loader-event-loop.mjs.

I disagree, those are testing different things.

Can I delete test/es-module/test-esm-loader.mjs now?

I'm still very much -1 on that.

I’m still very much -1 on that.

So what are you suggesting? We keep both files? We delete the spawn promisified version?

So what are you suggesting? We keep both files? We delete the spawn promisified version?

Both options sound good to me.

CI: https://ci.nodejs.org/job/node-test-pull-request/49582/

CI: https://ci.nodejs.org/job/node-test-pull-request/49613/

CI: https://ci.nodejs.org/job/node-test-pull-request/49634/

CI: https://ci.nodejs.org/job/node-test-pull-request/49639/

CI: https://ci.nodejs.org/job/node-test-pull-request/49647/

Commit Queue failed

- Loading data for nodejs/node/pull/46631
✔  Done loading data for nodejs/node/pull/46631
----------------------------------- PR info ------------------------------------
Title      esm: misc test refactors (#46631)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     GeoffreyBooth:refactor-loaders-tests -> nodejs:main
Labels     test, esm, author ready, loaders, commit-queue-squash
Commits    3
 - esm: misc test refactors
 - code review notes
 - Reorder assertions
Committers 1
 - Geoffrey Booth 
PR-URL: https://github.com/nodejs/node/pull/46631
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46631
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 13 Feb 2023 00:33:52 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46631#pullrequestreview-1299954715
   ✖  This PR needs to wait 53 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-02-17T18:24:53Z: https://ci.nodejs.org/job/node-test-pull-request/49647/
- Querying data for job/node-test-pull-request/49647/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4206855041

Landed in 9e840de

- add test specific to the event loop - move parallel tests into es-module folder - refactor fixture to add braces for if blocks - use 'os' instead of 'fs' as placeholder - spelling PR-URL: #46631 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

GeoffreyBooth added test Issues and PRs related to the tests. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Feb 13, 2023

GeoffreyBooth requested a review from aduh95 February 13, 2023 00:33

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 13, 2023

GeoffreyBooth mentioned this pull request Feb 13, 2023

Move ESM loaders off-thread #44710

Merged

7 tasks

GeoffreyBooth force-pushed the refactor-loaders-tests branch from 5a83017 to 260b2e4 Compare February 13, 2023 01:00

aduh95 reviewed Feb 13, 2023

View reviewed changes

esm: misc test refactors

3f5f827

- add test specific to the event loop - move parallel tests into es-module folder - refactor fixture to add braces for if blocks - use 'os' instead of 'fs' as placeholder - spelling

GeoffreyBooth force-pushed the refactor-loaders-tests branch from 260b2e4 to 3042822 Compare February 15, 2023 05:12

code review notes

e54cd4e

GeoffreyBooth force-pushed the refactor-loaders-tests branch from 3042822 to e54cd4e Compare February 15, 2023 05:18

aduh95 reviewed Feb 15, 2023

View reviewed changes

Comment thread test/es-module/test-esm-loader-spawn-promisified.mjs Outdated

Reorder assertions

1c0a8c6

aduh95 approved these changes Feb 15, 2023

View reviewed changes

GeoffreyBooth added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 15, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 16, 2023

GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Feb 17, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 17, 2023

github-actions Bot mentioned this pull request Feb 18, 2023

CI Reliability 2023-02-18 nodejs/reliability#499

Open

32 tasks

targos approved these changes Feb 18, 2023

View reviewed changes

GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot merged commit 9e840de into nodejs:main Feb 18, 2023

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

github-actions Bot mentioned this pull request Feb 19, 2023

CI Reliability 2023-02-19 nodejs/reliability#500

Open

21 tasks

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

esm: misc test refactors by GeoffreyBooth · Pull Request #46631 · nodejs/node · GitHub

Uh oh!

Uh oh!

esm: misc test refactors - #46631

esm: misc test refactors#46631nodejs-github-bot merged 3 commits intonodejs:mainnodejs/node:mainfrom GeoffreyBooth:refactor-loaders-testsGeoffreyBooth/node:refactor-loaders-testsCopy head branch name to clipboard

Conversation

GeoffreyBooth commented Feb 13, 2023

Uh oh!

targos commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Feb 13, 2023

Uh oh!

targos commented Feb 13, 2023

Uh oh!

aduh95 commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoffreyBooth commented Feb 15, 2023

Uh oh!

Uh oh!

aduh95 commented Feb 15, 2023

Uh oh!

GeoffreyBooth commented Feb 15, 2023

Uh oh!

aduh95 commented Feb 15, 2023

Uh oh!

nodejs-github-bot commented Feb 16, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

esm: misc test refactors#46631
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
GeoffreyBooth:refactor-loaders-tests

aduh95 commented Feb 13, 2023 •
edited
Loading