build: make test-addons dependency-free - #62388
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
cc @nodejs/build |
Sorry, something went wrong.
|
cc @nodejs/web-infra this makes our addon-verify generator obsolete, right? |
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #62388 +/- ##
==========================================
- Coverage 89.68% 89.67% -0.01%
==========================================
Files 676 676
Lines 206738 206738
Branches 39594 39602 +8
==========================================
- Hits 185414 185402 -12
- Misses 13467 13474 +7
- Partials 7857 7862 +5 see 33 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
ovflowd
left a comment
There was a problem hiding this comment.
I don't know how I feel about this, it moves back doc-related tooling to node/core, although we could argue this isn't really related to doc generation, doc-kit is also handling things such as node.1 generation and other things related to docs.
So it creates a gray area of... what should doc-kit take care of?
I'm ultimately fine with this change, but if merged would make the generator on doc-kit obsolete. Which is also fine.
We just need to be mindful this brings maintenance burden back to Node core to this particular thing AND requires also addons.md to contain these special "<-- addon-verfiy-file" syntax, which is at the very least... Janky.
I also believe, @joyeecheung you'd need to update contributor documentation (doc section) explaining what this addon-verify-file block is and document this script.
Sorry, something went wrong.
I think it's much higher maintenance burden if the building of the functional tests rely on an external dependency. For example, suppose the generated gyp files no longer work due to compiler toolchain support changes? Suppose the test harness changes? Suppose we are changing the build files and altering the build output layout to alter cache behavior? Suppose we want to finally support an alternative addon toolchain other than gyp? None of these are related to documentation, but instead of updating it in Node.js core which is where the other parts change and where this can actually be tested, this will need to be updated in doc-kit which is just weird and won't give the builds a proper test. With a script in this PR we can simply update a couple of lines and run Jenkins and be done. With it in doc-kit we need to modify it in isolation and go through multiple rounds of back and forth between the two repo it doesn't work across all platforms, that's much higher cost. At the very least the tooling should not do a full on parse of the markdown or run minifier or highlighter to extract these blocks, that was a hacky heuristic in the first place no matter where this is implemented, which IMO should've been fixed long before doc-kit was a thing. And once you strip those hacks away, there just isn't much of a point to keep it in another toolchain that doesn't correlate that much with building Node.js binary per-se. PRs like nodejs/doc-kit#691 shouldn't be necessary at all. The fact that someone needs to investigate why make test-only fails on riscv64 due to a markdown highlighter's dependency on wasm is already indicator that it's high maintainance burden. |
Sorry, something went wrong.
aduh95
left a comment
There was a problem hiding this comment.
Can you chmod +x tools/doc/addon-verify.mjs and fix the lint errors? Otherwise LGTM
Sorry, something went wrong.
`make test-addons` used to depend on a markdown parser and then doc-kit to extract C++ addon examples from addons.md by guessing the file contents based on headings. This is hacky and brittle. The introduction of doc-kit also means tests intended for verifying the binary like `make test-only` now need to support doc-building toolchains e.g. minifier, highlighter, and indirect dependencies that rely on prebuilt-addon/wasm, which defeats the purpose and makes it harder to run for experimental platforms. This patch adds explicit `<!-- addon-verify-file dir/filename -->` markers in addons.md to locate extractable code blocks, avoiding fragile heuristics based on heading text or code block order and eliminating the dependency with simpler parsing. Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Fixed it and I squashed the commits to add the Co-Authored-By field. @aduh95 can you take a look again? Thanks! |
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
`make test-addons` used to depend on a markdown parser and then doc-kit to extract C++ addon examples from addons.md by guessing the file contents based on headings. This is hacky and brittle. The introduction of doc-kit also means tests intended for verifying the binary like `make test-only` now need to support doc-building toolchains e.g. minifier, highlighter, and indirect dependencies that rely on prebuilt-addon/wasm, which defeats the purpose and makes it harder to run for experimental platforms. This patch adds explicit `<!-- addon-verify-file dir/filename -->` markers in addons.md to locate extractable code blocks, avoiding fragile heuristics based on heading text or code block order and eliminating the dependency with simpler parsing. Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #62388 Fixes: #62385 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com>
|
This doesn't land cleanly on v24.x-staging and would require a manual backport PR if we want it there |
Sorry, something went wrong.
make test-addons used to depend on a markdown parser and then doc-kit to extract C++ addon examples from addons.md by guessing the file contents based on headings. This is hacky and brittle. The introduction of doc-kit also means tests intended for verifying the binary like make test-only now need to support doc-building toolchains e.g. minifier, highlighter, and indirect dependencies that rely on prebuilt-addon/wasm, which defeats the purpose and makes it harder to run for experimental platforms.
This patch adds explicit
<!-- addon-verify-file dir/filename --> markers in addons.md to locate extractable code blocks, avoiding fragile heuristics based on heading text or code block order and eliminating the dependency with simpler parsing.
Fixes: #62385