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

node-test-commit: run make doc during tests #887

Closed
maclover7 opened this issue Sep 17, 2017 · 4 comments
Closed

node-test-commit: run make doc during tests #887

maclover7 opened this issue Sep 17, 2017 · 4 comments

Comments

@maclover7
Copy link
Contributor

Running make doc (and making sure the exit status is 0) can help with nodejs/node#15447. I think the chances a YAML error would be introduced are low, but it would be great to catch this via CI as an early warning system, and not via checking the nightly doc builds :)

cc @refack @vsemozhetbyt

@vsemozhetbyt
Copy link

Refs: nodejs/node#14930 (2 cases)

@refack
Copy link
Contributor

refack commented Sep 17, 2017

@maclover7 thanks for following up on this.

  1. IMHO this should be done in code, as a test in /test/sequential so that we could not only check sanity, but also validate properties of the resulted docs.
  2. Meanwhile I see no harm in adding make doc-only to node-test-linter

@maclover7
Copy link
Contributor Author

Looks like this happened again (see nodejs/node#14930 (comment) for details). Personally, I still think compiling docs via node-test-commit (or node-test-linter) is the best solution. I see @gibfahn and @mhdawson gave a +1 to @refack's comment above... what's the best way to move forward here?

@gibfahn
Copy link
Member

gibfahn commented Oct 6, 2017

what's the best way to move forward here?

So assuming we're doing this:

IMHO this should be done in code, as a test in /test/sequential so that we could not only check sanity, but also validate properties of the resulted docs.

that would just be a PR to nodejs/node that added a test in test/sequential/ that runs make doc and verifies that things work okay.

joyeecheung added a commit to nodejs/node that referenced this issue Oct 18, 2017
PR-URL: #16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Oct 18, 2017
Implement common.projectDir which points to the project directory.

PR-URL: #16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Oct 18, 2017
PR-URL: #16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
Implement common.projectDir which points to the project directory.

PR-URL: nodejs/node#16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
PR-URL: nodejs/node#16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
PR-URL: nodejs/node#16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
Implement common.projectDir which points to the project directory.

PR-URL: nodejs/node#16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants