Skip to content
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

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

Merged
merged 1 commit into from
May 9, 2016

Conversation

MylesBorins
Copy link
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 MylesBorins added the doc Issues and PRs related to the documentations. label Apr 15, 2016
@bnoordhuis
Copy link
Member

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

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 15, 2016 via email


To run the npm test suite:
**note: to run the suite on node v4 or earlier you must first**
** run `make install`**
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

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

LGTM

To run the npm test suite:

*note: to run the suite on node v4 or earlier you must first*
*run `make install`*
Copy link
Member

Choose a reason for hiding this comment

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

Why not put this in a text block like the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

LGTM with a nit

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

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

@MylesBorins
Copy link
Contributor Author

@mhdawson have your opinions on this pr changed?

@mhdawson
Copy link
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.

@MylesBorins
Copy link
Contributor Author

@mhdawson any movement on this?

@mhdawson
Copy link
Member

mhdawson commented May 9, 2016

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.

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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins merged commit 10a60a1 into nodejs:master May 9, 2016
MylesBorins pushed a commit that referenced this pull request May 9, 2016
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins deleted the expanded-test-info branch May 12, 2016 18:14
evanlucas pushed a commit that referenced this pull request May 17, 2016
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants