Test doctool #6031

iankronquist · 2016-04-04T00:45:18Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)? (N/A)

Affected core subsystem(s)

tools/doc

Description of change

Add tests for tools/doc. Fixes #5955. Includes tests for the functions toHTML (in html.js) and doJSON (in json.js). Also includes a regression test for #5873. Also introduces a new make target to run the tests.

I expect to be asked to squash these commits.

MylesBorins · 2016-04-04T00:51:09Z

Should these tests be including in the tests/ dir rather than docs/? /cc @nodejs/documentation @nodejs/testing

Are we going to want to run these in ci? /cc @nodejs/build

iankronquist · 2016-04-04T00:54:58Z

@thealphanerd I put them there because tools/doc is its own independent package which could be distributed separately from node code.
These new tests run if you do the make test target, so presumably they would run during CI.

addaleax · 2016-04-04T00:55:08Z

tools/doc/tests/test_json.js

+
+        if (err) throw err;
+        assert(output.source == 'foo');
+        assert(JSON.stringify(output) == JSON.stringify(item.json));


Any reason not to use assert.equal(output.source, 'foo'), assert.deepStrictEqual(output, item.json)?

Because I didn't know those existed. Let me fix that.

stevemao · 2016-04-04T03:33:14Z

Are we going to want to run these in ci?

Should definitely run in ci.

Should these tests be including in the tests/ dir rather than docs/?

I think these tests should be in docs/

Trott · 2016-04-04T04:49:30Z

This PR adds test-doc to Makefile but it also needs to be added to vcbuild.bat for Windows.

Trott · 2016-04-04T04:55:39Z

Should these tests be including in the tests/ dir rather than docs/?

Yes. Tests should go in the test directory.

As far as where in the test directory: They could go in a new directory like test/doc but oddball directories like that tend to be test graveyards (whassup test/debugger and test/pummel?). It would probably be just fine in test/parallel. Then it would run automatically in CI and if we ever introduce a change that breaks the doc tools, these tests might actually catch it before the change lands.

Trott · 2016-04-04T04:57:39Z

I put them there because tools/doc is its own independent package which could be distributed separately from node code.

I see the logic there and wouldn't stand in the way if others agreed. I look at it this way, though: All tests for the project are in the test directory and I really don't want to have to remember that there's this other oddball set of tests that is somewhere else. That just seems like cognitive overhead we can avoid.

Trott · 2016-04-04T04:58:11Z

These new tests run if you do the make test target, so presumably they would run during CI.

CI runs the make test-ci job, not make test. So you need to add it to that job. Also, it needs to be added to test-ci in vcbuild.bat to run on Windows.

Trott · 2016-04-04T05:01:03Z

Another argument for adding these to the project-wide test directory: Then they get linted with the lint rules for our tests rather than the lint rules for the doc directory. These tests don't load the common module the way all our tests are required to (because that module is what we use to detect leaked globals).

EDIT: Putting them in test/parallel also means that we can get TAP output from test.py and we can run stress tests in CI if we ever have a flaky test for the doctool.

Trott · 2016-04-04T05:03:19Z

Last comment for now, I promise, sorry for the storm of notifications: YES YES YES! +100 to having some tests for the doc tools.

iankronquist · 2016-04-04T05:25:41Z

@Trott I don't have access to a windows system. I could put some code in vcbuild.bat and hope that it works if you'd like.

Trott · 2016-04-04T05:39:30Z

I don't have access to a windows system. I could put some code in vcbuild.bat and hope that it works if you'd like.

One more benefit to putting these tests in test/parallel: You won't need any changes at all to Makefile or vcbuild.bat. Everything will just work. The tests will run in CI on all platforms.

iankronquist · 2016-04-04T05:45:54Z

@Trott here's a version with the tests living under test/
iankronquist/io.js@test-doctool...iankronquist:test-doctool-tests

Trott · 2016-04-04T05:53:48Z

here's a version with the tests living under test/

💯 😍 👍

Trott · 2016-04-04T05:55:43Z

@thealphanerd @jasnell I'm adding the lts-watch-v4.x label to this PR. Apologies in advance if that's misguided. Seems like it should be brought into LTS if it lands cleanly.

Trott · 2016-04-04T05:57:00Z

tools/doc/tests/test_html.js

@@ -0,0 +1,50 @@
+'use strict';
+
+var assert = require('assert');


Nit (which means you can ignore this comment if you want to): The project has been favoring const over var for module imports like this.

Knighton910 · 2016-04-04T07:10:53Z

👍

eljefedelrodeodeljefe · 2016-04-04T07:32:09Z

I think that is pretty much what @silverwind intended. Good stuff.

evanlucas · 2016-04-04T10:51:15Z

tools/doc/tests/test_html.js

+  });
+}
+
+test_toHTML();


Can we not wrap this in a function. With this being the only thing in the file, it's a bit redundant.

evanlucas · 2016-04-04T10:54:50Z

Thanks for the contribution! I'm with @Trott. Please make every variable const that can be. I think this is great though. I would really like to see us add support for verifying there are no broken link references, but that is something that can happen after the initial version gets merged.

I would prefer that we keep these in the test/ directory.

evanlucas · 2016-04-04T10:55:43Z

tools/doc/tests/test_html.js

+  // Test data is a list of objects with two properties.
+  // The file property is the file path.
+  // The html property is some html which will be generated by the doctool.
+  // This html will be stripped of all whitespace.


Maybe change the comment to state that it will be stripped of all unnecessary whitespace?

Edit: disregard. I just realized why you were doing that. Maybe update the comment to be more clear?

iankronquist · 2016-04-21T18:15:33Z

Looks like CI passed.

silverwind · 2016-04-21T18:27:57Z

Looks like the new tests got picked up fine:

https://ci.nodejs.org/job/node-test-commit-linux/3062/nodes=debian8-64/tapTestReport/test.tap-1065/
https://ci.nodejs.org/job/node-test-commit-linux/3062/nodes=debian8-64/tapTestReport/test.tap-1066/

LGTM

iankronquist · 2016-04-28T00:12:40Z

@silverwind Is there anyone else I need to get to take a look at this?

santigimeno · 2016-04-28T10:15:20Z

test/doctool/test-doctool-html.js

+  fs.readFile(item.file, 'utf8', common.mustCall(function(err, input) {
+    assert.ifError(err);
+    html(input, 'foo', 'doc/template.html',
+        common.mustCall(function(err, output) {


shouldn't common.mustCall... be aligned with input?

silverwind · 2016-04-28T20:57:00Z

@iankronquist after you've handled above comments, I suggest you squash the commits into a single one and add a nice summary of the changes in the commit message.

Addresses nodejs#5955 on GitHub. * Test the toHTML function in html.js. Check that given valid markdown it produces the expected html. One test case will prevent regressions of big nodejs#5873. * Check that when given valid markdown toJSON produces valid JSON with the expected schema. * Add doctool to the list of built in tests so it runs in CI.

iankronquist · 2016-04-28T22:20:03Z

@silverwind fixed & squashed.

* Test the toHTML function in html.js. Check that given valid markdown it produces the expected html. One test case will prevent regressions of #5873. * Check that when given valid markdown toJSON produces valid JSON with the expected schema. * Add doctool to the list of built in tests so it runs in CI. PR-URL: #6031 Fixes: #5955 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rich Trott <[email protected]>

silverwind · 2016-04-28T22:49:20Z

Thanks! Landed in 2e845f8 with some minor corrections to the commit message. Nice work!

eljefedelrodeodeljefe · 2016-04-28T23:01:47Z

Yes, really nice. Thx for the effort. Time to try it myself :) Awesome guys.

iankronquist · 2016-04-29T02:41:47Z

@silverwind excellent, thanks!

* Test the toHTML function in html.js. Check that given valid markdown it produces the expected html. One test case will prevent regressions of #5873. * Check that when given valid markdown toJSON produces valid JSON with the expected schema. * Add doctool to the list of built in tests so it runs in CI. PR-URL: #6031 Fixes: #5955 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins · 2016-05-17T23:04:44Z

if someone would like to see this landed it will need to be manually backported

iankronquist · 2016-05-18T01:53:00Z

@thealphanerd didn't this land in 2e845f8? Do you mean landed in the next release?

MylesBorins · 2016-05-18T01:53:48Z

Sorry for not being exact... I was referring to lts

* Test the toHTML function in html.js. Check that given valid markdown it produces the expected html. One test case will prevent regressions of #5873. * Check that when given valid markdown toJSON produces valid JSON with the expected schema. * Add doctool to the list of built in tests so it runs in CI. PR-URL: #6031 Fixes: #5955 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rich Trott <[email protected]>

iankronquist mentioned this pull request Apr 4, 2016

Testing the doctool #5955

Closed

addaleax reviewed Apr 4, 2016
View reviewed changes

iankronquist force-pushed the test-doctool branch 2 times, most recently from 931ff1d to f240d02 Compare April 4, 2016 01:02

jasnell added the test Issues and PRs related to the tests. label Apr 4, 2016

Trott added the doc Issues and PRs related to the documentations. label Apr 4, 2016

Trott added the lts-watch-v4.x label Apr 4, 2016

Trott reviewed Apr 4, 2016
View reviewed changes

evanlucas reviewed Apr 4, 2016
View reviewed changes

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

santigimeno reviewed Apr 28, 2016
View reviewed changes

iankronquist force-pushed the test-doctool branch from b7968ed to 94318cd Compare April 28, 2016 22:19

silverwind closed this Apr 28, 2016

This was referenced May 4, 2016

Propose v6.1.0 wrong base please ignore #6556

Closed

Propose v6.1.0 (Openssl update) #6557

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 1, 2016

jellelicht mentioned this pull request Jun 7, 2016

Doctool tests require absent eslint #7201

Closed

MylesBorins mentioned this pull request Jun 24, 2016

v4.4.7 proposal #7395

Merged

		@@ -0,0 +1,50 @@
		'use strict';

		var assert = require('assert');

Test doctool #6031

Test doctool #6031

Conversation

iankronquist commented Apr 4, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

MylesBorins commented Apr 4, 2016

iankronquist commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemao commented Apr 4, 2016

Trott commented Apr 4, 2016

Trott commented Apr 4, 2016

Trott commented Apr 4, 2016

Trott commented Apr 4, 2016

Trott commented Apr 4, 2016

Trott commented Apr 4, 2016

iankronquist commented Apr 4, 2016

Trott commented Apr 4, 2016

iankronquist commented Apr 4, 2016

Trott commented Apr 4, 2016

Trott commented Apr 4, 2016

Choose a reason for hiding this comment

Knighton910 commented Apr 4, 2016

eljefedelrodeodeljefe commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Apr 4, 2016

Choose a reason for hiding this comment

iankronquist commented Apr 21, 2016

silverwind commented Apr 21, 2016 • edited Loading

iankronquist commented Apr 28, 2016

Choose a reason for hiding this comment

silverwind commented Apr 28, 2016

iankronquist commented Apr 28, 2016

silverwind commented Apr 28, 2016

eljefedelrodeodeljefe commented Apr 28, 2016

iankronquist commented Apr 29, 2016

MylesBorins commented May 17, 2016

iankronquist commented May 18, 2016

MylesBorins commented May 18, 2016

silverwind commented Apr 21, 2016 •

edited

Loading