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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

iarna
Copy link
Member

@iarna iarna commented Oct 10, 2015

This is a prerequisite for bringing in npm 3. This will still work with npm 2.

The reason this is a problem is that in npm 3, test-legacy and test both run standard against the source tree. When test-legacy runs it installs things into its temp directory, which with node's make test-npm, is located in the npm folder itself. So when npm run-script test executes, it runs standard, which sees the things installed by test-legacy and refuses to continue. Using test-all solves this problem by combining test-legacy with test all in a single command.

npm itself never puts the temp folder in npm's source folder and so had never encountered this on its own.

r: @Fishrock123
r: @chrisdickinson

@mscdex mscdex added npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory. labels Oct 10, 2015
@Fishrock123
Copy link
Contributor

Unfortunately, I don't think this will quite work. We moved to this change a while ago so that we could run make test-npm Without a globally installed npm.

Is it possible to fix this on npm's end? I'll take a deeper look when I am back at my computer.

@iarna
Copy link
Member Author

iarna commented Oct 10, 2015

Oh yes, I remember that now, yeah, the version you have currently doesn't do what you want with npm 3 either, because our scripts are built a few layers deep. I'll update this patch in a little bit.

@iarna iarna force-pushed the npm-3-tests branch 2 times, most recently from 147ea7b to e31128c Compare October 10, 2015 01:16
@iarna
Copy link
Member Author

iarna commented Oct 10, 2015

Ok, I've updated the PR, making this the npm@3 equivalent of the old npm@2 incantation.

@Fishrock123
Copy link
Contributor

@iarna hmm, something isn't right here.

I'm using node @ master with the following patches applied:

(This and #3310)

curl -L https://github.com/nodejs/node/pull/3308.patch | git am --whitespace=fix
...
curl -L https://github.com/nodejs/node/pull/3310.patch | git am --whitespace=fix

(And also the following diff, to simplify output.)

diff --git a/src/node_version.h b/src/node_version.h
index 37dee1e..39b52ae 100644
--- a/src/node_version.h
+++ b/src/node_version.h
@@ -5,7 +5,7 @@
 #define NODE_MINOR_VERSION 0
 #define NODE_PATCH_VERSION 0

-#define NODE_VERSION_IS_RELEASE 0
+#define NODE_VERSION_IS_RELEASE 1

 #ifndef NODE_STRINGIFY
 #define NODE_STRINGIFY(n) NODE_STRINGIFY_HELPER(n)
diff --git a/tools/test-npm.sh b/tools/test-npm.sh
index cbb76cc..ccaa716 100755
--- a/tools/test-npm.sh
+++ b/tools/test-npm.sh
@@ -31,7 +31,7 @@ export npm_config_tmp="npm-tmp"
 # install npm devDependencies and run npm's tests

 ../$NODE cli.js install --ignore-scripts
-../$NODE cli.js run-script test-legacy
+# ../$NODE cli.js run-script test-legacy
 ../$NODE cli.js run-script tap -- \"test/tap/*.js\"

 # clean up everything one single shot

And I'm getting this output, where tap just hangs forever at the end.

Jeremiahs-MacBook-Pro:node Jeremiah$ make test-npm
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C out BUILDTYPE=Release V=1
make[1]: Nothing to be done for `all'.
ln -fs out/Release/node node
NODE=./node tools/test-npm.sh
npm WARN engine follow@0.12.1: wanted: {"node":"0.12.x || 0.10.x || 0.8.x"} (current: {"node":"5.0.0","npm":"3.3.6"})
npm WARN prefer global coffee-script@1.10.0 should be installed with -g
npm WARN prefer global js-beautify@1.5.10 should be installed with -g
npm WARN prefer global coffee-script@1.10.0 should be installed with -g
npm WARN prefer global js-beautify@1.5.10 should be installed with -g
npm@3.3.6 /Users/Jeremiah/Documents/node/test-npm
├─┬ cmd-shim@2.0.1
│ └── graceful-fs@3.0.8 
├── deep-equal@1.0.1 

...

  ├─┬ tap-parser@1.2.2 
  │ ├── events-to-array@1.0.2 
  │ └── readable-stream@2.0.2 
  └── tmatch@1.0.2 


> npm@3.3.6 tap /Users/Jeremiah/Documents/node/test-npm
> tap --timeout 240 "\"test/tap/*.js\""

Reading TAP data from stdin (use "-" argument to suppress)
^Cmake: *** [test-npm] Interrupt: 2

Jeremiahs-MacBook-Pro:node Jeremiah$

So I think, maybe this doesn't quite interact correctly with how we are setting up the test environment from the shell script?

@iarna
Copy link
Member Author

iarna commented Oct 15, 2015

Ok, I built my own integration branch and merged these two and also took my existing node/npm's out of my path and I'm running the test suite now. As soon as it passes I'll push an update to the npm-3-tests branch.

@iarna
Copy link
Member Author

iarna commented Oct 15, 2015

Ok, I've pushed an update that runs through all the tests successfully (at least in my integration branch).

Alternatively, I've ALSO prepared a patch to npm itself (which we will likely integrate regardless) to make it very careful to always use the version of node and npm that the test suite was originally initiated with. You can see that over here: npm/npm#9982

@Fishrock123
Copy link
Contributor

@iarna does npm/npm#9982 mean we could just run test-all?

@iarna
Copy link
Member Author

iarna commented Oct 15, 2015

@Fishrock123 Yes, but I have to convince folks that we should land something that can't work on windows. (Our test suite currently is mostly broken on windows, but this wouldn't be helping. Making it all work on windows will be a Project.)

../$NODE cli.js install --ignore-scripts
../$NODE cli.js run-script test-legacy
../$NODE cli.js run-script test
NODEPATH="$(../$NODE -e 'console.log(require("path").resolve(".."))')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node -p :)

Fwiw I had previously attempted to do something similar in a previous version of PR 1984: #1984 (comment)

Does the npm patch remove the need for this? I.e. will the new npm patch run without a global node? If so, that would be great for us. :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should, as it executes all other instances w/ the NODE env var set by npm based on process.execPath.

And oh, that's much easier =D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing w/ -p

@Fishrock123
Copy link
Contributor

@iarna The changes to test/run.js and test/tap/outdated-depth-deep.js would be windows-safe though, I think?

If so, maybe it would be better to float that and only fix test-legacy here.

@iarna
Copy link
Member Author

iarna commented Oct 15, 2015

@Fishrock123 Yes, and I'm sure there will be no objection to landing those. It's the whole way npm now builds its lifecycles out of other lifecycles that's painful. Previously it was just test-all that did this, but now ALL of the test lifecycles do this (ultimately calling a tap lifecycle that adds our standard arguments to tap). I may suggest adding a test-node lifecycle that directly does what test-legacy and test-tap do in a way that will use the version of node that it was run with.

@Fishrock123
Copy link
Contributor

Ok then I think the path forward would to be call the commands that test and test-legacy call in this script, and then float the other changes on the npm PR.

As far as I can tell that should make the PRs good-to-go, but I'll also do a test run in it's current state.

@Fishrock123
Copy link
Contributor

I am slightly concerned that tap may not pick up the correct node, however.

@iarna
Copy link
Member Author

iarna commented Oct 15, 2015

@Fishrock123 I was concerned about that too! So I double checked its source and it uses process.execPath to execute tests, so it should be fine as long it itself is executed with the right version of node.

NODEPATH="$(../$NODE -p 'require("path").resolve("..")')"
PATH="$NODEPATH:$PATH" ../$NODE cli.js install --ignore-scripts
PATH="$NODEPATH:$PATH" ../$NODE test/run.js
PATH="$NODEPATH:$PATH" ../$NODE cli.js run-script tap -- "test/tap/*.js"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok one thing here: could you do export PATH="$NODEPATH:$PATH" above these three lines? To my understanding it should give the same effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, it'll only matter if there's other work in this script that cares about PATH, which there isn't

@Fishrock123
Copy link
Contributor

LGTM minus a comment.

../$NODE cli.js install --ignore-scripts
../$NODE cli.js run-script test-legacy
../$NODE cli.js run-script test
NODEPATH="$(../$NODE -p 'require("path").resolve("..")')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rvagg can I get a quick LGTM on changing this line to export PATH="$(../$NODE -p 'require("path").resolve("..")'):$PATH"?

That will mean we no longer require the PATH="$NODEPATH:$PATH" on the 3 lines below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @nodejs/build

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that'll do, can be improved later perhaps

Fishrock123 added a commit that referenced this pull request Oct 22, 2015
Refs: #3308
PR-URL: #3489
Reviewed-By: Rod Vagg <rod@vagg.org>
@Fishrock123
Copy link
Contributor

Replaced by #3489 / ecab7a6...9b88864 - thanks for finding this in advance though!

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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants