FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: make test-doc and lint addon docs by joyeecheung · Pull Request #16377 · nodejs/node · GitHub

/ node Public

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

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:test-doc-target
ConversationCommits (1)ChecksFiles changed
Closed

build: make test-doc and lint addon docs#16377
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:test-doc-target

Conversation

joyeecheung commented Oct 22, 2017

Copy link
Copy Markdown
Member

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

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

joyeecheung commented Oct 22, 2017

Copy link
Copy Markdown
Member Author

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

vsemozhetbyt commented Oct 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

joyeecheung mentioned this pull request Oct 22, 2017
Closed

gibfahn commented Oct 22, 2017

Copy link
Copy Markdown
Member

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 commented Oct 22, 2017

Copy link
Copy Markdown
Member

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

Is test-doc going to call lint-md?

joyeecheung commented Oct 22, 2017

Copy link
Copy Markdown
Member Author

@gibfahn That's the plan

refack reviewed Oct 22, 2017
View reviewed changes
Comment thread Makefile Outdated

refack Oct 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 Oct 23, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good idea

joyeecheung commented Oct 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 reviewed Oct 28, 2017
View reviewed changes
Comment thread doc/api/addons.md Outdated

refack Oct 28, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

is this a lint issue?

joyeecheung Oct 28, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, needs two spaces here

refack commented Oct 28, 2017

Copy link
Copy Markdown
Contributor

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)

build: make test-doc and lint addon docs
87167a5 
- 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 commented Oct 29, 2017

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Oct 30, 2017

Copy link
Copy Markdown
Member Author

Going to land this later today.

joyeecheung added a commit that referenced this pull request Oct 30, 2017
build: make test-doc and lint addon docs
390eda1 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

joyeecheung commented Oct 30, 2017

Copy link
Copy Markdown
Member Author

Landed in 390eda1, thanks!

gibfahn pushed a commit that referenced this pull request Oct 30, 2017
build: make test-doc and lint addon docs
608a20b 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
build: make test-doc and lint addon docs
9805c3d 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
build: make test-doc and lint addon docs
d576e17 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn mentioned this pull request Oct 31, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
build: make test-doc and lint addon docs
2977aa3 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
build: make test-doc and lint addon docs
9851daa 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
build: make test-doc and lint addon docs
8bc5249 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
cjihrig mentioned this pull request Nov 6, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
build: make test-doc and lint addon docs
9bf1dc0 
- 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 <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
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
Closed
MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+1 more reviewer
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL