docs: add steps for running addons + npm tests#6231
Merged
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
MylesBorinsforce-pushed the expanded-test-info branch from 35b17f5 to f0f6276Compare
Member
bnoordhuis commented Apr 15, 2016
LGTM but s/docs:/doc:/ in the commit log. |
MylesBorinsforce-pushed the expanded-test-info branch from f0f6276 to 550d2acCompareContributorAuthor
MylesBorins commented Apr 15, 2016 via email
updated |
BUILDING.md Outdated
Contributor
There was a problem hiding this comment.
This doesn't render correctly. Try viewing the rendered version.
MylesBorinsforce-pushed the expanded-test-info branch 2 times, most recently from 7ffeb2d to 4ff78d5Compare
Contributor
Fishrock123 commented Apr 15, 2016
LGTM |
BUILDING.md Outdated
Member
There was a problem hiding this comment.
Why not put this in a text block like the others?
ContributorAuthor
There was a problem hiding this comment.
v5 > all use the internal npm to run the tests. No need to install.
Member
jasnell commented Apr 15, 2016
LGTM with a nit |
Member
mhdawson commented Apr 15, 2016
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 |
ContributorAuthor
MylesBorins commented Apr 15, 2016
afaik this has always been the case for npm2 and fixing it is non trivial |
ContributorAuthor
MylesBorins commented Apr 18, 2016
@mhdawson have your opinions on this pr changed? |
Member
mhdawson commented Apr 18, 2016
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. |
estliberitasforce-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompareContributorAuthor
MylesBorins commented Apr 27, 2016
@mhdawson any movement on this? |
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. |
MylesBorinsforce-pushed the expanded-test-info branch from 4ff78d5 to e2b44c3CompareCurrently 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]>
MylesBorinsforce-pushed the expanded-test-info branch from e2b44c3 to 10a60a1CompareMylesBorins 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]>
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]>
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 installto be run before running the npmtest suite.