-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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 use test-npm-package.js #11540
Conversation
cc/ @nodejs/npm @bnoordhuis @Fishrock123 @mscdex @jkrems (from the previous PR) |
👍 |
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.
Not sure I follow, as the person who pretty much always does the npm reviews. Why are we changing this?
@Fishrock123 do you remember #7867 ? The end result of that PR was that a js script was better than a bash script + a Powershell script (which is what's in #7867). I've got CI jobs set up to run Windows + Unix, and the plan is to be able to do full CI runs on all our platforms. Note that the script was written by @jkrems for EDIT: Note that CI isn't properly passing yet, it'll need to be passing (or only have known npm failures) before this can be merged. |
ping @gibfahn :-) |
84f81c2
to
ebf2e50
Compare
deps/npm/package.json
Outdated
@@ -204,7 +204,7 @@ | |||
"sprintf-js": "~1.0.3", | |||
"standard": "~6.0.8", | |||
"tacks": "~1.2.2", | |||
"tap": "~9.0.3" | |||
"tap": "*" |
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.
This change will be overwritten when we update npm. I think it's better to not edit the files under the dep
directory for a specific reason except deps-update.
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.
Yeah, I'm just doing this so I can run the tests with the -J
option, which is only available in the latest version of tap (that way the tests run 4x faster). This won't land with any changes to deps/npm
.
Ping @gibfahn are you still working on this? |
Looks like this got rebased and the comments addressed. PTAL @jasnell @nodejs/collaborators |
The change still LGTM |
ee5126e
to
27a5d4f
Compare
Thanks for fixing up the windows tests @refack . @nodejs/npm does this LGTY? Once this lands I'll fix up the jobs so we have a |
Ping @nodejs/npm @zkat |
I'll dig through this, this week. |
6d7f8e4
to
0b24fca
Compare
I think we can just go ahead and land hits @gibfahn ? |
Deletes the old test-npm.sh script. PR-URL: nodejs#11540 Refs: nodejs#7867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
ba5a44b
to
532d8b2
Compare
Agreed @BridgeAR , it's not perfect, but at least we have the ability to run CI on npm now. @nodejs/collaborators , you should now have access to:
These run |
Deletes the old test-npm.sh script. PR-URL: nodejs/node#11540 Refs: nodejs/node#7867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Should this be backported to |
Deletes the old test-npm.sh script. PR-URL: nodejs/node#11540 Refs: nodejs/node#7867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Also deletes the old test-npm.sh script.
NOTE: Changes to
deps/npm
are for testing purposes only and won't be landed in the final PR.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test
This replaces #7867
The tests can be run manually with:
or