build: improve make test-npm #1576
Conversation
| ../../$(NODE_EXE) cli.js run-script test-all && \ | ||
| ../../$(NODE_EXE) cli.js prune --prod && \ | ||
| rm -rf node_modules/.bin && \ |
There was a problem hiding this comment.
holy indentation, let me fix that.
Fishrock123
force-pushed
the
improve-test-npm
branch
from
f3fb227 to
a8b6cbe
Compare
|
@rvagg maybe you could give this a quick review? |
|
/cc @jbergstroem |
|
/cc @iojs/build actually - anything that involves Makefile, vcbuild.bat, gyp or other buildy things should get the attention of the build crew |
|
ftr I've tried this out and had problems doing it on this in-flight wifi so while I can't give a proper +1 it at least looks like it's probably good, will try and get around to testing later but would be happy to have someone else review if they can |
| cd deps/npm ; npm_config_cache="$(shell pwd)/npm-cache" \ | ||
| npm_config_prefix="$(shell pwd)/npm-prefix" \ | ||
| npm_config_tmp="$(shell pwd)/npm-tmp" \ | ||
| ../../$(NODE_EXE) cli.js install --ignore-scripts && \ |
There was a problem hiding this comment.
@othiym23 I have a problem with the logic here -- we're testing the bundled npm works as it should with io.js but we should also be testing that it's bundled properly and ready to release yet we have an npm install inside the bundled directory which could potentially be fixing any bundled dependency problems that may exist. Is there a better way to approach this that would leave the deps/npm directory untouched?
There was a problem hiding this comment.
afaik npm install is required for it to do some linking properly.
There was a problem hiding this comment.
The bundled npm doesn't include the devDependencies (and npm doesn't bundle the devDependencies in the version on the registry, either). They need to be installed before the tests can be run (obvs), but another way to do it would be to copy deps/npm elsewhere and then do the npm install on that, nuking it after the tests have run. I'm increasingly of the opinion that the best way to do this is with a shell script called from the Makefile, as this is getting pretty complicated (and hard to follow) for a Makefile stanza.
There was a problem hiding this comment.
This could probably be simplified by removing the cleanup steps.. if something does go wrong, you don't want them cleaned up anyway, and if it doesn't go wrong, it gets cleaned up on the next run. Anything on CI should be using a fresh checkout, and anything release should only be running release related scripts and not these tests.
That might clear enough of the complexity budget to allow for the extra steps of copying to a separate location if there's some advantage to doing so, but it seems like validating the in-place correctness of the bundled files is a problem for another set of tests anyway.
There was a problem hiding this comment.
Not cleaning up after running the tests will leave a lot of stuff that isn't gitignored, which is why I added the new cleanup to begin with.
There was a problem hiding this comment.
Sounds more like a broken .gitignore file than a broken Makefile to me ;-)
But I see your point. Tests that frustrate the people who are encouraged to run them are self defeating.
There was a problem hiding this comment.
See #1567 (comment) - @othiym23 advised that this may be better handled by cleanup than gitignore.
|
This test actually fails for me (with no global node/io.js installed): /Volumes/sink/io.js/deps/npm ±remotes/iojs/pr/1576 $ ../../iojs cli.js run-script test-all
> [email protected] test-all /Volumes/sink/io.js/deps/npm
> npm run test-legacy && npm test
env: node: No such file or directory
npm ERR! Darwin 14.3.0
npm ERR! argv "/Volumes/sink/io.js/out/Release/iojs" "/Volumes/sink/io.js/deps/npm/cli.js" "run-script" "test-all"
npm ERR! node v2.0.0
npm ERR! npm v2.8.3
npm ERR! file sh
npm ERR! code ELIFECYCLE
npm ERR! errno ENOENT
npm ERR! syscall spawn
npm ERR! [email protected] test-all: `npm run test-legacy && npm test`
npm ERR! spawn ENOENT
npm ERR!
npm ERR! Failed at the [email protected] 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! /Volumes/sink/io.js/deps/npm/npm-debug.log
zsh: exit 1 ../../iojs cli.js run-script test-all..problem being |
| @@ -143,12 +143,10 @@ test-npm: $(NODE_EXE) | |||
| cd deps/npm ; npm_config_cache="$(shell pwd)/npm-cache" \ | |||
There was a problem hiding this comment.
I'd cache output of pwd and reuse instead of calling it three times.
Edit: Since we generally don;'t support building out other folders than source dir, you could probably even use $(CURDIR)
Right, but that's a little out of the cope of this I think. |
|
@Fishrock123 yeah perhaps - I just brought it up since I couldn't complete the test successfully. |
Fishrock123
force-pushed
the
improve-test-npm
branch
from
a8b6cbe to
4451733
Compare
|
@jbergstroem updated, ptal. :) |
I agree that would probably be better, will start on that next. |
|
replaced by #1662 |
|
(I guess maybe I didn't need to make a new PR, but oh well) |
replaces #1567
I think I'm doing this correctly, is there any reason it was separated into two commands? Did that make it parallel? It didn't seem to change per noticeably for me.
Also, now removes
deps/npm/node_modules/.bin, which is generated bymake test-npm.R=@othiym23