Skip to content
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.

Sign up for GitHub

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

Jump to bottom

esm: misc test refactors #46631

Merged
merged 3 commits into from
Feb 18, 2023
Merged

esm: misc test refactors #46631

merged 3 commits into from
Feb 18, 2023

Conversation

GeoffreyBooth
Copy link
Member

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.

@GeoffreyBooth 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
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 13, 2023
@GeoffreyBooth GeoffreyBooth mentioned this pull request Feb 13, 2023
Merged
7 tasks
@targos
Copy link
Member

targos commented Feb 13, 2023

Move parallel/ tests into es-module/

There are no changes to parallel/ in this PR.

aduh95
aduh95 reviewed Feb 13, 2023
View reviewed changes
test/es-module/test-esm-loader-event-loop.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-event-loop.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-spawn-promisified.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-spawn-promisified.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-spawn-promisified.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-spawn-promisified.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-spawn-promisified.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-spawn-promisified.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-spawn-promisified.mjs Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2023

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

@targos
Copy link
Member

targos commented Feb 13, 2023

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.

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2023 •
edited
Loading

@GeoffreyBooth please consider the other tests as well: 5d7a2b3 (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.

@GeoffreyBooth
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
Copy link
Member Author

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.

@aduh95
Copy link
Contributor

aduh95 commented Feb 15, 2023

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.

@GeoffreyBooth
Copy link
Member Author

I’m still very much -1 on that.

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

@aduh95
Copy link
Contributor

aduh95 commented Feb 15, 2023

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

Both options sound good to me.

@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 15, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 16, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth 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 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
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Feb 18, 2023
Open
32 tasks
@GeoffreyBooth 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 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 nodejs-github-bot merged commit 9e840de into nodejs:main Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 9e840de

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@GeoffreyBooth @MylesBorins
esm: misc test refactors
07dfa3c 
- 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>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
Merged
@github-actions github-actions bot mentioned this pull request Feb 19, 2023
Open
21 tasks
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
@GeoffreyBooth @MylesBorins
esm: misc test refactors
f192b83 
- 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>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@GeoffreyBooth @danielleadams
esm: misc test refactors
320a8ad 
- 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>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GeoffreyBooth @targos @aduh95 @nodejs-github-bot