Skip to content

module: move test reporter loading#45923

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
Merged
nodejs-github-bot merged 9 commits into from
Dec 22, 2022
Merged

module: move test reporter loading #45923

nodejs-github-bot merged 9 commits into from
Dec 22, 2022

Conversation

@GeoffreyBooth
Copy link
Member

@GeoffreyBoothGeoffreyBooth commented Dec 20, 2022

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

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

nodejs-github-bot commented Dec 20, 2022

Review requested:

  • @nodejs/modules

@nodejs-github-botnodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. labels Dec 20, 2022
@GeoffreyBoothGeoffreyBooth mentioned this pull request Dec 20, 2022
Merged
3 tasks
@GeoffreyBoothGeoffreyBoothforce-pushed the move-test-reporter-loading branch from 722fb95 to 2751e68CompareDecember 20, 2022 18:20
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@GeoffreyBoothGeoffreyBooth mentioned this pull request Dec 20, 2022
Closed
cjihrig
cjihrig reviewed Dec 20, 2022
View reviewed changes
MoLow
MoLow approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@MoLowMoLow 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
MemberAuthor

GeoffreyBooth commented Dec 20, 2022

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
MemberAuthor

GeoffreyBooth commented Dec 20, 2022

@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

aduh95
aduh95 reviewed Dec 20, 2022
View reviewed changes
@MoLow
Copy link
Member

MoLow commented Dec 20, 2022

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

@MoLowMoLow 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
Member

@JakobJingleheimerJakobJingleheimer left a comment

Choose a reason for hiding this comment

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

🙌

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

nodejs-github-bot commented Dec 20, 2022

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

@GeoffreyBoothGeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2022
aduh95
aduh95 reviewed Dec 21, 2022
View reviewed changes
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]>
@MoLowMoLow 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]>
@MoLowMoLow 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
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]>
@ruyadornoruyadorno 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]>
@juanarboljuanarbol 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]>
@octo-stsocto-stsbot mentioned this pull request Dec 12, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@JakobJingleheimerJakobJingleheimerJakobJingleheimer approved these changes

@MoLowMoLowMoLow approved these changes

@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.esmIssues and PRs related to the ECMAScript Modules implementation.loadersIssues and PRs related to ES module loadersmoduleIssues and PRs related to the module subsystem.test_runnerIssues 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