tools: use local or specified $NODE for test-npm #1984

Fishrock123 · 2015-06-15T16:13:04Z

See: #1955 (comment)

cc @nodejs/build

Fishrock123 · 2015-06-15T16:13:33Z

Makefile

@@ -136,7 +136,7 @@ test-debugger: all
 	$(PYTHON) tools/test.py debugger

 test-npm: $(NODE_EXE)


These are still correct, right? I'm not too sure how makefile uses this part.

Yeah, NODE_EXE is mostly used as build target.

jbergstroem · 2015-06-16T00:18:15Z

tools/test-npm.sh

-../$NODE_EXE cli.js install --ignore-scripts
-../$NODE_EXE cli.js run-script test-all
+../$NODE cli.js install --ignore-scripts
+../$NODE cli.js run-script test-all


Aren't we changing directory a few lines up? Thinking this should just be $NODE. Guess it depends on how you pass the binary path.

It's not ./node because at this point we are inside the test-npm directory. :)

Yeah, you're right

Fishrock123 · 2015-06-17T04:46:43Z

@jbergstroem is this lgty?

jbergstroem · 2015-06-17T04:57:29Z

I haven't tried it but the change LGTM.

Fishrock123 · 2015-06-17T17:16:21Z

Rebased and updated, tested, can confirm it still works.

Unfortunately npm's tests still require a global node for now. ...unless that can be passed via env variable?

Fishrock123 · 2015-06-17T17:18:54Z

tools/test-npm.sh

+
+../$NODE cli.js install --ignore-scripts
+../$NODE cli.js run-script test-legacy
+../$NODE cli.js run-script test


Actually on second thought this always used the local node to run cli.js anyways because these are relative links.

Still, I think this PR is better practice given our makefile setup.

Fishrock123 · 2015-06-17T18:04:56Z

tools/test-npm.sh

@@ -3,11 +3,11 @@
 set -e

 # always change the working directory to the project's root directory
-cd $(dirname $0)/..
+cd $(dirname ${BASH_SOURCE[0]})/..


see: http://stackoverflow.com/a/9107028/1279026 (related comment linked below in the "get absolute path" part).

Seems zsh doesn't sport this variable unfortunately. There is an alternative, but I'd rather avoid using shell-specific things, if possible.

Ok, Will revert to using $0

Hmm, what issue were you trying to solve here exactly? Maybe there's an alternative way.

Nothing really, I just saw it was suggested in the stackoverflow, going to revert to using $0

Fishrock123 · 2015-06-17T18:07:05Z

Updated, PTAL.

It now creates a local node link and exports it to the used $PATH, this should run the tests using only the local node binary.

Will also test on my assigned linux machine.

Fishrock123 · 2015-06-17T18:39:14Z

tools/test-npm.sh

+
+## a hopefully temporary hack to use the local $NODE binary when calling node(1)
+# make a link to $NODE as test-npm/node
+ln -f $NODE test-npm/node


This may be an issue cross-platform.

My OS X ln cannot do relative symlinks (-r), but linux does not appear to like hard links.

silverwind · 2015-06-18T05:01:04Z

I wonder if we have other bash-specific things in those test-npm scripts. Could be an explanation for the weird issues I was having with test-npm on zsh.

Fishrock123 · 2015-06-19T18:25:57Z

PTAL: Updated to remove this bit, which I'll PR separately:

EDIT: on second thought it is kind of useless without the hack, so I'm re-adding it again.

Fishrock123 · 2015-06-19T18:52:31Z

tools/test-npm.sh

+popd > /dev/null
+# export that link as preferred in the $PATH
+export PATH=$PWD:$PATH
+##


Ok I can confirm this works on both OS X and Linux (Ubuntu 15), it just means we have an extra thing to clean up in the project root, as seen at the bottom of this file.

Looks like this fails on a more recent npm with these errors:

test/tap/lifecycle-path.js ............................ 2/4 make sure the path is correct not ok exit code +++ found --- wanted -0 +1 compare: === at: file: test/tap/lifecycle-path.js line: 55 column: 7 stack: | test/tap/lifecycle-path.js:55:7 f (node_modules/once/once.js:17:25) ChildProcess.<anonymous> (test/common-tap.js:56:5) maybeClose (internal/child_process.js:764:16) Socket.<anonymous> (internal/child_process.js:319:11) Pipe._onclose (net.js:467:12) make sure the path is correct not ok should be equivalent +++ found --- wanted [ - "{{ROOT}}/bin/node-gyp-bin" - "{{ROOT}}/test/tap/lifecycle-path/node_modules/.bin" - "/bin" - "/usr/bin" + "> ./node-bin/node print-path.js" ] at: file: test/tap/lifecycle-path.js line: 74 column: 7 stack: | test/tap/lifecycle-path.js:74:7 f (node_modules/once/once.js:17:25) ChildProcess.<anonymous> (test/common-tap.js:56:5) maybeClose (internal/child_process.js:764:16) Socket.<anonymous> (internal/child_process.js:319:11) Pipe._onclose (net.js:467:12)

From this test: https://github.com/npm/npm/blob/master/test/tap/lifecycle-path.js

@othiym23 is this worth the effort to hack this in before npm's tests work without a global node? It seems like this test is relying on the location of the node binary, is that something that can / should be fixed?

Fishrock123 · 2015-06-19T18:52:48Z

@silverwind / @jbergstroem ptal again. :)

Fishrock123 · 2015-06-24T22:32:37Z

Anyone able to review? :D

evanlucas · 2015-06-25T04:16:49Z

Hmm, getting some failures...not sure if it is related to the most recent changes you made or not though...

https://cloudup.com/cmC7mdzkL4x

Fishrock123 · 2015-06-25T04:46:32Z

@evanlucas what OS? Those look like npm failures. If my script is broken on your system the tests will almost certainly fail to run. :P

evanlucas · 2015-06-25T04:50:58Z

Yosemite. Haven't tried running them standalone yet though

Fishrock123 · 2015-06-25T05:35:39Z

Uh oh, the same two tests are failing for me now when rebased on master, but only with this patch? How is that possible... I'll have to investigate.

Edit: must have something to do with setting the $PATH.

silverwind · 2015-07-02T21:16:00Z

tools/test-npm.sh

+PWD=`pwd -P`
+popd > /dev/null
+# export that link as preferred in the $PATH
+export PATH=$PWD:$PATH


I think the original PATH should be restored after the test run? (Maybe through saving it to a temp var)

@silverwind you mean after the legacy tests? I could try that.

I mean having PATH=$PWD:$PATH will prepend the directory on each run of test-npm.sh, resulting in PATH getting longer and longer.

Hmmm, nope. It appears the tests themselves depend on a global node (from the tap module, I think?)

Since running the test requires it, and causes it to fail, that's not a possibility.

Not sure you understand what I mean. My issue is with the global shell's PATH being in an modified state after this script finishes executing (global node now pointing to the built one), and I think we should restore PATH after this script is done.

Maybe just removing the export is sufficient so all child scripts get the correct binary and we avoid touching the global PATH.

Fishrock123 · 2015-07-24T18:49:05Z

Updated to remove the $PATH hack. I'll try working on it separately. PTAL please, would like to land this haha.

silverwind · 2015-07-24T20:03:18Z

Looks pretty straightforward now. LGTM

jbergstroem · 2015-07-24T22:19:44Z

LGTM

PR-URL: nodejs#1984 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

Fishrock123 added the build Issues and PRs related to build files or the CI. label Jun 15, 2015

Fishrock123 reviewed Jun 15, 2015
View reviewed changes

Fishrock123 added test Issues and PRs related to the tests. npm Issues and PRs related to the npm client dependency or the npm registry. and removed build Issues and PRs related to build files or the CI. labels Jun 15, 2015

jbergstroem reviewed Jun 16, 2015
View reviewed changes

Fishrock123 force-pushed the fix-test-npm branch from 496cac6 to e487c35 Compare June 17, 2015 17:14

Fishrock123 reviewed Jun 17, 2015
View reviewed changes

Fishrock123 force-pushed the fix-test-npm branch from e487c35 to 3967162 Compare June 17, 2015 18:03

Fishrock123 reviewed Jun 17, 2015
View reviewed changes

Fishrock123 changed the title ~~tools: use local or specified NODE for test-npm~~ tools: use local or specified $NODE for test-npm Jun 19, 2015

Fishrock123 force-pushed the fix-test-npm branch from 3967162 to 1bbc4ed Compare June 19, 2015 18:25

Fishrock123 force-pushed the fix-test-npm branch 2 times, most recently from 05516f5 to 1959fd0 Compare June 19, 2015 18:47

Fishrock123 reviewed Jun 19, 2015
View reviewed changes

Fishrock123 force-pushed the fix-test-npm branch from 1959fd0 to d6dc006 Compare June 21, 2015 19:08

rvagg force-pushed the master branch from 9d21135 to 628a3ab Compare June 25, 2015 09:18

Fishrock123 self-assigned this Jul 1, 2015

silverwind reviewed Jul 2, 2015
View reviewed changes

Fishrock123 mentioned this pull request Jul 12, 2015

deps: upgrade npm to 2.13.0 #2152

Closed

brendanashworth added the tools Issues and PRs related to the tools directory. label Jul 19, 2015

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

Fishrock123 force-pushed the fix-test-npm branch from d6dc006 to d566029 Compare July 24, 2015 18:47

Fishrock123 force-pushed the fix-test-npm branch from d566029 to 4303372 Compare July 24, 2015 18:48

tools: use local or specified $NODE for test-npm

68b06e9

PR-URL: nodejs#1984 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

Fishrock123 force-pushed the fix-test-npm branch from 4303372 to 68b06e9 Compare July 25, 2015 00:59

Fishrock123 merged commit 68b06e9 into nodejs:master Jul 25, 2015

cjihrig mentioned this pull request Jul 28, 2015

Release Proposal: v2.5.0 #2239

Merged

Fishrock123 mentioned this pull request Oct 15, 2015

test: update test-npm to work with npm 3 #3308

Closed

		@@ -136,7 +136,7 @@ test-debugger: all
		$(PYTHON) tools/test.py debugger

		test-npm: $(NODE_EXE)

tools: use local or specified $NODE for test-npm #1984

tools: use local or specified $NODE for test-npm #1984

Conversation

Fishrock123 commented Jun 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jun 17, 2015

jbergstroem commented Jun 17, 2015

Fishrock123 commented Jun 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jun 17, 2015

Choose a reason for hiding this comment

silverwind commented Jun 18, 2015

Fishrock123 commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jun 19, 2015

Fishrock123 commented Jun 24, 2015

evanlucas commented Jun 25, 2015

Fishrock123 commented Jun 25, 2015

evanlucas commented Jun 25, 2015

Fishrock123 commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jul 24, 2015

silverwind commented Jul 24, 2015

jbergstroem commented Jul 24, 2015