Skip to content

Conversation

@pmarchini
Copy link
Member

This PR should address #60986!

At the moment, this solution doesn't take into account NODE_OPTIONS nor the config file.
In order to support both of them, at least for V8 options defined as kV8Option, changes to GetOptionsAsFlags are required.

They will follow in a follow-up PR.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@pmarchini pmarchini marked this pull request as ready for review December 8, 2025 23:36
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Dec 8, 2025
@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.53%. Comparing base (e28656a) to head (410174c).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60999      +/-   ##
==========================================
+ Coverage   88.51%   88.53%   +0.01%     
==========================================
  Files         703      703              
  Lines      208496   208512      +16     
  Branches    40213    40215       +2     
==========================================
+ Hits       184555   184609      +54     
+ Misses      15955    15948       -7     
+ Partials     7986     7955      -31     
Files with missing lines Coverage Δ
lib/internal/test_runner/runner.js 92.94% <100.00%> (+0.12%) ⬆️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

const child = spawnSync(
process.execPath,
[
'--allow-natives-syntax',
Copy link
Member

Choose a reason for hiding this comment

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

We don't actually validate this flag made any change. Do we want to pass this?
Maybe instead of calling globalThis.gc() we could check the execArgv inside the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

You absolutely right! Thanks for the suggestion!

@pmarchini pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2025
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x labels Dec 9, 2025
@pmarchini pmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 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 Dec 10, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/60999
✔  Done loading data for nodejs/node/pull/60999
----------------------------------- PR info ------------------------------------
Title      test_runner: propagate V8 options to child process (#60999)
Author     Pietro Marchini <pietro.marchini94@gmail.com> (@pmarchini)
Branch     pmarchini:issue-60986 -> nodejs:main
Labels     needs-ci, test_runner, lts-watch-v22.x, lts-watch-v24.x
Commits    1
 - test_runner: propagate V8 options to child process
Committers 1
 - Pietro Marchini <pietro.marchini94@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/60999
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/60999
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 08 Dec 2025 23:35:59 GMT
   ✔  Approvals: 6
   ✔  - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/60999#pullrequestreview-3554808102
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/60999#pullrequestreview-3555539052
   ✔  - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/60999#pullrequestreview-3556065594
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/60999#pullrequestreview-3556879192
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/60999#pullrequestreview-3556965246
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/60999#pullrequestreview-3559851497
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2025-12-09T12:33:04Z: https://ci.nodejs.org/job/node-test-pull-request/70446/
- Querying data for job/node-test-pull-request/70446/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/20116762228

PR-URL: nodejs#60999
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@aduh95 aduh95 merged commit 410174c into nodejs:main Dec 10, 2025
21 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Dec 10, 2025

Landed in 410174c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-failed An error occurred while landing this pull request using GitHub Actions. lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants