node icon indicating copy to clipboard operation
node copied to clipboard

test,cli: generate man-page and use it for testing `CLI.md`

Open avivkeller opened this issue 1 year ago • 7 comments

This PR does two things but I believe they are related enough to be included in a single PR.

  1. This PR updates the test-cli-node-options-doc test to be more accurate, that is, the following:
  • Check that a --no- option is correctly documented under NODE_OPTIONS
  • Check that all options in cli.md are located in the node.1 file, excluding No-Ops (but they can optionally exist).
  1. This PR adds a tool to generate the node.1 file: This is related to 1 because, while working on it, I released that my updated tests were failing on over 50 flags, and many others had incorrect descriptions in the manfile. In order to make the manfile pass the tests, and be correct (description-wise), I added a tool to generate the file based on cli.md.

avivkeller avatar Oct 04 '24 21:10 avivkeller

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.41%. Comparing base (7b5d660) to head (71fe8c7). Report is 81 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55268      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         653      654       +1     
  Lines      187476   187552      +76     
  Branches    36083    36081       -2     
==========================================
+ Hits       165763   165826      +63     
- Misses      14946    14959      +13     
  Partials     6767     6767              

see 25 files with indirect coverage changes

codecov[bot] avatar Oct 04 '24 23:10 codecov[bot]

Can you attach comparison of the result before and after?

ovflowd avatar Oct 14 '24 11:10 ovflowd

What makes this PR hard to review are the unknowns and little documentation, which in the end creates a "trust me, bro" sentiment since (at least I, as a reviewer) was given very little to be able to review these changes. It lacks more context and references to what is relevant for me to verify that it is working as expected.

+ It'd be great to have unit testing of the individual domain logic pieces of your tooling (extract them from the main function, into separate smaller functions, which allow us to better understand their in-and-outs)

I'd also love some JSDoc here, that would also be very helpful :)

ovflowd avatar Oct 14 '24 11:10 ovflowd

Thanks for the detailed review!

I hadn't even thought of it before, but I think you're right that api-docs-tooling is a nice home for this, because as you said: all the processors are already there. I'm gonna work on implementing this there, so all these string manipulations won't need to happen as much. If that's cleaner, I'll move all of this there.

What makes this PR hard to review are the unknowns and little documentation, which in the end creates a "trust me, bro" sentiment since (at least I, as a reviewer) was given very little to be able to review these changes. It lacks more context and references to what is relevant for me to verify that it is working as expected.

Yes, I should add documentation...


Moving to draft until: (A) Compared with a api-docs-tooling implemention (B) JSDoc / Docs added (C) Reduce the unknown variables

avivkeller avatar Oct 14 '24 11:10 avivkeller

Appreciate you!

ovflowd avatar Oct 14 '24 11:10 ovflowd

Ditto back to you :-)

avivkeller avatar Oct 14 '24 11:10 avivkeller

See https://github.com/nodejs/api-docs-tooling/pull/125, which implements this using the already established parsers, allowing for more details (more than 1 sentence) for the mandoc. Leaving this as a draft in case that is a no-go.

avivkeller avatar Oct 14 '24 16:10 avivkeller

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

nodejs-github-bot avatar Oct 21 '24 22:10 nodejs-github-bot

I'm concerned that contributors may forget to regenerate this file when they modify the CLI file, so I'll add a network test (network to download api-docs-tooling) to ensure it's properly generated.

avivkeller avatar Oct 21 '24 23:10 avivkeller

@nodejs/build this causes lint-md to use internet access, but not fail if the user does not have it. Is that something that the build team is okay with? #55266 seemed to be okay with it, and it required internet access.

avivkeller avatar Oct 24 '24 21:10 avivkeller

There haven't been any objections in a few days to my comment above, if a new CI is started, can this land anytime soon?

avivkeller avatar Oct 29 '24 17:10 avivkeller

Ack! The commits got mixed up.

avivkeller avatar Nov 16 '24 17:11 avivkeller

I'm going to recreate this PR.

avivkeller avatar Nov 16 '24 17:11 avivkeller