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.

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

test: make some requires lazy in common/index #56715

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 23, 2025

Lazily require child_process and isModuleNamespaceObject only when actually needed.

@jasnell jasnell requested review from mcollina and anonrig January 23, 2025 01:00
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 23, 2025
@nodejs-github-bot

This comment was marked as outdated.

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.21%. Comparing base (bade7a1) to head (47b9729).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56715   +/-   ##
=======================================
  Coverage   89.21%   89.21%           
=======================================
  Files         662      662           
  Lines      192124   192124           
  Branches    36980    36975    -5     
=======================================
+ Hits       171404   171409    +5     
+ Misses      13552    13548    -4     
+ Partials     7168     7167    -1     

see 20 files with indirect coverage changes

@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 23, 2025

@jasnell jasnell added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 24, 2025
@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 Jan 25, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56715
✔  Done loading data for nodejs/node/pull/56715
----------------------------------- PR info ------------------------------------
Title      test: make some requires lazy in common/index (#56715)
Author     James M Snell <[email protected]> (@jasnell)
Branch     jasnell:jasnell/test-lazy-requires -> nodejs:main
Labels     test, author ready
Commits    1
 - test: make some requires lazy in common/index
Committers 1
 - James M Snell <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56715
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56715
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 23 Jan 2025 01:00:29 GMT
   ✔  Approvals: 2
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/56715#pullrequestreview-2568641434
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/56715#pullrequestreview-2570142106
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-01-23T22:59:14Z: https://ci.nodejs.org/job/node-test-pull-request/64676/
- Querying data for job/node-test-pull-request/64676/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 56715
From https://github.com/nodejs/node
 * branch                  refs/pull/56715/merge -> FETCH_HEAD
✔  Fetched commits as c752615e2bbe..dfea11db9157
--------------------------------------------------------------------------------
Auto-merging test/common/index.js
CONFLICT (content): Merge conflict in test/common/index.js
error: could not apply dfea11db91... test: make some requires lazy in common/index
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
   ✘  Failed to apply patches
https://github.com/nodejs/node/actions/runs/12960548139

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jasnell jasnell force-pushed the jasnell/test-lazy-requires branch from dfea11d to 47b9729 Compare January 25, 2025 14:53
@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jan 25, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

jasnell added a commit that referenced this pull request Jan 25, 2025
PR-URL: #56715
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jan 25, 2025

Landed in 23d0a7f

@jasnell jasnell closed this Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants