test: Fix test-npm Makefile target for npm 2.8.4

[mhdawson]

Changes in npm 2.8.4 broke the test-npm target. This change
updates to allow the tests to run once again

[Fishrock123]

Huh, I wonder why this didn't break for io.js?

Either way, I recommend replacing this entirely with a port of nodejs/node@0b21ab1

[mhdawson]

Fishrock123, Does io.js do something to add npm to the path in a development build ? That seems to be the change in the how the tests are run in 2.8.4. Is there a job that runs this automatically for io.js so I can what steps it is running ?

[mhdawson]

I should have also added that I did look at nodejs/node@0b21ab1 but I don't see how it would fix the issue as it still ends up running the same test target in 2.8.4

[Fishrock123]

Does io.js do something to add npm to the path in a development build

but I don't see how it would fix the issue as it still ends up running the same test target in 2.8.4

Pardon?

It ignores using PATH entirely, just like node@master, and it runs everything on a copy so it's more robust.

Sorry, I didn't realize 0.12's makefile was different than master in this way. Carry on, I suppose.

[mhdawson]

I tried io.js to see the behaviour there. I did the following

./configure
make
make test-npm

and I get this failure:

mhdawson@duv-aurora:~/iojs/io.js$ make
make -C out BUILDTYPE=Release V=1
make[1]: Entering directory `/home/mhdawson/iojs/io.js/out'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/home/mhdawson/iojs/io.js/out'
ln -fs out/Release/iojs iojs
mhdawson@duv-aurora:~/iojs/io.js$ make test-npm
make -C out BUILDTYPE=Release V=1
make[1]: Entering directory `/home/mhdawson/iojs/io.js/out'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/home/mhdawson/iojs/io.js/out'
ln -fs out/Release/iojs iojs
NODE_EXE=iojs tools/test-npm.sh
module.js:339
    throw err;
          ^
Error: Cannot find module '/home/mhdawson/iojs/io.js/test-npm/cli.js'
    at Function.Module._resolveFilename (module.js:337:15)
    at Function.Module._load (module.js:287:25)
    at Function.Module.runMain (module.js:472:10)
    at startup (node.js:124:18)
    at node.js:960:3
make: *** [test-npm] Error 1
This is on an ubuntu 14.04 machine

[Fishrock123]

@mhdawson Btw if you put code between two ``` it will format nicer. :)

That sounds like the files didn't copy correctly. Does cp -r work on your machine? (I don't have access to linux..)

[mhdawson]

cp -r does work on my machine

I also wondered about the use of shell scripts and how that affects windows. Has that been tested ?

[Fishrock123]

I also don't have good access to windows, so that wasn't tested yet, if I can improve it, please let me know. :) (we don't have a good setup for this on the CI yet..)

[Fishrock123]

@mhdawson Looks like there was a slight difference between how BSD and GNU cp works: nodejs/node@1baba05

Can you try running make test-npm from io.js@master?

[Fishrock123]

(That is still unrelated to 0.10 / 0.12, @misterdjules this still needs review and probably landing for those older versions.)

[mhdawson]

I reached out last week to our contact at npm on Julien's suggestion to see if we can confirm the right way to resolve. I'll have to follow up again as I've not seen a response. As for trying out the current io.js test I'll try to check that out tomorrow.

[Fishrock123]

Probably just cc @othiym23 here then.

[othiym23]

@mhdawson Sorry, GitHub is probably a better way to get my attention for technical npm-related stuff (thanks for the page, @Fishrock123!). The last couple weeks have been kind of a mess for me attention-wise due to travel and conferencing, and email comes a distant second to GitHub when I'm in attention-conservation mode.

This change is necessary if and only if the build / test host doesn't previously have a copy of npm installed (npm test runs npm run test-legacy && npm run test -- it won't test the previously installed version of npm, but it will use it to run the tests). If it simplifies things to presume that no version of npm was previously installed, this is a simple way to make things work and should be exactly equivalent to what we had before.

I also wondered about the use of shell scripts and how that affects windows. Has that been tested ?

This is a good reason to stick with pure npm scripts or the Makefile. The shell script will probably work with msysgit's Bash, but won't work with Cygwin or plain cmd.exe or Powershell.

Frankly, the CI story for npm on Windows is poor right now. It's an outstanding item on the npm team's work list to get all of the tests passing on Windows again (a bunch of Unixisms have crept in over the last 16 months or so), and the current Makefile + package.json combo isn't really intended to support doing a full npm test + publish + release build on Windows. I would be delighted to accept patches to npm that make that experience smoother, but the team probably won't have much time to get to this anytime soon.

[mhdawson]

@othiym23. Up to know I believe the assumption has been that when testing npm with node the you could test without having an installed npm. Discussing with Julien I think we agreed it was desirable that you could just clone, build and then run test-npm.

In this context I think your reply might be agreeing with what I have in the pull request as the way to support this. Another alternative would be having an additional target that would run all required targets without requiring an install. Possibly a test-all-noinstall. The advantage of this is that if you added to the list that test-all runs you could also add to test-all-noinstall and there would be less chance that we'd miss running any new targets that are added. If that makes sense I'd be happy to submit a pull request to add that. We might then use the change in my current PR to resolve in Node until we upgrade to a new version of npm.

@Fishrock123 I updated from the io.js repo and re-ran. The current output looks like the same issue as I see with Node. ie since I have not already installed npm the test-all target now complains that it can't find it:

make -C out BUILDTYPE=Release V=1
make[1]: Entering directory `/home/mhdawson/iojs/io.js/out'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/home/mhdawson/iojs/io.js/out'
ln -fs out/Release/iojs iojs
NODE_EXE=iojs tools/test-npm.sh
npm WARN prefer global marked@0.3.3 should be installed with -g
npm WARN engine follow@0.11.4: wanted: {"node":"0.10.x || 0.8.x"} (current: {"node":"2.2.2","npm":"2.11.0"})
require-inject@1.2.0 node_modules/require-inject
deep-equal@1.0.0 node_modules/deep-equal
sprintf-js@1.0.2 node_modules/sprintf-js
marked@0.3.3 node_modules/marked
marked-man@0.1.4 node_modules/marked-man
npm-registry-mock@1.0.1 node_modules/npm-registry-mock
âââ util-extend@1.0.1
âââ hock@0.2.5 (deep-equal@0.2.1)
nock@2.2.0 node_modules/nock
âââ propagate@0.3.1
âââ lodash@2.4.1
âââ debug@1.0.4 (ms@0.6.2)
âââ chai@1.10.0 (assertion-error@1.0.0, deep-eql@0.1.3)
npm-registry-couchapp@2.6.9 node_modules/npm-registry-couchapp
âââ json@9.0.3
âââ couchapp@0.11.0 (watch@0.8.0, url@0.10.3, coffee-script@1.9.3, connect@3.3.5, nano@6.1.3, http-proxy@0.8.7)
tap@1.1.4 node_modules/tap
âââ exit-code@1.0.2
âââ foreground-child@1.2.0
âââ supports-color@1.3.1
âââ buffer-equal@0.0.1
âââ signal-exit@2.1.2
âââ readable-stream@1.1.13 (core-util-is@1.0.1, string_decoder@0.10.31, isarray@0.0.1)
âââ tap-parser@1.1.5 (events-to-array@1.0.2)
âââ tap-mocha-reporter@0.0.13 (escape-string-regexp@1.0.3, diff@1.4.0, debug@2.2.0)
âââ coveralls@2.11.2 (lcov-parse@0.0.6, log-driver@1.2.4, js-yaml@3.0.1, request@2.40.0)
âââ js-yaml@3.3.1 (esprima@2.2.0, argparse@1.0.2)
âââ nyc@2.2.1 (spawn-wrap@1.0.1, strip-bom@1.0.0, yargs@3.10.0, lodash@3.9.3, istanbul@0.3.14)
> npm@2.11.0 test-all /home/mhdawson/iojs/io.js/test-npm
> npm run test-legacy && npm test
sh: 1: npm: not found
npm ERR! Linux 3.13.0-49-generic
npm ERR! argv "/home/mhdawson/iojs/io.js/out/Release/iojs" "/home/mhdawson/iojs/io.js/test-npm/cli.js" "run-script" "test-all"
npm ERR! node v2.2.2
npm ERR! npm  v2.11.0
npm ERR! file sh
npm ERR! code ELIFECYCLE
npm ERR! errno ENOENT
npm ERR! syscall spawn
npm ERR! npm@2.11.0 test-all: `npm run test-legacy && npm test`
npm ERR! spawn ENOENT
npm ERR!
npm ERR! Failed at the npm@2.11.0 test-all script 'npm run test-legacy && npm test'.
npm ERR! This is most likely a problem with the npm package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run test-legacy && npm test
npm ERR! You can get their info via:
npm ERR!     npm owner ls npm
npm ERR! There is likely additional logging output above.
npm ERR! Please include the following file with any support request:
npm ERR!     /home/mhdawson/iojs/io.js/test-npm/npm-debug.log
make: *** [test-npm] Error 1

[Fishrock123]

@mhdawson correct.

I spoke with Rod about this recently, and we talked about making a (sym)link to deps/npm/cli.js and exporting that into the env from the Makefile when running the tests. However, i haven't had time to actually implement it all to try. Conversation on IRC: http://logs.libuv.org/io.js/2015-05-25#22:08:10.730

[othiym23]

I've discussed this with @mhdawson and @Fishrock123 and I think this is the simplest solution for now, and that it makes sense for it to be the responsibility of the parties downstreaming npm into Node to update this task should anything change in npm's tests.

[Fishrock123]

👍

[mhdawson]

@misterdjules if you are ok with our conclusion can you LGTM and I'll go ahead and land.

[misterdjules]

@mhdawson @othiym23 @Fishrock123 I haven't been part of the conversation so far, so accept my apologies if what I'm suggesting has already been discussed. Would that be better to have the test-all npm script be defined in npm's package.json as following:

"test-all": "node ./test/run.js && tap --timeout 240 test/tap/*.js

? More generally, not using the npm command in these test npm scripts seems like it would give the same result without the need to downstream changes into node every time they change.

It would also allow us to keep using test-all in the test-npm target in node, and thus would prevent future npm upgrades from missing important tests and potential bugs.

[Fishrock123]

See also: nodejs/node#1926

@misterdjules npm would like to reduce it's own duplication to which we agreed.

Plus, IIRC we figured it would be better for us to chose which tests we want to run, and it's not really a headache to update it with npm if something changes in the future.

[misterdjules]

@Fishrock123 What about the approach you mentioned in one of your previous comments? That seems like it would solve the problem without any of the drawbacks of this PR (needing to downstream changes and missing tests when upstream tests change).

[Fishrock123]

@misterdjules we discussed all of this extensively. That is unfortunately really complicated and kind of even more brittle.

[misterdjules]

@Fishrock123 I have no doubt about it, but I need to understand why it's more brittle than the current approach in order to review it. Would you mind sharing more details about why it's more brittle?

Also, if @othiym23 who was part of this discussion can review that PR, then that's enough for me since he's now a collaborator (he's not actually in the @joyent/node-collaborators team yet, but that's a technical detail and he'll be added when his onboarding is done hopefully next week) .

[mhdawson]

I think @othiym23 had looked at the PR and the result of the discussion is that he thought it was the way to go. @othiym23 if you did review closely enough to add your lgtm then it sounds like Julien would happy with that.

[mhdawson]

Filling in part of the discussion for @misterdjules we did discuss the aspect of making sure that what Node runs as a target stays up to date. The feeling was that it was best left as a task for the contributor that pulls in updates for npm, and since that it would likely be @othiym23 doing it there was no concern about getting it right.

[misterdjules]

@mhdawson Thank you for the info 👍 I had the feeling that the approach that @Fishrock123 had started to try would be much better. If you guys have determined it's not feasible, don't let me block you from making progress.

[mhdawson]

Been busy, will plan to pull this in this week

[mhdawson]

landed as dbda13a