FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: amplify and optimize doctool/test-make-doc by vsemozhetbyt · Pull Request #19581 · nodejs/node · GitHub

/ node Public

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

Closed
vsemozhetbyt wants to merge 2 commits into
nodejs:masterfrom
vsemozhetbyt:test-doctool-test-make-doc
ConversationCommits (2)ChecksFiles changed
Closed

test: amplify and optimize doctool/test-make-doc#19581
vsemozhetbyt wants to merge 2 commits into
nodejs:masterfrom
vsemozhetbyt:test-doctool-test-make-doc

Conversation

vsemozhetbyt commented Mar 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor
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 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 commented Mar 25, 2018

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13855/

vsemozhetbyt commented Mar 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

One ARM fail seems doc unrelated.

Trott commented Mar 25, 2018

Copy link
Copy Markdown
Member

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

BridgeAR approved these changes Mar 25, 2018
View reviewed changes
Comment thread test/doctool/test-make-doc.js
assert.ok(
fs.statSync(path.join(apiPath, actualDoc)).size !== 0,
`${actualDoc} is empty`
);

BridgeAR Mar 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vsemozhetbyt Mar 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

vsemozhetbyt commented Mar 25, 2018

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13862/

vsemozhetbyt commented Mar 25, 2018

Copy link
Copy Markdown
Contributor Author

The same ARM fail seems doc unrelated again.

Trott commented Mar 26, 2018

Copy link
Copy Markdown
Member

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

vsemozhetbyt commented Mar 26, 2018

Copy link
Copy Markdown
Contributor Author

ARM-fanned all green.

vsemozhetbyt added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 26, 2018
vsemozhetbyt added a commit that referenced this pull request Mar 28, 2018
test: amplify and optimize doctool/test-make-doc
b5884fb 
PR-URL: #19581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

vsemozhetbyt commented Mar 28, 2018

Copy link
Copy Markdown
Contributor Author

Landed in b5884fb

vsemozhetbyt deleted the test-doctool-test-make-doc branch March 28, 2018 00:32
mcollina mentioned this pull request Mar 30, 2018
Closed
targos pushed a commit that referenced this pull request Apr 2, 2018
test: amplify and optimize doctool/test-make-doc
0daa063 
PR-URL: #19581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos mentioned this pull request Apr 4, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL