Skip to content
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

test: amplify and optimize doctool/test-make-doc #19581

Closed
wants to merge 2 commits into from
Closed

test: amplify and optimize doctool/test-make-doc #19581

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Mar 25, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Currently, the doctool/test-make-doc.js has 2 tiny performance nits and 2 logic oversights.

Performance nits

  1. The list of doc files is filtered too late, after some usage with unneeded files burden.
  2. The list of links in TOC is not filtered at all, while 48 links from 98 ones are duplicate.

Logic oversights.

  1. JSON docs generation is not tested, while it is possible that JSON generation is aborted after all the HTML files are created.
  2. Makefile generates docs by getting the console.log() output from the tools/doc/generate.js and redirecting it to the doc files. If tools/doc/generate.js throws, an empty doc file is still created. So we can have a situation when the last .md source has an error and all the needed files are nevertheless present, just one of them is empty. So we need to check if all the files are not empty.

Refactoring strategy

  1. Filter out unneeded files as soon as possible.
  2. Filter out duplicate links.
  3. Include JSON files in generation tests.
  4. Check that all the actual files are not empty.

The test is changed considerably, but it may be more strict, full and performant in this form.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Mar 25, 2018
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 25, 2018

One ARM fail seems doc unrelated.

@Trott
Copy link
Member

Trott commented Mar 25, 2018

ARM re-run: https://ci.nodejs.org/job/node-test-commit-arm-fanned/15393/

assert.ok(
fs.statSync(path.join(apiPath, actualDoc)).size !== 0,
`${actualDoc} is empty`
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could actually be combined with the upper loop. That way the loop has to run only once instead of twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

The same ARM fail seems doc unrelated again.

@Trott
Copy link
Member

Trott commented Mar 26, 2018

ARM-fanned re-run: https://ci.nodejs.org/job/node-test-commit-arm-fanned/15425/

@vsemozhetbyt
Copy link
Contributor Author

ARM-fanned all green.

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 26, 2018
vsemozhetbyt added a commit that referenced this pull request Mar 28, 2018
PR-URL: #19581
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@vsemozhetbyt
Copy link
Contributor Author

Landed in b5884fb

@vsemozhetbyt vsemozhetbyt deleted the test-doctool-test-make-doc branch March 28, 2018 00:32
targos pushed a commit that referenced this pull request Apr 2, 2018
PR-URL: #19581
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants