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

richardlau · 2019-06-11T18:21:35Z

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

nodejs-github-bot · 2019-06-11T18:21:38Z

richardlau · 2019-06-11T18:26:32Z

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

-bash-4.2$ ./node test/parallel/test-process-env-allowed-flags-are-documented.jsassert.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.

Trott · 2019-06-12T04:58:17Z

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:17const 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 😁.

BridgeAR · 2019-06-12T17:36:15Z

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.

richardlau · 2019-06-13T17:54:06Z

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.

richardlau · 2019-06-14T00:27:41Z

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

jasnell · 2019-06-14T03:20:26Z

A lint rule might be better?

richardlau · 2019-06-14T03:40:23Z

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.

Trott · 2019-06-15T03:47:53Z

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 5557153Compare 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 threaddoc/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 8917655Compare 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 threadtest/parallel/test-process-env-allowed-flags-are-documented.js Outdated

richardlau force-pushed the docallowednodeoptions branch from 8917655 to 7b41260Compare 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 6bfc840Compare 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

Uh oh!

Uh oh!

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!

TrottJun 12, 2019

Choose a reason for hiding this comment

Uh oh!

richardlauJun 12, 2019

Choose a reason for hiding this comment

Uh oh!

richardlauJun 12, 2019

Choose a reason for hiding this comment

Uh oh!

TrottJun 12, 2019

Choose a reason for hiding this comment

Uh oh!

richardlauJun 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

richardlau commented Jun 11, 2019•
edited
Loading

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