-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools?: refactor test-npm into shell script #1662
Conversation
npm_config_tmp="npm-tmp" \ | ||
../$NODE_EXE cli.js install --ignore-scripts && \ | ||
../$NODE_EXE cli.js run-script test-all && \ | ||
../$NODE_EXE cli.js prune --prod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps this stanza could be improved, but I'm not so sure how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a script, if I'm not mistaken you can just export npm_config_cache="bloo-blee-bloo"
and it won't affect the outer environment. Then you can un-chain the cli runs, since set -e
is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a script, if I'm not mistaken you can just
export npm_config_cache="bloo-blee-bloo"
and it won't affect the outer environment.
Ok, not sure if this is the best to check but make test-npm && printenv
didn't see anything new.
Then you can un-chain the cli runs, since
set -e
is enabled.
I don't think I understand, why wouldn't that work without set -e
?
7dea3b6
to
5a8fd8a
Compare
Shell script looks good to me. Quick question, the output of |
Those could be some good next steps, yes. Currently it just prints out everything that Some of it is already (parsed) tap output, the rest could probably be tap-ified on npm's end. |
@jbergstroem actually that might just be npm installing it's devDependencies. I'll take a look tomorrow. |
@Fishrock123 also, I seem to get these artefacts as a result of
..that userconfig looks like a bug? |
@jbergstroem currently, yes. This patch resolves that, please try it. :) (userconfig is just a shortcoming in npm's tests..) |
@Fishrock123 ah, I probably didn't pull your latest update. I'll try again. Edit: Typo in checkout.. see above ( |
LGTM |
5a8fd8a
to
5bf6c87
Compare
Updated the NODE_EXE check to |
I can't seem to call the script by itself with a NODE_EXE and have it work, any idea why running it from make works, but without doesn't? |
@Fishrock123 works for me: $ NODE_EXE=./iojs tools/test-npm.sh
npm WARN prefer global [email protected] should be installed with -g
npm WARN engine [email protected]: wanted: {"node":"0.10.x || 0.8.x"} (current: {"node":"2.0.1","npm":"2.9.0"})
[email protected] node_modules/require-inject
<snip> |
Doh. I was trying to do NODE_EXE=./node without thinking about it. >_< |
Extracts test-npm from Makefile and puts it in tools/test-npm.sh Also improves test-npm to use a separate copy of deps/npm for testing. PR-URL: nodejs#1662 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
5bf6c87
to
0b21ab1
Compare
Fixes #1549 |
Extracts test-npm from Makefile and puts it in tools/test-npm.sh Also improves test-npm to use a separate copy of deps/npm for testing. PR-URL: nodejs#1662 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
As suggested, test-npm now resides in a separate script file, and makes a copy of deps/npm to run the tests on.
R=@othiym23 / @iojs/build / @evanlucas