test: test make doc and verify toc#16208

joyeecheung · 2017-10-15T01:38:43Z

Fixes: nodejs/build#887

Checklist

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

Affected core subsystem(s)

test

joyeecheung · 2017-10-15T01:43:14Z

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

joyeecheung · 2017-10-15T01:51:23Z

test/sequential/test-make-doc.js

On a second thought...maybe we can just add doc-only to the dependency of make test and then only use this file to do verification?

joyeecheung · 2017-10-15T04:02:37Z

Ping @nodejs/testing @nodejs/documentation

Trott · 2017-10-15T04:32:49Z

Failures on FreeBSD seem relevant. E.g., https://ci.nodejs.org/job/node-test-commit-freebsd/12335/nodes=freebsd10-64/console:

not ok 1973 sequential/test-make-doc --- duration_ms: 0.193 severity: fail stack: |- stderr is not empty:  make[2]: illegal argument to -j -- must be positive integer! assert.js:709 assert.ifError = function ifError(err){if (err) throw err}; ^ Error: Command failed: make doc make[2]: illegal argument to -j -- must be positive integer! at ChildProcess.exithandler (child_process.js:269:12) at emitTwo (events.js:136:13) at ChildProcess.emit (events.js:227:7) at maybeClose (internal/child_process.js:944:16) at Socket.stream.socket.on (internal/child_process.js:364:11) at emitOne (events.js:126:13) at Socket.emit (events.js:224:7) at Pipe._handle.close [as _onclose] (net.js:551:12) ...

joyeecheung · 2017-10-15T05:13:50Z

@nodejs/build Is -jn required in make on FreeBSD CI? Should I hard-code it?

vsemozhetbyt · 2017-10-15T05:18:51Z

Is it worth to make this separable in a test suite and to update Makefile and vcbuild.bat to use in CI for doc only PRs? Or maybe to join with Linter CI into some light quick CI job?

vsemozhetbyt · 2017-10-15T05:20:55Z

test/sequential/test-make-doc.js

Maybe destructuring?

vsemozhetbyt · 2017-10-15T10:15:11Z

test/sequential/test-make-doc.js

assert.ok(docs.includes('_toc.html'));?

vsemozhetbyt · 2017-10-15T10:28:30Z

test/sequential/test-make-doc.js

ditto + the message?

vsemozhetbyt · 2017-10-15T10:34:38Z

test/sequential/test-make-doc.js

vsemozhetbyt · 2017-10-15T10:42:17Z

test/sequential/test-make-doc.js

Do we need a shell for make or execFile() suffices?

vsemozhetbyt · 2017-10-15T10:44:21Z

test/sequential/test-make-doc.js

console.error() here and in the next line? Or stderr should not be used in tests?

vsemozhetbyt

Thank you. Any suggestions are completely arbitrary)

cjihrig · 2017-10-15T14:15:40Z

test/sequential/test-make-doc.js

Wouldn't assert.strictEqual(stderr, ''); give pretty much the same results?

@cjihrig The stderr would be truncated by AssertionError to 128 chars, personally I prefer to keep it intact

Implement common.projectDir which points to the project directory.

joyeecheung · 2017-10-17T07:57:57Z

Tried to put the build step into Makefile to work around the BSD make. New CI: https://ci.nodejs.org/job/node-test-pull-request/10775/

joyeecheung · 2017-10-17T08:06:57Z

Oops, put doc-only in the wrong place. New CI: https://ci.nodejs.org/job/node-test-pull-request/10777/

joyeecheung · 2017-10-17T08:23:20Z

Hmm..putting make doc in build-ci or as a dependency of test-ci results in:

mkdir -p out/doc mkdir -p out/doc/api/ mkdir -p out/doc/api/assets make[1]: *** No rule to make target `out/doc/api/assets/sh_javascript.min.js', needed by `doc-only'. Stop.

I will try to investigate later. Also cc/ @nodejs/build if anyone knows what's going on with this error...

joyeecheung · 2017-10-17T10:30:43Z

There is a redundant slash in the definition of apidoc_dirs (out/doc/api/assets/ instead of out/doc/api/assets that has been defined). I wonder why it has never failed on my Linux box before...

Anyway, a new CI run looks close (https://ci.nodejs.org/job/node-test-commit/13219/), although there is a related error on node-test-commit-linux-fips, still investigating..

not ok 1945 sequential/test-make-doc --- duration_ms: 0.214 severity: fail stack: |- fs.js:930 return binding.readdir(pathModule.toNamespacedPath(path), options.encoding); ^ Error: ENOENT: no such file or directory, scandir '/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/out/doc/api' at Object.fs.readdirSync (fs.js:930:18) at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/sequential/test-make-doc.js:17:17) at Module._compile (module.js:601:30) at Object.Module._extensions..js (module.js:612:10) at Module.load (module.js:520:32) at tryModuleLoad (module.js:483:12) at Function.Module._load (module.js:475:3) at Function.Module.runMain (module.js:642:10) at startup (bootstrap_node.js:187:16) at bootstrap_node.js:605:3

joyeecheung · 2017-10-18T05:52:01Z

Addressed nits by @refack . CI before landing: https://ci.nodejs.org/job/node-test-pull-request/10804/

joyeecheung · 2017-10-18T10:10:52Z

There were multiple unrelated inspector tests errors that should hopefully fixed by #16281. Just to be safe: https://ci.nodejs.org/job/node-test-commit/13256/

Implement common.projectDir which points to the project directory. PR-URL: #16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

PR-URL: #16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

joyeecheung · 2017-10-18T12:31:25Z

CI failures are unrelated. Landed in e6dfd59...df5dc2d, thanks!

vsemozhetbyt · 2017-10-18T12:41:46Z

Should we make a rule to run full CI for doc PRs now? Or would this be overkill?

addaleax · 2017-10-18T12:47:22Z

@vsemozhetbyt I think we can do it optionally, if we want to make sure. The one thing we probably really should do is run make test after landing any PR?

(Also: @joyeecheung Awesome work! Didn’t see this PR until just now, but nice that we have this now.)

joyeecheung · 2017-10-18T12:50:15Z

@vsemozhetbyt Good question. I am thinking about a make test-doc and moving the doc validations into test/doc, then configure a job for this. This PR is mostly about trying to get the test script working first.

refack · 2017-10-18T12:52:58Z

Should we make a rule to run full CI for doc PRs now? Or would this be overkill?

AFAIK nodejs/build#887 and similars were caused by code changes in tools/doc, which now we have a way to validate 🎉

vsemozhetbyt · 2017-10-18T12:58:12Z

We can also consider joining linter CI and doc CI in a new CI job. Previously, we used linter CI also for benchmark PRs, but now we tend to make benchmarks fully testable, so the separate linter CI job remains useful only for the docs with code fragments. We can as well make these two CI joined in some special light CI job for docs.

refack · 2017-10-18T13:00:14Z

And there's the #12756 dream

Implement common.projectDir which points to the project directory. PR-URL: #16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

PR-URL: #16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Trott · 2017-10-18T18:21:52Z

test/common/index.js

 (cpus.length>1||cpus[0].speed>999);

 exports.rootDir=exports.isWindows ? 'c:\\' : '/';
+exports.projectDir=path.resolve(__dirname,'..','..');


I think this won't work on our cross-compiling Raspberry Pi devices in CI. @nodejs/build ?

Hmmmm...but it did pass in the CI, so...hmmm....

Oh, perhaps CI was run before that specific feature was added? I'm certainly seeing it fail 100% of the time in CI right now as in https://ci.nodejs.org/job/node-test-binary-arm/11032/.
not ok 272 sequential/test-make-doc --- duration_ms: 2.153 severity: fail stack: |- fs.js:926 return binding.readdir(pathModule.toNamespacedPath(path), options.encoding); ^ Error: ENOENT: no such file or directory, scandir '/home/iojs/build/workspace/node-test-binary-arm/out/doc/api' at Object.fs.readdirSync (fs.js:926:18) at Object.<anonymous> (/home/iojs/build/workspace/node-test-binary-arm/test/sequential/test-make-doc.js:15:17) at Module._compile (module.js:607:30) at Object.Module._extensions..js (module.js:618:10) at Module.load (module.js:526:32) at tryModuleLoad (module.js:489:12) at Function.Module._load (module.js:481:3) at Function.Module.runMain (module.js:648:10) at startup (bootstrap_node.js:191:16) at bootstrap_node.js:609:3 ...

I wonder if the problem isn't common.projectDir at all but instead that the Pi devices run make test-ci-js and not make test or make test-ci?

(...and so it skips the make doc....which we probably want it to skip because who knows how long it would take to run on a Pi and we'd be running it on all of them just for a test that only one of them runs...)

I think I've got a fix in #16301 but we'll see if CI agrees....

Hmm, I wonder why this passed the CI before...anyway thanks for the fix!

@joyeecheung I think it passed CI because this change wasn't in the branch when CI ran. It was run before a5262f1 landed.

Implement common.projectDir which points to the project directory. PR-URL: nodejs/node#16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

PR-URL: nodejs/node#16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-11-16T20:23:16Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

joyeecheung · 2017-11-17T07:12:14Z

@MylesBorins I'd say no if it does not land cleanly

PR-URL: nodejs/node#16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Implement common.projectDir which points to the project directory. PR-URL: nodejs/node#16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 15, 2017

joyeecheung commented Oct 15, 2017
View reviewed changes

vsemozhetbyt reviewed Oct 15, 2017
View reviewed changes

test/sequential/test-make-doc.js Outdated
Copy link
Contributor
vsemozhetbytOct 15, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe destructuring?

vsemozhetbyt reviewed Oct 15, 2017
View reviewed changes

test/sequential/test-make-doc.js Outdated
Copy link
Contributor
vsemozhetbytOct 15, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.ok(docs.includes('_toc.html'));?
thefourtheye reacted with thumbs up emoji

vsemozhetbyt reviewed Oct 15, 2017
View reviewed changes

test/sequential/test-make-doc.js Outdated
Copy link
Contributor
vsemozhetbytOct 15, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto + the message?

vsemozhetbyt reviewed Oct 15, 2017
View reviewed changes

test/sequential/test-make-doc.js Outdated
Copy link
Contributor
vsemozhetbytOct 15, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto?

vsemozhetbyt reviewed Oct 15, 2017
View reviewed changes

test/sequential/test-make-doc.js Outdated
Copy link
Contributor
vsemozhetbytOct 15, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a shell for make or execFile() suffices?

vsemozhetbyt reviewed Oct 15, 2017
View reviewed changes

test/sequential/test-make-doc.js Outdated
Copy link
Contributor
vsemozhetbytOct 15, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error() here and in the next line? Or stderr should not be used in tests?

vsemozhetbyt approved these changes Oct 15, 2017
View reviewed changes

cjihrig approved these changes Oct 15, 2017
View reviewed changes

vsemozhetbyt mentioned this pull request Oct 16, 2017
Feature request: check for CI runs nodejs/node-review#18
Closed

joyeecheung added 4 commits October 17, 2017 15:29

test: add common.projectDir
d8cc63b
Implement common.projectDir which points to the project directory.

test: test make doc and verify toc
43ce6da

doc projectDir
48e3b30

put the make doc part in Makefile
882172f

joyeecheung force-pushed the test-doc branch from 9d113da to 882172fCompare October 17, 2017 07:55

joyeecheung added 2 commits October 17, 2017 16:01

fix lint
2ff4a77

s/doc-only/doc/
271ff20

put make doc in build-ci
60c3c85

remove redundant slash
014e360

put doc in the dependency of test-ci
f678717

refack approved these changes Oct 18, 2017
View reviewed changes

joyeecheung closed this Oct 18, 2017

targos pushed a commit that referenced this pull request Oct 18, 2017
test: test make doc and verify toc
fefcd82
PR-URL: #16208Fixes: nodejs/build#887 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Trott reviewed Oct 18, 2017
View reviewed changes

cjihrig mentioned this pull request Oct 18, 2017
src: pass context to Get() operations for child processes #16247
Merged
2 tasks

This was referenced Oct 22, 2017
Request to join the build WG nodejs/build#927
Merged
A new CI job for linting and doc tests nodejs/build#929
Closed

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

joyeecheung added dont-land-on-v6.x and removed backport-requested-v6.x labels Nov 17, 2017

Uh oh!

test: test make doc and verify toc#16208

test: test make doc and verify toc #16208

Conversation

joyeecheung commented Oct 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Oct 15, 2017

Uh oh!

joyeecheungOct 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 15, 2017

Uh oh!

Trott commented Oct 15, 2017

Uh oh!

joyeecheung commented Oct 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 17, 2017

Uh oh!

joyeecheung commented Oct 17, 2017

Uh oh!

joyeecheung commented Oct 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Oct 17, 2017

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

vsemozhetbyt commented Oct 18, 2017

Uh oh!

addaleax commented Oct 18, 2017

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

refack commented Oct 18, 2017

Uh oh!

vsemozhetbyt commented Oct 18, 2017

Uh oh!

refack commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrottOct 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

joyeecheungOct 15, 2017•
edited
Loading

joyeecheung commented Oct 15, 2017•
edited
Loading

joyeecheung commented Oct 17, 2017•
edited
Loading

TrottOct 18, 2017•
edited
Loading

joyeecheungOct 19, 2017•
edited
Loading