-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
test: add some validation for JSON doc output #61413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| import * as common from '../common/index.mjs'; | ||
|
|
||
| import assert from 'node:assert'; | ||
| import { existsSync } from 'node:fs'; | ||
| import fs from 'node:fs/promises'; | ||
| import path from 'node:path'; | ||
|
|
||
| // This tests that `make doc` generates the JSON documentation properly. | ||
| // Note that for this test to pass, `make doc` must be run first. | ||
|
|
||
| if (common.isWindows) { | ||
| common.skip('`make doc` does not run on Windows'); | ||
| } | ||
|
|
||
| function validateModule(module) { | ||
| assert.strictEqual(typeof module, 'object'); | ||
| assert.strictEqual(module.type, 'module'); | ||
| assert.ok(module.name); | ||
| assert.ok(module.textRaw); | ||
| } | ||
|
|
||
| function validateMisc(misc) { | ||
| assert.strictEqual(typeof misc, 'object'); | ||
| assert.strictEqual(misc.type, 'misc'); | ||
| assert.ok(misc.name); | ||
| assert.strictEqual(typeof misc.name, 'string'); | ||
| assert.ok(misc.textRaw); | ||
| assert.strictEqual(typeof misc.textRaw, 'string'); | ||
| } | ||
|
|
||
| let numberOfDeprecatedSections = 0; | ||
| let numberOfRemovedAPIs = 0; | ||
|
|
||
| const metaExpectedKeys = new Set([ | ||
| 'added', | ||
| 'changes', | ||
| 'deprecated', | ||
| 'napiVersion', | ||
| 'removed', | ||
| ]); | ||
|
|
||
| function validateMeta(meta) { | ||
| assert.partialDeepStrictEqual(metaExpectedKeys, new Set(Object.keys(meta))); | ||
| assert.ok(!Object.hasOwn(meta, 'added') || Array.isArray(meta.added) || typeof meta.added === 'string'); | ||
| if (meta.deprecated) { | ||
| numberOfDeprecatedSections++; | ||
| assert.ok(Array.isArray(meta.deprecated) || typeof meta.deprecated === 'string'); | ||
| } | ||
| if (meta.removed) { | ||
| numberOfRemovedAPIs++; | ||
| assert.ok(Array.isArray(meta.removed) || typeof meta.removed === 'string'); | ||
| } | ||
|
|
||
| assert.ok(!Object.hasOwn(meta, 'napiVersion') || Number(meta.napiVersion)); | ||
| assert.ok(Array.isArray(meta.changes)); | ||
| } | ||
|
|
||
| function findAllKeys(obj, allKeys = new Set()) { | ||
| for (const [key, value] of Object.entries(obj)) { | ||
| if (Number.isNaN(Number(key))) allKeys.add(key); | ||
| if (typeof value === 'object') findAllKeys(value, allKeys); | ||
|
|
||
| if (key === 'miscs') { | ||
| assert.ok(Array.isArray(value)); | ||
| assert.ok(value.length); | ||
| value.forEach(validateMisc); | ||
| } else if (key === 'modules') { | ||
| assert.ok(Array.isArray(value)); | ||
| assert.ok(value.length); | ||
| value.forEach(validateModule); | ||
| } else if (key === 'meta') { | ||
| validateMeta(value); | ||
| } | ||
| } | ||
| return allKeys; | ||
| } | ||
|
|
||
| const allExpectedKeys = new Set([ | ||
| 'added', | ||
| 'changes', | ||
| 'classes', | ||
| 'classMethods', | ||
| 'commit', | ||
| 'ctors', | ||
| 'default', | ||
| 'deprecated', | ||
| 'desc', | ||
| 'description', | ||
| 'displayName', | ||
| 'events', | ||
| 'examples', | ||
| 'globals', | ||
| 'introduced_in', | ||
| 'meta', | ||
| 'methods', | ||
| 'miscs', | ||
| 'modules', | ||
| 'name', | ||
| 'napiVersion', | ||
| 'options', | ||
| 'params', | ||
| 'pr-url', | ||
| 'properties', | ||
| 'removed', | ||
| 'return', | ||
| 'shortDesc', | ||
| 'signatures', | ||
| 'source', | ||
| 'stability', | ||
| 'stabilityText', | ||
| 'textRaw', | ||
| 'type', | ||
| 'version', | ||
| ]); | ||
|
|
||
| for await (const dirent of await fs.opendir(new URL('../../out/doc/api/', import.meta.url))) { | ||
| if (!dirent.name.endsWith('.md')) continue; | ||
|
|
||
| const jsonPath = path.join(dirent.parentPath, dirent.name.slice(0, -2) + 'json'); | ||
| const expectedSource = `doc/api/${dirent.name}`; | ||
| if (dirent.name === 'quic.md') { | ||
| assert.ok(!existsSync(jsonPath)); // QUIC documentation is not public yet | ||
| continue; | ||
| } | ||
|
|
||
| console.log('testing', jsonPath, 'based on', expectedSource); | ||
|
|
||
| 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); | ||
|
Comment on lines
+128
to
+131
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this check, it seems unnecessary, as this is a JSON file.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, we don't review the generated JSON manually during PRs, right? It's not even on git diffs, afaik?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be surprised if that was the case, empty spaces are very compressible, gzipped size should be almost the same |
||
|
|
||
| const json = JSON.parse(fileContent); | ||
|
|
||
| assert.strictEqual(json.type, 'module'); | ||
| assert.strictEqual(json.source, expectedSource); | ||
| 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', ...({ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this change over time as more api files are added? Should we document that somewhere? |
||
| 'addons.md': ['introduced_in', 'miscs'], | ||
| 'cli.md': ['introduced_in', 'miscs'], | ||
| 'debugger.md': ['introduced_in', 'stability', 'stabilityText', 'miscs'], | ||
| 'deprecations.md': ['introduced_in', 'miscs'], | ||
| 'documentation.md': ['introduced_in', 'miscs'], | ||
| 'errors.md': ['introduced_in', 'classes', 'miscs'], | ||
| 'esm.md': ['introduced_in', 'meta', 'stability', 'stabilityText', 'properties', 'miscs'], | ||
| 'globals.md': ['introduced_in', 'stability', 'stabilityText', 'classes', 'methods', 'miscs'], | ||
| 'index.md': [], | ||
| 'intl.md': ['introduced_in', 'miscs'], | ||
| 'n-api.md': ['introduced_in', 'stability', 'stabilityText', 'miscs'], | ||
| 'packages.md': ['introduced_in', 'meta', 'miscs'], | ||
| 'process.md': ['globals'], | ||
| 'report.md': ['introduced_in', 'stability', 'stabilityText', 'meta', 'miscs'], | ||
| }[dirent.name] ?? ['modules'])]); | ||
|
|
||
| assert.partialDeepStrictEqual(allExpectedKeys, findAllKeys(json)); | ||
| } | ||
|
|
||
| assert.strictEqual(numberOfDeprecatedSections, 39); // Increase this number every time a new API is deprecated. | ||
| assert.strictEqual(numberOfRemovedAPIs, 46); // Increase this number every time a section is marked as removed. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know
make docdoesn't run on Windows too, doc-kit should work on Windows tho. But I assume it is skipped on Windows CI.