FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v22.x backport] src,test: fix config file parsing for flags defaulted to true by geeksilva97 · Pull Request #59947 · nodejs/node · GitHub

/ node Public

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

Closed
geeksilva97 wants to merge 4 commits into
nodejs:v22.x-stagingfrom
geeksilva97:backport-59110-to-v22.x
ConversationCommits (4)ChecksFiles changed
Closed

[v22.x backport] src,test: fix config file parsing for flags defaulted to true#59947
geeksilva97 wants to merge 4 commits into
nodejs:v22.x-stagingfrom
geeksilva97:backport-59110-to-v22.x

Conversation

geeksilva97 commented Sep 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

pmarchini and others added 4 commits September 20, 2025 12:09
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>
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>
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>
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 requested a review from a team as a code owner September 20, 2025 15:12

nodejs-github-bot commented Sep 20, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

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

geeksilva97 changed the base branch from main to v22.x-staging September 20, 2025 15:12
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 changed the title Backport 59110 to v22.x [v22.x backport] src: config namespaces Sep 20, 2025
geeksilva97 added the backport-open-v22.x Indicate that the PR has an open backport label Sep 20, 2025
geeksilva97 mentioned this pull request Sep 20, 2025
Merged
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 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 reviewed Sep 20, 2025
View reviewed changes
Comment thread src/node_options.cc
AddOption("--experimental-test-isolation",
kAllowedInEnvvar,
OptionNamespaces::kTestRunnerNamespace);
AddOption("--test-isolation",

richardlau Sep 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

geeksilva97 Sep 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

richardlau commented Sep 20, 2025

Copy link
Copy Markdown
Member

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

richardlau added the config Issues or PRs related to the config subsystem label Sep 20, 2025

geeksilva97 commented Sep 20, 2025

Copy link
Copy Markdown
Contributor Author

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

let me check

geeksilva97 commented Sep 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Sep 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author 
test_global_setup_path

Probably this should come first

geeksilva97 commented Sep 22, 2025

Copy link
Copy Markdown
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 commented Sep 22, 2025

Copy link
Copy Markdown
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

aduh95 commented Oct 7, 2025

Copy link
Copy Markdown
Contributor

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 commented Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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.

6 participants

Back | FazBrowse Home | New Git URL