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

MylesBorins · 2016-02-09T18:35:45Z

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

MylesBorins · 2016-02-09T18:37:04Z

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

Trott · 2016-02-09T18:44:39Z

test/parallel/test-npm-install.js

+proc.on('exit', function(code) {
+  // npm install in tmpDir should be essentially a no-op
+  // if npm is not broken the process should always exit 0
+  // assert that there are no obvious failures


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.

MylesBorins · 2016-02-09T18:45:04Z

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

Trott · 2016-02-09T18:46:04Z

test/parallel/test-npm-install.js

+proc.stderr.setEncoding('utf8');
+
+proc.stdout.on('data', (data) => stdout += data);
+proc.stdout.on('err', (data) => stderr += data);


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

Trott · 2016-02-09T18:48:32Z

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

MylesBorins · 2016-02-09T18:57:22Z

@Trott updated which fixes all your nits

max-mapper · 2016-02-09T19:40:50Z

test/parallel/test-npm-install.js

+  'npm',
+  'bin',
+  'npm-cli.js'
+);


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

MylesBorins · 2016-02-09T19:50:02Z

closing in favor of #5163

will reopen if that does not land

MylesBorins · 2016-02-13T10:07:12Z

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

bnoordhuis · 2016-02-13T10:30:41Z

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.

jasnell · 2016-03-02T17:04:23Z

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

orangemocha · 2016-03-02T20:29:50Z

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.

MylesBorins · 2016-03-04T00:46:12Z

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

iarna · 2016-03-04T00:51:30Z

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).

MylesBorins · 2016-03-05T02:33:04Z

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

santigimeno · 2016-03-05T07:31:16Z

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.

iarna · 2016-03-05T07:34:25Z

@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.)

santigimeno · 2016-03-05T07:54:24Z

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>

MylesBorins · 2016-03-07T22:00:11Z

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>

MylesBorins · 2016-03-10T22:52:11Z

@jasnell are you open to moving to lts watch?

jasnell · 2016-03-11T02:19:17Z

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

MylesBorins · 2016-03-30T18:29:58Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MylesBorins commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

Trott Feb 9, 2016

MylesBorins commented Feb 9, 2016

Trott Feb 9, 2016

Trott commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

max-mapper Feb 9, 2016

MylesBorins Feb 9, 2016

max-mapper Feb 9, 2016

MylesBorins Feb 9, 2016

MylesBorins commented Feb 9, 2016

MylesBorins commented Feb 13, 2016

bnoordhuis commented Feb 13, 2016

jasnell commented Mar 2, 2016

orangemocha commented Mar 2, 2016

MylesBorins commented Mar 4, 2016

iarna commented Mar 4, 2016

MylesBorins commented Mar 5, 2016

santigimeno commented Mar 5, 2016

iarna commented Mar 5, 2016

santigimeno commented Mar 5, 2016

MylesBorins commented Mar 7, 2016

MylesBorins commented Mar 10, 2016

jasnell commented Mar 11, 2016

MylesBorins commented Mar 30, 2016

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

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

Conversation

MylesBorins commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

Trott Feb 9, 2016

Choose a reason for hiding this comment

MylesBorins commented Feb 9, 2016

Trott Feb 9, 2016

Choose a reason for hiding this comment

Trott commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

max-mapper Feb 9, 2016

Choose a reason for hiding this comment

MylesBorins Feb 9, 2016

Choose a reason for hiding this comment

max-mapper Feb 9, 2016

Choose a reason for hiding this comment

MylesBorins Feb 9, 2016

Choose a reason for hiding this comment

MylesBorins commented Feb 9, 2016

MylesBorins commented Feb 13, 2016

bnoordhuis commented Feb 13, 2016

jasnell commented Mar 2, 2016

orangemocha commented Mar 2, 2016

MylesBorins commented Mar 4, 2016

iarna commented Mar 4, 2016

MylesBorins commented Mar 5, 2016

santigimeno commented Mar 5, 2016

iarna commented Mar 5, 2016

santigimeno commented Mar 5, 2016

MylesBorins commented Mar 7, 2016

MylesBorins commented Mar 10, 2016

jasnell commented Mar 11, 2016

MylesBorins commented Mar 30, 2016