Skip to content

src,test: fix config file parsing for flags defaulted to true #59110

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

Merged
merged 1 commit into from
Jul 20, 2025
+20 −0
Merged

src,test: fix config file parsing for flags defaulted to true #59110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/node_config_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ ParseResult ConfigReader::ProcessOptionValue(
if (result) {
// If the value is true, we need to set the flag
output->push_back(option_name);
} else {
// Ensure negation is made putting the "--no-" prefix
output->push_back("--no-" +
option_name.substr(2, option_name.size() - 2));
}

break;
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/rc/warnings-false.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"nodeOptions": {
"warnings": false
}
}
11 changes: 11 additions & 0 deletions test/parallel/test-config-file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ test('should parse boolean flag', async () => {
strictEqual(result.code, 0);
});

test('should parse boolean flag defaulted to true', async () => {
const result = await spawnPromisified(process.execPath, [
'--experimental-config-file',
Comment on lines +63 to +65
Copy link
Member

Choose a reason for hiding this comment

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

Very minor nit: we can also expose internals in order to directly verify the value of the option that we're setting. This might help decouple the option behaviour from the test. In this case, we're using the behaviour of process.emitWarning to validate that --no-warnings has been correctly managed.

Example:
https://github.com/geeksilva97/node/blob/99ef802c0031af7d0e3313cda60b6fac8471ac06/test/parallel/test-config-file.js#L479-L491

fixtures.path('rc/warnings-false.json'),
'-p', 'process.emitWarning("A warning")',
]);
strictEqual(result.stderr, '');
strictEqual(result.stdout, 'undefined\n');
strictEqual(result.code, 0);
});

test('should throw an error when a flag is declared twice', async () => {
const result = await spawnPromisified(process.execPath, [
'--no-warnings',
Expand Down
Loading