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

docs: add steps for running addons + npm tests by MylesBorins · Pull Request #6231 · nodejs/node · GitHub

/ node Public

docs: add steps for running addons + npm tests - #6231

Merged
MylesBorins merged 1 commit into
nodejs:masterfrom
MylesBorins:expanded-test-info
May 9, 2016
ConversationCommits (1)ChecksFiles changed
Merged

docs: add steps for running addons + npm tests#6231
MylesBorins merged 1 commit into
nodejs:masterfrom
MylesBorins:expanded-test-info

Conversation

MylesBorins commented Apr 15, 2016

Copy link
Copy Markdown
Contributor

Currently we do not document how to run the test suite for native
modules or for npm. This commit updates BUILDING.md with the
information needed to do so. It includes a caveat about Node v4
and earlier requiring make install to be run before running the npm
test suite.

MylesBorins added the doc Issues and PRs related to the documentations. label Apr 15, 2016

bnoordhuis commented Apr 15, 2016

Copy link
Copy Markdown
Member

LGTM but s/docs:/doc:/ in the commit log.

MylesBorins commented Apr 15, 2016 via email

Copy link
Copy Markdown
Contributor Author

Fishrock123 reviewed Apr 15, 2016
View reviewed changes
Comment thread BUILDING.md Outdated

Fishrock123 Apr 15, 2016

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

This doesn't render correctly. Try viewing the rendered version.

MylesBorins Apr 15, 2016

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

fixed

MylesBorins force-pushed the expanded-test-info branch 2 times, most recently from 7ffeb2d to 4ff78d5 Compare April 15, 2016 18:49

Fishrock123 commented Apr 15, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell reviewed Apr 15, 2016
View reviewed changes
Comment thread BUILDING.md Outdated

jasnell Apr 15, 2016

Copy link
Copy Markdown
Member

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 not put this in a text block like the others?

MylesBorins Apr 15, 2016

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

v5 > all use the internal npm to run the tests. No need to install.

jasnell commented Apr 15, 2016

Copy link
Copy Markdown
Member

LGTM with a nit

MylesBorins mentioned this pull request Apr 15, 2016
Closed

mhdawson commented Apr 15, 2016

Copy link
Copy Markdown
Member

I'm not sure we want to require a make install to be able to run the tests. We've had this break in the past and have fixed it

MylesBorins commented Apr 15, 2016

Copy link
Copy Markdown
Contributor Author

afaik this has always been the case for npm2 and fixing it is non trivial

MylesBorins commented Apr 18, 2016

Copy link
Copy Markdown
Contributor Author

@mhdawson have your opinions on this pr changed?

mhdawson commented Apr 18, 2016

Copy link
Copy Markdown
Member

Just talking to Richard to understand what the issue he was seeing and to understand if there is any other work around besides running make install. If there is one we might want to include in the docs as well.

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

MylesBorins commented Apr 27, 2016

Copy link
Copy Markdown
Contributor Author

@mhdawson any movement on this?

mhdawson commented May 9, 2016

Copy link
Copy Markdown
Member

Sorry, should have commented earlier. Since it reflects what is currently the case I think we should go ahead and land, and if we decide to change anything in 4.X at some point we can update then.

LGTM.

MylesBorins force-pushed the expanded-test-info branch from 4ff78d5 to e2b44c3 Compare May 9, 2016 21:09
doc: add steps for running addons + npm tests
10a60a1 
Currently we do not document how to run the test suite for native
modules or for npm. This commit updates BUILDING.md with the
information needed to do so. It includes a caveat about Node v4
and earlier requiring `make install` to be run before running the npm
test suite.

PR-URL: nodejs#6231
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins force-pushed the expanded-test-info branch from e2b44c3 to 10a60a1 Compare May 9, 2016 21:10
MylesBorins merged commit 10a60a1 into nodejs:master May 9, 2016
MylesBorins pushed a commit that referenced this pull request May 9, 2016
doc: add steps for running addons + npm tests
4795b52 
Currently we do not document how to run the test suite for native
modules or for npm. This commit updates BUILDING.md with the
information needed to do so. It includes a caveat about Node v4
and earlier requiring `make install` to be run before running the npm
test suite.

PR-URL: #6231
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins deleted the expanded-test-info branch May 12, 2016 18:14
evanlucas pushed a commit that referenced this pull request May 17, 2016
doc: add steps for running addons + npm tests
5c154a8 
Currently we do not document how to run the test suite for native
modules or for npm. This commit updates BUILDING.md with the
information needed to do so. It includes a caveat about Node v4
and earlier requiring `make install` to be run before running the npm
test suite.

PR-URL: #6231
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
evanlucas mentioned this pull request May 17, 2016
Merged
MylesBorins pushed a commit that referenced this pull request May 18, 2016
doc: add steps for running addons + npm tests
29e25d8 
Currently we do not document how to run the test suite for native
modules or for npm. This commit updates BUILDING.md with the
information needed to do so. It includes a caveat about Node v4
and earlier requiring `make install` to be run before running the npm
test suite.

PR-URL: #6231
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins mentioned this pull request May 18, 2016
Merged
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

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL