build: make test-doc and lint addon docs #16377

joyeecheung · 2017-10-22T06:39:35Z

Prepping for nodejs/build#929

Implements the make test-doc target that build, verify and lint docs
Lint the C++ snippets in addon docs
When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test, tools

joyeecheung · 2017-10-22T06:40:52Z

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

vsemozhetbyt · 2017-10-22T10:00:03Z

BTW, why we abstain from making docs and running recently added doc tests on Windows? Is it from vcbuild.bat complication? It would be handy to have an option to make test doc builds on Windows locally.

cc @nodejs/platform-windows

gibfahn · 2017-10-22T17:14:43Z

BTW, why we abstain from making docs and running recently added doc tests on Windows? Is it from vcbuild.bat complication? It would be handy to have an option to make test doc builds on Windows locally.

I think it's just waiting for someone to implement.

gibfahn · 2017-10-22T17:32:03Z

@joyeecheung so what's the difference between make test-doc and make lint-md (#12756)?

Is test-doc going to call lint-md?

joyeecheung · 2017-10-22T19:41:24Z

@gibfahn That's the plan

refack · 2017-10-22T21:41:39Z

Makefile

@@ -262,7 +263,7 @@ endif

 test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
 	$(RM) -r test/addons/??_*/
-	$(NODE) $<
+	[ -x $(NODE) ] && $(NODE) $< || node $<


Why just here? shouldn't this be handled in a global manner? Or just err with the message "to run with a precompiled node binary run make NODE=<path_to_node> <target>"

joyeecheung · 2017-10-28T07:58:21Z

Figured out why the linter failed (ci doesn't build the addon docs before linting them).

@refack I gave the global $(NODE) a try and turns out it's trickier than I thought. Many rules use this pattern but some of them depend on the actual build rule indirectly, so the status of -x ./node might change. I'll separate the refactor into another PR.

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

refack · 2017-10-28T13:57:02Z

doc/api/addons.md

@@ -1095,7 +1095,7 @@ static void at_exit_cb1(void* arg) {
  Isolate* isolate = static_cast<Isolate*>(arg);
  HandleScope scope(isolate);
  Local<Object> obj = Object::New(isolate);
-  assert(!obj.IsEmpty()); // assert VM is still alive
+  assert(!obj.IsEmpty());  // assert VM is still alive


is this a lint issue?

Yes, needs two spaces here

refack · 2017-10-28T13:58:46Z

Change looks good, but I'm assuming it's not well covered by CI?
I guess it would be nice if someone manually checked the changed targets (me Windows, no good make tester):

test
(test/addons/.docbuildstamp)
~~test-ci~~ (CI)
test-doc
lint-js
~~lint-js-ci~~ (CI)
lint-addon-docs
~~lint-ci~~ (CI)

- Implements the make test-doc target that build, verify and lint docs - Lint the C++ snippets in addon docs - When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc.

joyeecheung · 2017-10-29T04:52:43Z

Rebased & squashed. New CI: https://ci.nodejs.org/job/node-test-pull-request/11063/

joyeecheung · 2017-10-30T03:38:30Z

Going to land this later today.

- Implements the make test-doc target that build, verify and lint docs - Lint the C++ snippets in addon docs - When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc. PR-URL: #16377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

joyeecheung · 2017-10-30T09:36:16Z

Landed in 390eda1, thanks!

- Implements the make test-doc target that build, verify and lint docs - Lint the C++ snippets in addon docs - When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc. PR-URL: #16377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

- Implements the make test-doc target that build, verify and lint docs - Lint the C++ snippets in addon docs - When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc. PR-URL: nodejs/node#16377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

- Implements the make test-doc target that build, verify and lint docs - Lint the C++ snippets in addon docs - When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc. PR-URL: nodejs#16377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

- Implements the make test-doc target that build, verify and lint docs - Lint the C++ snippets in addon docs - When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc. PR-URL: nodejs/node#16377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 22, 2017

joyeecheung mentioned this pull request Oct 22, 2017

A new CI job for linting and doc tests nodejs/build#929

Closed

jasnell approved these changes Oct 22, 2017

View reviewed changes

refack reviewed Oct 22, 2017

View reviewed changes

joyeecheung force-pushed the test-doc-target branch from f85887a to de6d72a Compare October 28, 2017 07:11

refack approved these changes Oct 28, 2017

View reviewed changes

refack reviewed Oct 28, 2017

View reviewed changes

joyeecheung force-pushed the test-doc-target branch from 13f210f to 87167a5 Compare October 29, 2017 04:52

joyeecheung closed this Oct 30, 2017

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

cjihrig mentioned this pull request Nov 6, 2017

v9.1.0 proposal #16851

Merged

MylesBorins added the lts-watch-v6.x label Nov 16, 2017

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 20, 2017

rvagg mentioned this pull request Apr 2, 2018

build: make lint-ci work properly on Linux make #19746

Closed

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: make test-doc and lint addon docs #16377

build: make test-doc and lint addon docs #16377

joyeecheung commented Oct 22, 2017

joyeecheung commented Oct 22, 2017

vsemozhetbyt commented Oct 22, 2017 •

edited

Loading

gibfahn commented Oct 22, 2017

gibfahn commented Oct 22, 2017

joyeecheung commented Oct 22, 2017

refack Oct 22, 2017

joyeecheung Oct 23, 2017

joyeecheung commented Oct 28, 2017 •

edited

Loading

refack Oct 28, 2017

joyeecheung Oct 28, 2017

refack commented Oct 28, 2017

joyeecheung commented Oct 29, 2017

joyeecheung commented Oct 30, 2017

joyeecheung commented Oct 30, 2017

build: make test-doc and lint addon docs #16377

build: make test-doc and lint addon docs #16377

Conversation

joyeecheung commented Oct 22, 2017

Checklist

Affected core subsystem(s)

joyeecheung commented Oct 22, 2017

vsemozhetbyt commented Oct 22, 2017 • edited Loading

gibfahn commented Oct 22, 2017

gibfahn commented Oct 22, 2017

joyeecheung commented Oct 22, 2017

refack Oct 22, 2017

Choose a reason for hiding this comment

joyeecheung Oct 23, 2017

Choose a reason for hiding this comment

joyeecheung commented Oct 28, 2017 • edited Loading

refack Oct 28, 2017

Choose a reason for hiding this comment

joyeecheung Oct 28, 2017

Choose a reason for hiding this comment

refack commented Oct 28, 2017

joyeecheung commented Oct 29, 2017

joyeecheung commented Oct 30, 2017

joyeecheung commented Oct 30, 2017

vsemozhetbyt commented Oct 22, 2017 •

edited

Loading

joyeecheung commented Oct 28, 2017 •

edited

Loading