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

module: move test reporter loading #45923

Merged
merged 9 commits into from
Dec 22, 2022
Merged

module: move test reporter loading #45923

merged 9 commits into from
Dec 22, 2022

Conversation

GeoffreyBooth
Copy link
Member

This PR resolves my concerns with #45712 (comment), without making any changes to --test-reporter or breaking any tests. Essentially, it moves the module loading that --test-reporter wants to do to happen inside the test_runner section of the codebase, keeping it separate; and it loads all test reporters through the ESM loader, following our current design for module loading features and preserving support for things like loaders applying to test reporters.

All changes in this PR for files under lib/internal/modules are just reverting the changes to those files that landed in #45712.

cc @nodejs/test_runner @nodejs/loaders @nodejs/modules

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders test_runner Issues and PRs related to the test runner subsystem. labels Dec 20, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. labels Dec 20, 2022
@GeoffreyBooth GeoffreyBooth mentioned this pull request Dec 20, 2022
Merged
3 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@GeoffreyBooth GeoffreyBooth mentioned this pull request Dec 20, 2022
Closed
cjihrig
cjihrig reviewed Dec 20, 2022
View reviewed changes
doc/api/test.md Outdated Show resolved Hide resolved
MoLow
MoLow approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, @GeoffreyBooth looks really good. indeed a ping on the original PR would have resolved this entire discussion!

@GeoffreyBooth
Copy link
Member Author

thanks, @GeoffreyBooth looks really good. indeed a ping on the original PR would have resolved this entire discussion!

Agreed. Sorry if I came across as a bit annoyed. At first I thought you’d introduced a merge conflict with #45869 which I’ve been trying to land for days, but fortunately you hadn’t 😄

@MoLow
Copy link
Member

MoLow commented Dec 20, 2022

@GeoffreyBooth if it is ok with you can we cherry-pick / push this commit to this PR? 9bb6248

@GeoffreyBooth
Copy link
Member Author

@GeoffreyBooth if it is ok with you can we cherry-pick / push this commit to this PR? 9bb6248

I guess, but I kind of feel like this should be on its own as its mostly a “revert” PR which are usually isolated. Also this should hopefully be able to land immediately, which would then allow #45712 to go out in a release; whereas if there are any delays in whatever you want to cherry-pick this onto, then this revert fix and #45712’s release also get delayed.

@MoLow
Copy link
Member

MoLow commented Dec 20, 2022

Ok Il ship it in a follow-up PR then

@MoLow
Copy link
Member

MoLow commented Dec 20, 2022

Discussed with @GeoffreyBooth in slack, and pushed tests + lint fix

@MoLow MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 20, 2022
JakobJingleheimer
JakobJingleheimer approved these changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@GeoffreyBooth GeoffreyBooth force-pushed the move-test-reporter-loading branch 2 times, most recently from d896dce to 021ad3d Compare December 20, 2022 23:51
@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2022
@RafaelGSS RafaelGSS added dont-land-on-v16.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Jan 4, 2023
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
@GeoffreyBooth @RafaelGSS
module: move test reporter loading
1d686b7 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: #45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jan 26, 2023
@GeoffreyBooth @MoLow
module: move test reporter loading
9df0311 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jan 26, 2023
@GeoffreyBooth @MoLow
module: move test reporter loading
5a2ea89 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@MoLow MoLow mentioned this pull request Jan 26, 2023
Closed
MoLow pushed a commit to MoLow/node that referenced this pull request Jan 26, 2023
@GeoffreyBooth @MoLow
module: move test reporter loading
ee143db 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jan 26, 2023
@GeoffreyBooth @MoLow
module: move test reporter loading
249f764 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@MoLow MoLow mentioned this pull request Jan 26, 2023
Closed
@MoLow MoLow added backport-open-v18.x Indicate that the PR has an open backport. backport-open-v19.x and removed dont-land-on-v16.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Jan 26, 2023
MoLow pushed a commit to MoLow/node that referenced this pull request Jan 26, 2023
@GeoffreyBooth @MoLow
module: move test reporter loading
7ad6b73 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
ruyadorno pushed a commit that referenced this pull request Jan 31, 2023
@GeoffreyBooth @ruyadorno
module: move test reporter loading
3a3a6d8 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

Backport-PR-URL: #46361
PR-URL: #45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Feb 1, 2023
Merged
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 6, 2023
@GeoffreyBooth @MoLow
fix: move test reporter loading
0556bc7 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs/node#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 12c0571c8fece32d274eaf0ae197c0eb1948fe11)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 7, 2023
@GeoffreyBooth @MoLow
fix: move test reporter loading
9ff5db7 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs/node#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 12c0571c8fece32d274eaf0ae197c0eb1948fe11)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 7, 2023
@GeoffreyBooth @MoLow
fix: move test reporter loading
5d12932 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs/node#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 12c0571c8fece32d274eaf0ae197c0eb1948fe11)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 7, 2023
@GeoffreyBooth @MoLow
fix: move test reporter loading
57de87d 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs/node#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 12c0571c8fece32d274eaf0ae197c0eb1948fe11)
MoLow pushed a commit to nodejs/node-core-test that referenced this pull request Feb 8, 2023
@GeoffreyBooth @MoLow
fix: move test reporter loading
a5e0e9e 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs/node#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
(cherry picked from commit 12c0571c8fece32d274eaf0ae197c0eb1948fe11)
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
@GeoffreyBooth @MoLow
module: move test reporter loading
a34809a 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
@GeoffreyBooth @juanarbol
module: move test reporter loading
4e60e56 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: #45923
Backport-PR-URL: #46839
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
@GeoffreyBooth @juanarbol
module: move test reporter loading
403df21 
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: #45923
Backport-PR-URL: #46839
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos removed the backport-open-v18.x Indicate that the PR has an open backport. label Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@aduh95 aduh95 aduh95 approved these changes

@MoLow MoLow MoLow approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@benjamingr benjamingr benjamingr 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 module Issues and PRs related to the module subsystem. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@GeoffreyBooth @nodejs-github-bot @MoLow @Flarna @targos @RafaelGSS @benjamingr @cjihrig @JakobJingleheimer @aduh95