build: make test-addons dependency-free#62388
build: make test-addons dependency-free#62388joyeecheung wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
|
cc @nodejs/build |
3d0aa10 to
2c71036
Compare
|
cc @nodejs/web-infra this makes our addon-verify generator obsolete, right? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62388 +/- ##
==========================================
- Coverage 91.60% 89.67% -1.93%
==========================================
Files 337 676 +339
Lines 140745 206693 +65948
Branches 21802 39582 +17780
==========================================
+ Hits 128925 185360 +56435
- Misses 11595 13462 +1867
- Partials 225 7871 +7646 🚀 New features to boost your workflow:
|
2c71036 to
ba7d859
Compare
`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.
ba7d859 to
9d62d0b
Compare
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.
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-addonsused 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 likemake test-onlynow 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