-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
test: add tests to ensure that node.1 is kept in sync with cli.md #58878
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.
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
Changes from all commits
31315b2
6acdbef
8f55596
09a14d1
a2a1c6f
c3be698
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
import '../common/index.mjs'; | ||
import assert from 'node:assert'; | ||
import { createReadStream } from 'node:fs'; | ||
import { createInterface } from 'node:readline'; | ||
import { resolve, join } from 'node:path'; | ||
|
||
// This test checks that all the environment variables defined in the public CLI documentation (doc/api/cli.md) | ||
// are also documented in the manpage file (doc/node.1) and vice-versa (that all the environment variables | ||
// in the manpage are present in the CLI documentation) | ||
|
||
const rootDir = resolve(import.meta.dirname, '..', '..'); | ||
|
||
const cliMdEnvVarNames = await collectCliMdEnvVarNames(); | ||
const manpageEnvVarNames = await collectManPageEnvVarNames(); | ||
|
||
assert(cliMdEnvVarNames.size > 0, | ||
'Unexpectedly not even a single env variable was detected when scanning the `doc/api/cli.md` file' | ||
); | ||
|
||
assert(manpageEnvVarNames.size > 0, | ||
'Unexpectedly not even a single env variable was detected when scanning the `doc/node.1` file' | ||
); | ||
|
||
// TODO(dario-piotrowicz): add the missing env variables to the manpage and remove this set | ||
// (refs: https://github.com/nodejs/node/issues/58894) | ||
const knownEnvVariablesMissingFromManPage = new Set([ | ||
'NODE_COMPILE_CACHE', | ||
'NODE_DISABLE_COMPILE_CACHE', | ||
'NODE_PENDING_PIPE_INSTANCES', | ||
'NODE_TEST_CONTEXT', | ||
'NODE_USE_ENV_PROXY', | ||
]); | ||
|
||
for (const envVarName of cliMdEnvVarNames) { | ||
if (!manpageEnvVarNames.has(envVarName) && !knownEnvVariablesMissingFromManPage.has(envVarName)) { | ||
assert.fail(`The "${envVarName}" environment variable (present in \`doc/api/cli.md\`) is missing from the \`doc/node.1\` file`); | ||
} | ||
manpageEnvVarNames.delete(envVarName); | ||
} | ||
|
||
if (manpageEnvVarNames.size > 0) { | ||
assert.fail(`The following env variables are present in the \`doc/node.1\` file but not in the \`doc/api/cli.md\` file: ${ | ||
[...manpageEnvVarNames].map((name) => `"${name}"`).join(', ') | ||
}`); | ||
} | ||
|
||
async function collectManPageEnvVarNames() { | ||
const manPagePath = join(rootDir, 'doc', 'node.1'); | ||
const fileStream = createReadStream(manPagePath); | ||
|
||
const rl = createInterface({ | ||
input: fileStream, | ||
}); | ||
|
||
const envVarNames = new Set(); | ||
|
||
for await (const line of rl) { | ||
const match = line.match(/^\.It Ev (?<envName>[^ ]*)/); | ||
if (match) { | ||
envVarNames.add(match.groups.envName); | ||
} | ||
} | ||
|
||
return envVarNames; | ||
} | ||
|
||
async function collectCliMdEnvVarNames() { | ||
const cliMdPath = join(rootDir, 'doc', 'api', 'cli.md'); | ||
const fileStream = createReadStream(cliMdPath); | ||
|
||
let insideEnvVariablesSection = false; | ||
|
||
const rl = createInterface({ | ||
input: fileStream, | ||
}); | ||
|
||
const envVariableRE = /^### `(?<varName>[^`]*?)(?:=[^`]+)?`$/; | ||
|
||
const envVarNames = new Set(); | ||
|
||
for await (const line of rl) { | ||
if (line.startsWith('## ')) { | ||
if (insideEnvVariablesSection) { | ||
// We were in the environment variables section and we're now exiting it, | ||
// so there is no need to keep checking the remaining lines, | ||
// we might as well close the stream and return | ||
fileStream.close(); | ||
return envVarNames; | ||
} | ||
|
||
// We've just entered the options section | ||
insideEnvVariablesSection = line === '## Environment variables'; | ||
continue; | ||
} | ||
|
||
if (insideEnvVariablesSection) { | ||
const match = line.match(envVariableRE); | ||
if (match) { | ||
const { varName } = match.groups; | ||
envVarNames.add(varName); | ||
} | ||
} | ||
} | ||
|
||
return envVarNames; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,167 @@ | ||
import '../common/index.mjs'; | ||
import assert from 'node:assert'; | ||
import { createReadStream, readFileSync } from 'node:fs'; | ||
import { createInterface } from 'node:readline'; | ||
import { resolve, join } from 'node:path'; | ||
import { EOL } from 'node:os'; | ||
|
||
// This test checks that all the CLI flags defined in the public CLI documentation (doc/api/cli.md) | ||
// are also documented in the manpage file (doc/node.1) | ||
// Note: the opposite (that all variables in doc/node.1 are documented in the CLI documentation) | ||
// is covered in the test-cli-node-options-docs.js file | ||
|
||
const rootDir = resolve(import.meta.dirname, '..', '..'); | ||
|
||
const cliMdPath = join(rootDir, 'doc', 'api', 'cli.md'); | ||
const cliMdContentsStream = createReadStream(cliMdPath); | ||
|
||
const manPagePath = join(rootDir, 'doc', 'node.1'); | ||
const manPageContents = readFileSync(manPagePath, { encoding: 'utf8' }); | ||
|
||
// TODO(dario-piotrowicz): add the missing flags to the node.1 and remove this set | ||
// (refs: https://github.com/nodejs/node/issues/58895) | ||
const knownFlagsMissingFromManPage = new Set([ | ||
'build-snapshot', | ||
'build-snapshot-config', | ||
'disable-sigusr1', | ||
'disable-warning', | ||
'dns-result-order', | ||
'enable-network-family-autoselection', | ||
'env-file-if-exists', | ||
'env-file', | ||
'experimental-network-inspection', | ||
'experimental-print-required-tla', | ||
'experimental-require-module', | ||
'experimental-sea-config', | ||
'experimental-worker-inspection', | ||
'expose-gc', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only one I'm not sure about adding to node.1. The rest should be fine tho. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the rationale about wanting the flag in the CLI docs but not in the manpage? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation was added in #53078 It seems like you and Joyee requested the doc's removal, but only part of it was actually removed? 🤔 (although Joyee's next suggestion makes it sound like they were ok with the docs after all?... 🤔) Anyways, it looks likely that the docs for the flag were just added by mistake, what do you think? |
||
'force-node-api-uncaught-exceptions-policy', | ||
'import', | ||
'network-family-autoselection-attempt-timeout', | ||
'no-async-context-frame', | ||
'no-experimental-detect-module', | ||
'no-experimental-global-navigator', | ||
'no-experimental-require-module', | ||
'no-network-family-autoselection', | ||
'openssl-legacy-provider', | ||
'openssl-shared-config', | ||
'report-dir', | ||
'report-directory', | ||
'report-exclude-env', | ||
'report-exclude-network', | ||
'run', | ||
'snapshot-blob', | ||
'trace-env', | ||
'trace-env-js-stack', | ||
'trace-env-native-stack', | ||
'trace-require-module', | ||
'use-system-ca', | ||
'watch-preserve-output', | ||
]); | ||
jasnell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const optionsEncountered = { dash: 0, dashDash: 0, named: 0 }; | ||
let insideOptionsSection = false; | ||
|
||
const rl = createInterface({ | ||
input: cliMdContentsStream, | ||
}); | ||
|
||
const isOptionLineRegex = /^###(?: `[^`]*`,?)*$/; | ||
|
||
for await (const line of rl) { | ||
if (line.startsWith('## ')) { | ||
if (insideOptionsSection) { | ||
// We were in the options section and we're now exiting it, | ||
// so there is no need to keep checking the remaining lines, | ||
// we might as well close the stream and exit the loop | ||
cliMdContentsStream.close(); | ||
break; | ||
} | ||
|
||
// We've just entered the options section | ||
insideOptionsSection = line === '## Options'; | ||
continue; | ||
} | ||
|
||
if (insideOptionsSection && isOptionLineRegex.test(line)) { | ||
if (line === '### `-`') { | ||
if (!manPageContents.includes(`${EOL}.It Sy -${EOL}`)) { | ||
throw new Error(`The \`-\` flag is missing in the \`doc/node.1\` file`); | ||
} | ||
optionsEncountered.dash++; | ||
continue; | ||
} | ||
|
||
if (line === '### `--`') { | ||
if (!manPageContents.includes(`${EOL}.It Fl -${EOL}`)) { | ||
throw new Error(`The \`--\` flag is missing in the \`doc/node.1\` file`); | ||
} | ||
optionsEncountered.dashDash++; | ||
continue; | ||
} | ||
|
||
const flagNames = extractFlagNames(line); | ||
|
||
optionsEncountered.named += flagNames.length; | ||
|
||
const manLine = `.It ${flagNames | ||
.map((flag) => `Fl ${flag.length > 1 ? '-' : ''}${flag}`) | ||
.join(' , ')}`; | ||
|
||
if ( | ||
// Note: we don't check the full line (note the EOL only at the beginning) because | ||
// options can have arguments and we do want to ignore those | ||
!manPageContents.includes(`${EOL}${manLine}`) && | ||
!flagNames.every((flag) => knownFlagsMissingFromManPage.has(flag))) { | ||
assert.fail( | ||
`The following flag${ | ||
flagNames.length === 1 ? '' : 's' | ||
} (present in \`doc/api/cli.md\`) ${flagNames.length === 1 ? 'is' : 'are'} missing in the \`doc/node.1\` file: ${ | ||
flagNames.map((flag) => `"${flag}"`).join(', ') | ||
}` | ||
); | ||
} | ||
} | ||
} | ||
|
||
assert.strictEqual(optionsEncountered.dash, 1); | ||
|
||
assert.strictEqual(optionsEncountered.dashDash, 1); | ||
|
||
assert(optionsEncountered.named > 0, | ||
'Unexpectedly not even a single cli flag/option was detected when scanning the `doc/cli.md` file' | ||
); | ||
|
||
/** | ||
* Function that given a string containing backtick enclosed cli flags | ||
* separated by `, ` returns the name of flags present in the string | ||
* e.g. `extractFlagNames('`-x`, `--print "script"`')` === `['x', 'print']` | ||
* @param {string} str target string | ||
* @returns {string[]} the name of the detected flags | ||
*/ | ||
function extractFlagNames(str) { | ||
const match = str.match(/`[^`]*?`/g); | ||
if (!match) { | ||
return []; | ||
} | ||
return match.map((flag) => { | ||
// Remove the backticks from the flag | ||
flag = flag.slice(1, -1); | ||
|
||
// Remove the dash or dashes | ||
flag = flag.replace(/^--?/, ''); | ||
|
||
// If the flag contains parameters make sure to remove those | ||
const nameDelimiters = ['=', ' ', '[']; | ||
const nameCutOffIdx = Math.min(...nameDelimiters.map((d) => { | ||
const idx = flag.indexOf(d); | ||
if (idx > 0) { | ||
return idx; | ||
} | ||
return flag.length; | ||
})); | ||
flag = flag.slice(0, nameCutOffIdx); | ||
|
||
return flag; | ||
}); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.