test: add test-npm-install to parallel tests suite by MylesBorins · Pull Request #5166 · nodejs/node · GitHub

Currently we are not testing that npm install works.

This is a very naive / basic test that shells out to npm install
in an empty tempDir. While this test will not be able to check
that npm install is 100% working, it should catch certain edge
cases that break it.

This is currently blocked from being tested until #4525 lands

/cc @nodejs/npm @nodejs/testing

CI: https://ci.nodejs.org/job/node-test-pull-request/1613/
Expected to be all red as npm is broken on master

Nit: capitalization and (more importantly) punctuation in comments. I tried reading the first two lines four or five times before I figured out that each line is its own sentence.

It seems like whatever I had working locally is not working atm... digging in

Probably no need to capture stdout and stderr if nothing will be done with them.

If it passes again when npm is fixed on master, LGTM with comments.

@Trott updated which fixes all your nits

mebbe this would be more stable https://github.com/nodejs/node/blob/master/deps/npm/package.json#L31

@maxogden can you elaborate?

less likely to break if npm changes their entry point. prob doesnt matter though :)

Ah, I see what you mean.

I'd prefer to not start grabbing package.json and resolving the path from there... lots of extra logic.

If npm changes their entry point this test will break when they send in a PR... it will be a pretty easy fix

closing in favor of #5163

will reopen if that does not land

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

This test doesn't strike me as a very good idea for two reasons:

It seems to require internet access, which makes it unsuitable for test/parallel. test/internet is a better place but it's not run on the CI.
Correct me if I'm wrong, but a plain npm install will walk up the directory tree until it finds a node_modules directory, right? That means you could get some pretty arbitrary behavior based on what people have installed outside their node source tree.

Marking as don't land in v4 until we're certain this is going to work out.

LGTM if CI is happy. The last CI run failed because a slave went offline. New run here: https://ci.nodejs.org/job/node-test-pull-request/1817/

Currently we are not testing that `npm install` works. This is a very naive / basic test that shells out to `npm install` in an empty `tempDir`. While this test will not be able to check that `npm install` is 100% working, it should catch certain edge cases that break it.

CI is all green. I attempted to look for network traffic but with wireshark, but I'm not familiar enough with it to really get a clear reading, too much noise.

I was talking to @zkat last night who implied there should be no network traffic. Maybe @iarna or @othiym23 can chime in

npm definitely doesn't do anything on the network with an empty package.json. Even with dependencies, it'll only touch the network if your deps require it (eg, registry, remote tarball, remote git repo).

I'm not seeing any -1's and I'm seeing 3 LGTM's

I'll land this at some point on Monday if no one else has objections

Following @iarna comment, would it be a good idea to add a local path as a dependency? Something like this:

"test-module": "file:///path/to/test-module"

If I understand it correctly, it would not trigger any network operation.

@santigimeno you'd probably want a relative path, but yeah, that'd exercise a bit more. But I'd be -1 on holding back this PR any further. Seems like that'd make a fine new PR though.

(And for the record, I'm LGTM on this as is.)

Ok, I can to that when this PR is merged. Thanks

Currently we are not testing that `npm install` works. This is a very naive / basic test that shells out to `npm install` in an empty `tempDir`. While this test will not be able to check that `npm install` is 100% working, it should catch certain edge cases that break it. PR-URL: nodejs#5166 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>

landed in 061ebb3

Currently we are not testing that `npm install` works. This is a very naive / basic test that shells out to `npm install` in an empty `tempDir`. While this test will not be able to check that `npm install` is 100% working, it should catch certain edge cases that break it. PR-URL: #5166 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>

@jasnell are you open to moving to lts watch?

I haven't been following how well this has been working since it landed. I assume no issues?

moving to lts-watch as there have been no issues

Currently we are not testing that `npm install` works. This is a very naive / basic test that shells out to `npm install` in an empty `tempDir`. While this test will not be able to check that `npm install` is 100% working, it should catch certain edge cases that break it. PR-URL: #5166 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>

MylesBorins mentioned this pull request Feb 9, 2016

Test basic functionality of gyp + npm nodejs/testing#15

Closed

MylesBorins force-pushed the test-npm branch from 45d1c76 to 42d7ba5 Compare February 9, 2016 18:43

Trott reviewed Feb 9, 2016
View reviewed changes

MylesBorins changed the title test: add test-npm-install to parallel tests suite WIP test: add test-npm-install to parallel tests suite Feb 9, 2016

Trott reviewed Feb 9, 2016
View reviewed changes

MylesBorins force-pushed the test-npm branch from 42d7ba5 to 943c1e7 Compare February 9, 2016 18:56

MylesBorins changed the title WIP test: add test-npm-install to parallel tests suite test: add test-npm-install to parallel tests suite Feb 9, 2016

MylesBorins force-pushed the test-npm branch from 943c1e7 to 91b4f79 Compare February 9, 2016 18:57

mscdex added test Issues and PRs related to the tests. npm Issues and PRs related to the npm client dependency or the npm registry. labels Feb 9, 2016

This was referenced Feb 9, 2016

fs: add a temporary fix for re-evaluation support #5102

Closed

Revert "fs: deprecate fs.read's string interface" #5163

Closed

max-mapper reviewed Feb 9, 2016
View reviewed changes

MylesBorins closed this Feb 9, 2016

MylesBorins deleted the test-npm branch February 10, 2016 01:14

MylesBorins restored the test-npm branch February 11, 2016 19:03

MylesBorins reopened this Feb 11, 2016

MylesBorins closed this Feb 12, 2016

MylesBorins deleted the test-npm branch February 12, 2016 00:40

MylesBorins restored the test-npm branch February 13, 2016 10:03

MylesBorins reopened this Feb 13, 2016

MylesBorins force-pushed the test-npm branch from 91b4f79 to f0fbc73 Compare February 13, 2016 10:05

jasnell added the dont-land-on-v4.x label Mar 2, 2016

MylesBorins force-pushed the test-npm branch from 0228191 to d3c9834 Compare March 4, 2016 00:14

MylesBorins closed this Mar 7, 2016

Fishrock123 mentioned this pull request Mar 7, 2016

v5.8.0 proposal #5559

Merged

santigimeno mentioned this pull request Mar 9, 2016

test: improve test-npm-install #5613

Closed

4 tasks

MylesBorins added lts-watch-v4.x and removed dont-land-on-v4.x labels Mar 30, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

MylesBorins deleted the test-npm branch April 16, 2016 18:43

test: add test-npm-install to parallel tests suite by MylesBorins · Pull Request #5166 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: add test-npm-install to parallel tests suite - #5166

test: add test-npm-install to parallel tests suite#5166MylesBorins wants to merge 1 commit intonodejs:masternodejs/node:masterfrom MylesBorins:test-npmMylesBorins/node:test-npmCopy head branch name to clipboard

Conversation

MylesBorins commented Feb 9, 2016

Uh oh!

MylesBorins commented Feb 9, 2016

Uh oh!

Trott Feb 9, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Feb 9, 2016

Uh oh!

Trott Feb 9, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Feb 9, 2016

Uh oh!

MylesBorins commented Feb 9, 2016

Uh oh!

max-mapper Feb 9, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins Feb 9, 2016

Choose a reason for hiding this comment

Uh oh!

max-mapper Feb 9, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins Feb 9, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Feb 9, 2016

Uh oh!

MylesBorins commented Feb 13, 2016

Uh oh!

bnoordhuis commented Feb 13, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

orangemocha commented Mar 2, 2016

Uh oh!

MylesBorins commented Mar 4, 2016

Uh oh!

iarna commented Mar 4, 2016

Uh oh!

MylesBorins commented Mar 5, 2016

Uh oh!

santigimeno commented Mar 5, 2016

Uh oh!

iarna commented Mar 5, 2016

Uh oh!

santigimeno commented Mar 5, 2016

Uh oh!

MylesBorins commented Mar 7, 2016

Uh oh!

MylesBorins commented Mar 10, 2016

Uh oh!

jasnell commented Mar 11, 2016

Uh oh!

MylesBorins commented Mar 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

test: add test-npm-install to parallel tests suite#5166
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:test-npm