Skip to content

[v22.x backport] src,test: fix config file parsing for flags defaulted to true #59947

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
Open
wants to merge 4 commits into
base: v22.x-staging
Choose a base branch
from
Open

[v22.x backport] src,test: fix config file parsing for flags defaulted to true #59947

wants to merge 4 commits into from

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Sep 20, 2025 •
edited
Loading

pmarchini and others added 4 commits September 20, 2025 12:09
@pmarchini @geeksilva97
src: support namespace options in configuration file
f55a9b6 
PR-URL: nodejs#58073
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Giovanni Bucci <github@puskin.it>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
@pmarchini @geeksilva97
src: enhance error messages for unknown options
5df9cd1 
PR-URL: nodejs#58677
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere @geeksilva97
src: -Wunreachable-code-break in node_config_file.cc
339affe 
PR-URL: nodejs#58901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
@geeksilva97
src,test: fix config file parsing for flags defaulted to true
62dc933 
PR-URL: nodejs#59110
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@geeksilva97 geeksilva97 requested a review from a team as a code owner September 20, 2025 15:12
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/releasers
  • @nodejs/tsc

@geeksilva97 geeksilva97 changed the base branch from main to v22.x-staging September 20, 2025 15:12
@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Sep 20, 2025
@geeksilva97 geeksilva97 changed the title Backport 59110 to v22.x [v22.x backport] src: config namespaces Sep 20, 2025
@geeksilva97 geeksilva97 added the backport-open-v22.x Indicate that the PR has an open backport label Sep 20, 2025
@geeksilva97 geeksilva97 mentioned this pull request Sep 20, 2025
Merged
@geeksilva97 geeksilva97 changed the title [v22.x backport] src: config namespaces [v22.x backport] src,test: fix config file parsing for flags defaulted to true Sep 20, 2025
@richardlau richardlau added v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport labels Sep 20, 2025
richardlau
richardlau reviewed Sep 20, 2025
View reviewed changes
src/node_options.cc
AddOption("--experimental-test-isolation",
kAllowedInEnvvar,
OptionNamespaces::kTestRunnerNamespace);
AddOption("--test-isolation",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have consensus to remove the experimental status from test isolation in 22.x? I don't even know if all of the relevant commits are on v22.x as a lot of test runner related PRs are awaiting manual backports cc @nodejs/test_runner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and there are a lot of things missing. I cherry-pick node_config stuff, and a bunch of testrunner tests fail.

@richardlau
Copy link
Member

@geeksilva97 This is failing linter and tests on GitHub actions.

@richardlau richardlau added the config Issues or PRs related to the config subsystem label Sep 20, 2025
@geeksilva97
Copy link
Contributor Author

@geeksilva97 This is failing linter and tests on GitHub actions.

let me check

@geeksilva97
Copy link
Contributor Author

geeksilva97 commented Sep 21, 2025 •
edited
Loading

Without manual intervention, the commit c1f090d doesn't even compile when picked to the v22.x-staging branch.

../src/node_options.cc:863:34: error: no member named 'test_global_setup_path' in 'node::EnvironmentOptions'
  863 |             &EnvironmentOptions::test_global_setup_path,
      |              ~~~~~~~~~~~~~~~~~~~~^
1 error generated.
make[1]: *** [/Users/edy/projects/contributions/node/out/Release/obj.target/libnode/src/node_options.o] Error 1
make[1]: *** Waiting for unfinished jobs....

It misses this test_global_setup_path (exists on main). I guess that there's another PR that should be backported.

@geeksilva97
Copy link
Contributor Author

geeksilva97 commented Sep 21, 2025 •
edited
Loading 

test_global_setup_path

Probably this should come first

@geeksilva97
Copy link
Contributor Author 

test_global_setup_path

Probably this should come first

This was not enough. A bunch test_runner related tests are failing. I don't know exactly what should be the very first commit that should be picked. Would you have a clue @pmarchini ?

@pmarchini
Copy link
Member 

test_global_setup_path

Probably this should come first

This was not enough. A bunch test_runner related tests are failing. I don't know exactly what should be the very first commit that should be picked. Would you have a clue @pmarchini ?

Hey @geeksilva97, I'll take a look ASAP

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

@richardlau richardlau richardlau left review comments

Assignees
No one assigned
Labels
config Issues or PRs related to the config subsystem meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@geeksilva97 @nodejs-github-bot @richardlau @pmarchini @codebytere