Skip to content
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

Merged
merged 1 commit into from
May 12, 2015

Conversation

Fishrock123
Copy link
Contributor

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

@Fishrock123 Fishrock123 added test Issues and PRs related to the tests. npm Issues and PRs related to the npm client dependency or the npm registry. labels May 8, 2015
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
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

@jbergstroem
Copy link
Member

Shell script looks good to me. Quick question, the output of test-npm looks very verbose. Is this needed/intentional? If we were to use stdout from jenkins as a tap provider we need to do something about it.

@Fishrock123
Copy link
Contributor Author

If we were to use stdout from jenkins as a tap provider we need to do something about it.

Those could be some good next steps, yes. Currently it just prints out everything that ./npm run-script test-all outputs.

Some of it is already (parsed) tap output, the rest could probably be tap-ified on npm's end.

@Fishrock123
Copy link
Contributor Author

@jbergstroem actually that might just be npm installing it's devDependencies. I'll take a look tomorrow.

@jbergstroem
Copy link
Member

@Fishrock123 also, I seem to get these artefacts as a result of make test-npm:

/Volumes/sink/io.js ±remotes/iojs/pr/1562 $ git status
HEAD detached at iojs/pr/1562
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

    modified:   deps/npm/test/fixtures/config/userconfig-with-gc

Untracked files:
  (use "git add <file>..." to include in what will be committed)

    deps/npm/node_modules/.bin/

..that userconfig looks like a bug?

@Fishrock123
Copy link
Contributor Author

@jbergstroem currently, yes. This patch resolves that, please try it. :)

(userconfig is just a shortcoming in npm's tests..)

@jbergstroem
Copy link
Member

@Fishrock123 ah, I probably didn't pull your latest update. I'll try again.

Edit: Typo in checkout.. see above (s/1562/1662/g 😭) Looks all good.

@evanlucas
Copy link
Contributor

LGTM

@Fishrock123
Copy link
Contributor Author

Updated the NODE_EXE check to if [ -z $NODE_EXE ]; then, ptal, landing after this review.

@Fishrock123
Copy link
Contributor Author

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?

@jbergstroem
Copy link
Member

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

@Fishrock123
Copy link
Contributor Author

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]>
@Fishrock123 Fishrock123 merged commit 0b21ab1 into nodejs:master May 12, 2015
@Fishrock123
Copy link
Contributor Author

Fixes #1549

Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 19, 2015
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants