test: add some validation for JSON doc output #61413
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61413 +/- ##
==========================================
- Coverage 88.53% 88.52% -0.02%
==========================================
Files 704 704
Lines 208876 208883 +7
Branches 40330 40338 +8
==========================================
- Hits 184937 184904 -33
- Misses 15922 15969 +47
+ Partials 8017 8010 -7 🚀 New features to boost your workflow:
|
| if (dirent.name !== 'index.md') { | ||
| assert.ok(json.introduced_in || Object.values(json).at(-1)?.[0].introduced_in); | ||
| } | ||
| assert.deepStrictEqual(Object.keys(json), ['type', 'source', ...({ |
There was a problem hiding this comment.
Will this change over time as more api files are added? Should we document that somewhere?
| const fileContent = await fs.readFile(jsonPath, 'utf8'); | ||
| // A proxy to check if the file is human readable is to count if it contains | ||
| // at least 3 line return. | ||
| assert.strictEqual(fileContent.split('\n', 3).length, 3); |
There was a problem hiding this comment.
I don't like this check, it seems unnecessary, as this is a JSON file.
There was a problem hiding this comment.
We can agree to disagree. Having the output JSON human readable is very much useful to review it. (If you meant that line returns are not a good proxy for human readable, I'm open to suggestions)
There was a problem hiding this comment.
I mean, we don't review the generated JSON manually during PRs, right? It's not even on git diffs, afaik?
There was a problem hiding this comment.
to my understanding, not prettyifying them saves lots of network bandwidth and decreases file size by 70-80%, I think you can still use browsers or tools that automatically prettify'em if you want to visualize the JSON manually?
There was a problem hiding this comment.
I'd be surprised if that was the case, empty spaces are very compressible, gzipped size should be almost the same
| if (common.isWindows) { | ||
| common.skip('`make doc` does not run on Windows'); | ||
| } |
There was a problem hiding this comment.
Instead, can we check if the files exist, because if this test is run on Windows in a space where those files do exist, IMO we should test them
There was a problem hiding this comment.
I took it from
node/test/doctool/test-make-doc.mjs
Line 8 in bc9ffc3
We should keep both consistent IMO
There was a problem hiding this comment.
I didn't know make doc doesn't run on Windows too, doc-kit should work on Windows tho. But I assume it is skipped on Windows CI.
6 participants
We don't have any validation for the JSON output of the docs, errors can easily slip up there. Those tests become more important to gain confidence to switch a different system to generate those.
It's also a way to enforce consistency as demonstrated by the Markdown change in this PR.