Skip to content

test: test make doc and verify toc #16208

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

Closed
wants to merge 11 commits into from

Conversation

joyeecheung
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 15, 2017
@joyeecheung
Copy link
Member Author

}
}

exec('make doc', {
Copy link
Member Author

@joyeecheung joyeecheung Oct 15, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Ping @nodejs/testing @nodejs/documentation

@Trott
Copy link
Member

Trott commented Oct 15, 2017

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
Copy link
Member Author

joyeecheung commented Oct 15, 2017

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

@vsemozhetbyt
Copy link
Contributor

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?

const common = require('../common');

const assert = require('assert');
const exec = require('child_process').exec;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe destructuring?

function verifyToc() {
const apiPath = path.join(common.projectDir, 'out', 'doc', 'api');
const docs = fs.readdirSync(apiPath);
assert.notStrictEqual(docs.indexOf('_toc.html'), -1);
Copy link
Contributor

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'));?


const linkedHtmls = links.map((link) => link.match(re)[1]);
for (const html of linkedHtmls) {
assert.notStrictEqual(docs.indexOf(html), -1, `${html} does not exist`);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto + the message?


for (const html of generatedHtmls) {
assert.notStrictEqual(
linkedHtmls.indexOf(html),
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto?

}
}

exec('make doc', {
Copy link
Contributor

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?

}, common.mustCall(function onExit(err, stdout, stderr) {
console.log(stdout);
if (stderr.length > 0) {
console.log('stderr is not empty: ');
Copy link
Contributor

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?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Thank you. Any suggestions are completely arbitrary)

cwd: common.projectDir
}, common.mustCall(function onExit(err, stdout, stderr) {
console.log(stdout);
if (stderr.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@joyeecheung
Copy link
Member Author

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
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 17, 2017

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
Copy link
Member Author

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
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

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/

joyeecheung added a commit that referenced this pull request Oct 18, 2017
Implement common.projectDir which points to the project directory.

PR-URL: #16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
joyeecheung added a commit that referenced this pull request Oct 18, 2017
PR-URL: #16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@joyeecheung
Copy link
Member Author

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

@vsemozhetbyt
Copy link
Contributor

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

@addaleax
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Contributor

refack commented Oct 18, 2017

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
Copy link
Contributor

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
Copy link
Contributor

refack commented Oct 18, 2017

And there's the #12756 dream

targos pushed a commit that referenced this pull request Oct 18, 2017
Implement common.projectDir which points to the project directory.

PR-URL: #16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@@ -59,6 +59,8 @@ exports.enoughTestCpu = Array.isArray(cpus) &&
(cpus.length > 1 || cpus[0].speed > 999);

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@Trott Trott Oct 18, 2017

Choose a reason for hiding this comment

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

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
  ...

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

(...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...)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@joyeecheung joyeecheung Oct 19, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

addaleax pushed a commit to ayojs/ayo that referenced this pull request 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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins
Copy link
Contributor

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
Copy link
Member Author

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

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#16208
Fixes: nodejs/build#887
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request 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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-test-commit: run make doc during tests
8 participants