doc,test: test documentation consistency for NODE_OPTIONS by richardlau · Pull Request #28179 · nodejs/node · GitHub

doc: add missing options allowed in NODE_OPTIONS
Add missing options to the list of allowed options for the
NODE_OPTIONS environment variable. Sort the list alphabetically.
doc,test: test documentation consistency for NODE_OPTIONS
Add a test that checks that the documented allowed options for the
NODE_OPTIONS environment variable are consistent with the actually
allowed options.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This is a draft/wip because it currently (legitimately) fails:

-bash-4.2$ ./node test/parallel/test-process-env-allowed-flags-are-documented.js
assert.js:89
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: The following options are not documented as allowed in NODE_OPTIONS in /home/users/riclau/sandbox/github/nodejs/doc/api/cli.md: --tls-min-v1.2 --input-type --experimental-worker --tls-min-v1.3 --debug-arraybuffer-allocations --tls-max-v1.2 --experimental-policy --preserve-symlinks --http-parser --tls-min-v1.0 --es-module-specifier-resolution --preserve-symlinks-main --tls-max-v1.3 --tls-min-v1.1 --prof-process -r --debug-port
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-process-env-allowed-flags-are-documented.js:35:8)
    at Module._compile (internal/modules/cjs/loader.js:777:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:788:10)
    at Module.load (internal/modules/cjs/loader.js:640:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:840:10)
    at internal/main/run_main_module.js:17:11 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: 17,
  expected: 0,
  operator: 'strictEqual'
}
-bash-4.2$

#28146 will fix the missing tls options. I'll add the other missing entries to the docs. If anyone sees anything in this list that shouldn't be there please say so.

This seems brittle. A line-wrap change could break it. And I'll definitely break it when I inevitably decide to reduce passive voice from the doc. But I don't have a better idea, so...¯\(ツ)/¯ (FWIW, I'd prefer if the doc said something like "Valid options are:". But obviously that can and likely should be some other PR some time.)

FWIW if we change the line in doc/api/cli.md to Valid options are: the test fails:

-bash-4.2$ ./node test/parallel/test-process-env-allowed-flags-are-documented.js
/home/users/riclau/sandbox/github/nodejs/test/parallel/test-process-env-allowed-flags-are-documented.js:17
const docOptions = cliDocs.match(docRegExp)[0];
                                           ^

TypeError: Cannot read property '0' of null
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-process-env-allowed-flags-are-documented.js:17:44)
    at Module._compile (internal/modules/cjs/loader.js:779:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:790:10)
    at Module.load (internal/modules/cjs/loader.js:642:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:842:10)
    at internal/main/run_main_module.js:17:11
-bash-4.2$

I'll see what we can do to make this better.

@Trott Changed this so that it now starts at the section header, which is less likely to change. PTAL.

(Test now still works when the prose is changed to "Valid options are:" but I'll let you make that change.)

Since HTML comments are valid Markdown, maybe we can delineate the list with a self-documenting HTML comment and use a RegExp to search for that? A suggestion, but totally not blocking this.

Added HTML comment markers. PTAL.

Of course now we have markers the follow on step would be to automatically update the list (since Node.js know which options are allowed in the environment and we've marked in the document where the lists should go). I'd prefer that to be a follow on PR 😁.

One more thought: we might also validate the correct documentation order.

Add missing options to the list of allowed options for the `NODE_OPTIONS` environment variable. Sort the list alphabetically.

Add a test that checks that the documented allowed options for the `NODE_OPTIONS` environment variable are consistent with the actually allowed options.

One more thought: we might also validate the correct documentation order.

Validate the options are sorted? Maybe as a follow up PR -- at the moment some options are listed with their aliases (e.g. --require, -r and --inspect-port, --debug-port) plus there are two lists (the Node.js options and V8 options) which means it's not quite as straightforward as it might seem at first.

@BridgeAR After changing the test and documentation to use markers to denote where to parse the options from the documentation this ended up being much easier so I've updated the test to check the options are alphabetically sorted.

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/23871/

A lint rule might be better?

A lint rule might be better?

The node binary running the linter may not be the same as the one corresponding to the source tree. However I won't stand in the way if anyone else wants to attempt this as a lint rule.

Landed in 3cdd5a2...5225586

Add missing options to the list of allowed options for the `NODE_OPTIONS` environment variable. Sort the list alphabetically. PR-URL: nodejs#28179 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a test that checks that the documented allowed options for the `NODE_OPTIONS` environment variable are consistent with the actually allowed options. PR-URL: nodejs#28179 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add missing options to the list of allowed options for the `NODE_OPTIONS` environment variable. Sort the list alphabetically. PR-URL: #28179 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a test that checks that the documented allowed options for the `NODE_OPTIONS` environment variable are consistent with the actually allowed options. PR-URL: #28179 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add missing options to the list of allowed options for the `NODE_OPTIONS` environment variable. Sort the list alphabetically. PR-URL: #28179 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a test that checks that the documented allowed options for the `NODE_OPTIONS` environment variable are consistent with the actually allowed options. PR-URL: #28179 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

richardlau added the wip Issues and PRs that are still a work in progress. label Jun 11, 2019

This comment has been minimized.

Sign in to view

richardlau added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Jun 11, 2019

richardlau force-pushed the docallowednodeoptions branch from e9202ca to 5557153 Compare June 12, 2019 01:47

richardlau removed the wip Issues and PRs that are still a work in progress. label Jun 12, 2019

richardlau marked this pull request as ready for review June 12, 2019 01:47

richardlau commented Jun 12, 2019

View reviewed changes

Comment thread doc/api/cli.md Outdated

Trott reviewed Jun 12, 2019

View reviewed changes

Trott approved these changes Jun 12, 2019

View reviewed changes

richardlau force-pushed the docallowednodeoptions branch from 5557153 to 8917655 Compare June 12, 2019 15:41

richardlau mentioned this pull request Jun 12, 2019

Bot does not post or edit comment on new Jenkins builds nodejs/github-bot#239

Closed

BridgeAR approved these changes Jun 12, 2019

View reviewed changes

Comment thread test/parallel/test-process-env-allowed-flags-are-documented.js Outdated

richardlau force-pushed the docallowednodeoptions branch from 8917655 to 7b41260 Compare June 12, 2019 17:25

This comment has been minimized.

Sign in to view

cjihrig approved these changes Jun 12, 2019

View reviewed changes

richardlau added 2 commits June 13, 2019 13:41

doc: add missing options allowed in NODE_OPTIONS

310d148

Add missing options to the list of allowed options for the `NODE_OPTIONS` environment variable. Sort the list alphabetically.

doc,test: test documentation consistency for NODE_OPTIONS

6bfc840

Add a test that checks that the documented allowed options for the `NODE_OPTIONS` environment variable are consistent with the actually allowed options.

richardlau force-pushed the docallowednodeoptions branch from 7b41260 to 6bfc840 Compare June 13, 2019 17:48

Trott approved these changes Jun 15, 2019

View reviewed changes

Trott closed this Jun 15, 2019

joyeecheung mentioned this pull request Jun 16, 2019

CI failures: 20190612-20190615 nodejs/reliability#23

Closed

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

kissofire mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities kissofire/node#73

Open

wedataintelligence mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities wedataintelligence/node#38

Open

ron-a20 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities ron-a20/node#71

Open

Dmarch28 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities Dmarch28/node#62

Open

aliceUnhinged613 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities aliceUnhinged613/node#64

Open

137717unity mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities 137717unity/node#62

Open

baby636 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities baby636/node#61

Open

molllyn1 mentioned this pull request Jul 23, 2022

[Snyk] Fix for 1 vulnerabilities molllyn1/node#53

Open

doc,test: test documentation consistency for NODE_OPTIONS by richardlau · Pull Request #28179 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc,test: test documentation consistency for NODE_OPTIONS - #28179

doc,test: test documentation consistency for NODE_OPTIONS#28179richardlau wants to merge 2 commits intonodejs:masternodejs/node:masterfrom richardlau:docallowednodeoptionsrichardlau/node-1:docallowednodeoptionsCopy head branch name to clipboard

Conversation

richardlau commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Jun 11, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Trott Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

richardlau Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

richardlau Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

Trott Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

richardlau Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR commented Jun 12, 2019

Uh oh!

This comment has been minimized.

richardlau commented Jun 13, 2019

Uh oh!

richardlau commented Jun 14, 2019

Uh oh!

jasnell commented Jun 14, 2019

Uh oh!

richardlau commented Jun 14, 2019

Uh oh!

Trott commented Jun 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

doc,test: test documentation consistency for NODE_OPTIONS#28179
richardlau wants to merge 2 commits into
nodejs:masterfrom
richardlau:docallowednodeoptions

richardlau commented Jun 11, 2019 •
edited
Loading

nodejs-github-bot commented Jun 11, 2019 •
edited
Loading