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
Closed
geeksilva97 wants to merge 4 commits into from
Closed

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

geeksilva97 wants to merge 4 commits into from

Conversation

@geeksilva97
Copy link
Contributor

@geeksilva97geeksilva97 commented Sep 20, 2025•
edited
Loading

pmarchiniand 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 <[email protected]> Reviewed-By: Giovanni Bucci <[email protected]> Reviewed-By: Daniel Lemire <[email protected]>
@pmarchini@geeksilva97
src: enhance error messages for unknown options
5df9cd1
PR-URL: nodejs#58677 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
@codebytere@geeksilva97
src: -Wunreachable-code-break in node_config_file.cc
339affe
PR-URL: nodejs#58901 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
@geeksilva97
src,test: fix config file parsing for flags defaulted to true
62dc933
PR-URL: nodejs#59110 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@geeksilva97geeksilva97 requested a review from a team as a code ownerSeptember 20, 2025 15:12
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 20, 2025

Review requested:

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

@geeksilva97geeksilva97 changed the base branch from main to v22.x-stagingSeptember 20, 2025 15:12
@nodejs-github-botnodejs-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
@geeksilva97geeksilva97 changed the title Backport 59110 to v22.x[v22.x backport] src: config namespacesSep 20, 2025
@geeksilva97geeksilva97 added the backport-open-v22.x Indicate that the PR has an open backport label Sep 20, 2025
@geeksilva97geeksilva97 mentioned this pull request Sep 20, 2025
Merged
@geeksilva97geeksilva97 changed the title [v22.x backport] src: config namespaces[v22.x backport] src,test: fix config file parsing for flags defaulted to trueSep 20, 2025
@richardlaurichardlau 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

@richardlaurichardlauSep 20, 2025

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
ContributorAuthor

@geeksilva97geeksilva97Sep 22, 2025

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

richardlau commented Sep 20, 2025

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

@richardlaurichardlau added the config Issues or PRs related to the config subsystem label Sep 20, 2025
@geeksilva97
Copy link
ContributorAuthor

geeksilva97 commented Sep 20, 2025

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

let me check

@geeksilva97
Copy link
ContributorAuthor

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
ContributorAuthor

geeksilva97 commented Sep 21, 2025•
edited
Loading

test_global_setup_path

Probably this should come first

@geeksilva97
Copy link
ContributorAuthor

geeksilva97 commented Sep 22, 2025

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

pmarchini commented Sep 22, 2025

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

@aduh95
Copy link
Contributor

aduh95 commented Oct 7, 2025

Almost no test runner change have landed on v22.x since #54881 wasn't backported, resulting in lots of conflicts and/or output difference. I've tried and quickly given up a few months ago. Given that there's only one v22.x release before it goes into maintenance, the situation is unlikely to change at this point.

@geeksilva97
Copy link
ContributorAuthor

geeksilva97 commented Oct 28, 2025

Almost no test runner change have landed on v22.x since #54881 wasn't backported, resulting in lots of conflicts and/or output difference. I've tried and quickly given up a few months ago. Given that there's only one v22.x release before it goes into maintenance, the situation is unlikely to change at this point.

Totally agree. I'm closing the PR.

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

Reviewers

@richardlaurichardlaurichardlau left review comments

Assignees

No one assigned

Labels

configIssues or PRs related to the config subsystemmetaIssues and PRs related to the general management of the project.toolsIssues and PRs related to the tools directory.v22.xIssues 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.

6 participants

@geeksilva97@nodejs-github-bot@richardlau@pmarchini@aduh95@codebytere