make test-npm in LTS (4.4.3)

[richardlau]

• Version: v4.4.3
• Platform: Linux drv-aeon 2.6.32-131.12.1.el6.x86_64 deps: update openssl to 1.0.1j #1 SMP Sun Jul 31 16:44:56 EDT 2011 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: test

make test-npm appears to be broken in LTS (v4.4.3):

• console output with Node v4.4.3: make test-npm

I saw #6150 contains a patch to remove the call to test-legacy, so I've applied that patch and now get a different error:

• console output with Node v4.4.3 + 4b71896f3df29c00eaf559778e12e1357a6468c7: make test-npm

This looks like the test is looking for npm -- Presumably the makefile or tools/test-npm.sh should be adding npm to the path?

I've tried adding test-npm/bin to my PATH, which results in yet another error:

• console output with Node v4.4.3 with 4b71896f3df29c00eaf559778e12e1357a6468c7+PATH: make test-npm

This looks like test-npm/bin/npm is incorrectly resolving npm-cli.js -- I think test-npm/bin/npm is written for an installed Node (i.e. expecting npm to live under a node_modules directory) and not the copy made for the test.

[richardlau]

CC @zkat

[MylesBorins]

@richardlau this is my bad. It looks like it is the result of the recent upgrade to npm v2.15.1

This is now fixed in v4.x-staging

Thank you for pointing this out!

[MylesBorins]

it is worth mentioning that make test-npm is now working as expected on v4.x-staging

[richardlau]

@thealphanerd Nope, still fails for me with v4.x-staging

• console output with v4.x-staging: make test-npm

[richardlau]

-bash-4.1$ git describe
v4.4.3-6-gc92febf
-bash-4.1$

[MylesBorins]

Are you running make install first? I believe that v4 npm tests use system npm

[richardlau]

Is this documented anywhere?

[richardlau]

(No I wasn't running make install first, there wasn't anything suggesting I needed to do that)

[mhdawson]

There was earlier issues with this:

#1926

and I believe we were trying to ensure it could continue to run without having to install node globally

[MylesBorins]

I've sent a PR to update the docs

#6231

[mhdawson]

Reopening for discussion as to whether it should required a global install

[mhdawson]

From my perspective requiring a global install is not that friendly. I'd want people to be able to easily run the tests without having to affect their environment outside of the tree they are compiling Node in. And there was some prior agreement on that so we should discuss.

[Fishrock123]

There's not much to discuss here. That is the state of things in npm 2 and fixing it isn't super trivial iirc.

Besides, most people aren't going to be running those tests, and we're adding a doc note about it.

[Fishrock123]

Note that this works as expected on master with npm 3.

[MylesBorins]

@mhdawson #1926 was never backported to v4.x

So here is package.json for npm in v4.4.1 which we are aware of working in CI

node/deps/npm/package.json

Line 193 in 5368455

"test-tap": "npm run tap -- \"test/tap/*.js\""

as you can see it calls to npm expecting it in the path. Nothing has changed here between v4.4.1 and v4.4.3

Alternatively on master we can see that npm 3 is a bit smarter about setting up the env

https://github.com/nodejs/node/blob/master/deps/npm/package.json#L210

[mhdawson]

Ok so I can understand "its already fixed in later versions" and was always broken in post 0.12.X so we won't fix the versions in the middle.

In that case I expect Richard's problem is unrelated to have to do the global install as the tests were running ok until 4.4.2

[Fishrock123]

Iirc even with our tools fix, npm will create child processes on a global node.

(Note: I may be incorrect on that, and anyone is welcome to investigate!)

[addaleax]

@Fishrock123 As of npm/npm#11204, npm will spawn its script child processes with the Node.js version it was invoked with by adding the directory in which process.execPath resides to the PATH.
According to the PR’s milestones, that’s npm >= 3.7.0 and >= 2.14.17, which have been bundled with Node.js >= 5.8.0 and >= 4.4.0 respectively… I’ve done a ton of digging on issues that were related to that change.

[mhdawson]

@addaleax so if I read your statement correctly if the npm in the build tree was used then the child processes would be as well, right ?

[addaleax]

@mhdawson I think so, yes. I’m definitely not an expert for the Node core side of npm testing, but the corresponding line to that change is there in v4.x.