[WIP] Update test-npm target #7867

gibfahn · 2016-07-25T11:23:52Z

Checklist

make -j4 test-npm (UNIX), or vcbuild test-npm (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test, npm

Description of change

Update the test-npm script to generate tap output, and add a windows version.

N.B. If the deps/npm change will be upstreamed in npm/npm#13735.

Ref: nodejs/build#317

CI: https://ci.nodejs.org/view/All/job/gibfahn-test-npm/
Windows CI: https://ci.nodejs.org/job/gibfahn-test-npm-win

Known Issues

test-npm is currently failing due to npm/npm#13457 which seems to be caused by #7168 (see #7168 (comment) for more info).

I've also seen failures due to npm/npm#12220.

Manual testing

~~Build gibfahn:rebased-test-npm-fix to get the #7168 revert as well.~~ Shouldn't be necessary now #7168 has landed

~~On windows, do set debug=1 to print the TAP output to stdout, otherwise it gets redirected to test-npm.tap (I haven't found a tee equivalent for batch).~~ Now using powershell instead of batch which allows use of Tee-Object

cc @thealphanerd

mscdex · 2016-07-25T12:16:29Z

lib/fs.js

@@ -122,6 +142,10 @@ function nullCheck(path, callback) {
  return true;
 }

+function isFd(path) {


Why the name change (isFD -> isFd)?

That's just reverting the commits from #7168 so test-npm can be run. Not part of the PR.

MylesBorins · 2016-07-25T16:17:54Z

@gibfahn there is already a PR in for the revert against fs

#7846

might be better to keep the two separated for the sake of simplicity. Perhaps you can build this PR on top of #7846?

gibfahn · 2016-07-25T16:40:10Z

@thealphanerd I've removed the two revert commits. If I rebase onto #7846 presumably the extra commits will still show up as this PR is against master. Testing these changes will require rebasing/adding the revert commits.

Simplicity of code review vs. simplicity of running the tests, not sure which one to prioritise...

EDIT: @thealphanerd could you rebase your PR?

gibfahn · 2016-07-25T17:09:21Z

Results after initial run:

Results on win:

1..258
# failed 157 of 258 tests
# skip: 1
# time=3884348.615ms

64 min

Results on Linux:

1..258
# failed 6 of 258 tests
# skip: 1
# time=528817.407ms

8.8 min

MylesBorins · 2016-07-25T18:03:28Z

tools/test-npm.sh

@@ -37,7 +38,7 @@ node cli.js rebuild
 # install npm devDependencies and run npm's tests
 node cli.js install --ignore-scripts
 # run the tests
-node cli.js run-script test-node
+node cli.js run-script test-node -- --reporter=tap | tee ../test-npm.tap


is this not something that could be exposed as a variable. That way we could use different reporters.

This is particularly important as we are having issues currently with the speed of the tap reporter, alternatively using an xunit reporter in CI could prove handy.

As well, I'm not sure that we need to save tap output in all instances, especially when running the tests locally, perhaps there is a way to also expose this behind an option?

I'll add a reporter variable for this and the .bat.

For the saving, is there a downside to having the .tap file left behind? It gets sent to the stdout as well. If there's a reporter option, then I could make it so I only set the tee if --reporter=tap or --reporter=junit.

It might be worth trying to replicate the current api exposed by the python test runner. An option to specify reporter, and an option to specify output file name -p tap --logfile test.tap

@thealphanerd So you're saying have a ./configure option of -p tap or --reporter tap and --logfile test.tap, and Makefile/vcbuild.bat variables called REPORTER and LOGFILE?

Follow up: --reporter or --progress? I'm leaning towards progress as it's what tools/test.py does.

I don't think it would necessarily be a configure option. Rather an option that could be passed to the shell script.

If we have that we could easily make a test-npm-ci job and call the script with the appropriate flags

richardlau · 2016-07-25T18:08:28Z

@gibfahn Since tools/test-npm.bat is a port of tools/test-npm.sh it would be good to keep the comments from tools/test-npm.sh so that it is easier to compare sections of the batch script to the shell script.

richardlau · 2016-07-25T18:10:04Z

tools/test-npm.bat

+node cli.js rebuild
+node cli.js install --ignore-scripts
+
+rem windows doesn't have tee, so not sure how to get output on screen and in file


Powershell has Tee-Object

Fishrock123 · 2016-07-26T08:00:46Z

cc @nodejs/build, the unix script part looks ok to me.

iarna · 2016-07-27T22:20:02Z

tools/test-npm.bat

+
+@rem run the tests
+if defined debug (
+  node_modules\.bin\tap.cmd --timeout 240 --reporter=tap test\tap


Why make these different then what we do in the shell script?

If the test-node target doesn't work on Windows then I'd like to patch that in npm.

It's important to that what tests get run remain in npm's control because it is something that will change over time. For instance, in the next release of npm there will be a new folder under test named network with tests that require the network and the test-node target will be updated to:

"test-node": "\"$NODE\" \"node_modules/.bin/tap\" --timeout 240 \"test/tap/*.js\" \"test/network/*.js\""

I was having some problems running it previously, this was a temporary workaround. I agree it makes sense to use the node cli.js run-script test-node script on windows and fix if necessary.

Aaand, yeah, now that I paste that in it obviously wouldn't work on Windows. Buuut, we're way better about ensuring that the version of node you ran npm with is the one used for lifecycle scripts, so we can actually rewrite that as:

"test-node": "tap --timeout 240 \"test/tap/*.js\" \"test/network/*.js\""

which will work on windows.

gibfahn · 2016-12-13T12:09:48Z

@bnoordhuis Okay, where would you like it? I guess I'd better write it in Node, that way people will be able to review it better.

bnoordhuis · 2016-12-13T12:21:59Z

tools/test-npm.js? We can always move or rename it later.

jkrems · 2016-12-14T03:43:48Z

FYI: To unblock testing on windows I added a temporary JS-port of tools/test-npm.sh to my node-inspect PR - 144c7dc

nodejs#7867 (comment)

bnoordhuis · 2017-01-24T11:50:35Z

@gibfahn What's the status of this? I can review it but it seems to be in need of a rebase.

Pass -p/--progress and --logfile as you would for tools/test.py Add *test.tap to .gitignore Make test-npm default to using the node binary in out/Release (like tools/test.py). This can still be changed by setting the $NODE environment variable (as before).

test-npm.ps1 should mimic test-npm.sh on Windows. Also adds the test-npm target to vcbuild.bat

gibfahn · 2017-01-24T12:15:56Z

@bnoordhuis Rebased (the change to deps/npm is no longer necessary as npm have updated their npm run-script test-node script).

The issue you raised in #7867 (comment) is that this requires separate scripts for windows and linux. The three ways to go forward with this are:

Use this as is (tools/test-npm.sh already exists, so it's the least change.
Use @jkrems's js port in 144c7dc
Include citgm in nodejs/node increases repo size by ~15MB.
Do an npm install citgm as part of make test-npm (requires internet access).

My preference would be to use citgm, we don't really want to be maintaining two module-testing scripts, and citgm is purpose built for node module testing. You can already do citgm ./deps/npm, and it works out of the box.

bnoordhuis · 2017-01-24T15:07:02Z

Is there a way to make it work with citgm that has zero configuration and zero points of failures?

For example, needing to manually check out citgm counts as configuration, automatically checking it out as a potential point of failure.

gibfahn · 2017-01-24T16:48:52Z

Obviously you'd have to get citgm from somewhere, so you could either check it in (makes nodejs/node bigger) or npm install citgm (network dependency).

The reason to use citgm is that it already handles various module testing oddities and works with sandboxing/windows etc. Once you had it you'd be able to use it with other things like node-inspect.

Checking it in is what we do with eslint (tools/eslint is 18MB, citgm is about 15MB).

bnoordhuis · 2017-01-24T21:09:59Z

@nodejs/ctc See #7867 (comment) and #7867 (comment). Opinions on whether to vendor citgm or do something else?

mscdex · 2017-01-24T23:04:52Z

Personally I'm not too thrilled about increasing the size of the repo by that much, so I would prefer alternative options.

gibfahn · 2017-01-25T00:57:18Z

@mscdex the options are listed in #7867 (comment), which would you prefer?

mscdex · 2017-01-25T01:44:32Z

@gibfahn What are the downsides to option 2?

gibfahn · 2017-01-31T13:04:10Z

@mscdex I guess the downside is that we'd have to maintain what is essentially a subsection of citgm. The question is how big a subsection it is, if it's small then it's worth doing to keep the size down, if it's large then it becomes less useful.

Having taken a look at what citgm is actually doing, I'd say we should go ahead with 2., we can always replace it with citgm if it becomes unmanageable, and I'm also keen to keep the repo small.

@jkrems could you put 144c7dc in a separate PR so we can get it reviewed and in?

jkrems · 2017-01-31T16:56:06Z

@gibfahn I'd rather not open more PRs that I'd have to monitor, I already have 2 long running ones that I have to keep checking. But feel free to "steal" the code and PR it in isolation. :)

gibfahn · 2017-02-24T16:31:41Z

This is continued in #11540

Deletes the old test-npm.sh script. PR-URL: nodejs#11540 Refs: nodejs#7867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Deletes the old test-npm.sh script. PR-URL: #11540 Refs: #7867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Deletes the old test-npm.sh script. PR-URL: nodejs/node#11540 Refs: nodejs/node#7867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jul 25, 2016

ChALkeR added wip Issues and PRs that are still a work in progress. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. and removed meta Issues and PRs related to the general management of the project. labels Jul 25, 2016

gibfahn force-pushed the test-npm-fix branch from 564148b to 5f6c90b Compare July 25, 2016 12:12

mscdex reviewed Jul 25, 2016
View reviewed changes

mscdex added the npm Issues and PRs related to the npm client dependency or the npm registry. label Jul 25, 2016

gibfahn force-pushed the test-npm-fix branch 2 times, most recently from 852d866 to e8a503b Compare July 25, 2016 15:04

gibfahn mentioned this pull request Jul 25, 2016

CI Job for testing NPM nodejs/build#317

Closed

gibfahn force-pushed the test-npm-fix branch from e8a503b to a491dd5 Compare July 25, 2016 15:45

gibfahn force-pushed the test-npm-fix branch 2 times, most recently from 30d289e to 18125e6 Compare July 25, 2016 16:39

gibfahn force-pushed the test-npm-fix branch 3 times, most recently from ae014dc to b2d0924 Compare July 25, 2016 17:02

MylesBorins reviewed Jul 25, 2016
View reviewed changes

richardlau reviewed Jul 25, 2016
View reviewed changes

gibfahn force-pushed the test-npm-fix branch from b2d0924 to 67d3b59 Compare July 27, 2016 13:23

iarna reviewed Jul 27, 2016
View reviewed changes

gibfahn force-pushed the test-npm-fix branch from 67d3b59 to 19a29d6 Compare July 28, 2016 07:37

gibfahn force-pushed the test-npm-fix branch 2 times, most recently from 0729964 to a11a97b Compare August 4, 2016 11:39

gibfahn added a commit to gibfahn/node that referenced this pull request Dec 28, 2016

TEMP: patch to npm test-node script for npm test

c75f097

nodejs#7867 (comment)

gibfahn added 2 commits January 24, 2017 12:08

build,test: add test-npm.ps1 to test npm on windows

66bb063

test-npm.ps1 should mimic test-npm.sh on Windows. Also adds the test-npm target to vcbuild.bat

gibfahn force-pushed the test-npm-fix branch from 28b80e2 to 66bb063 Compare January 24, 2017 12:08

nodejs-github-bot added the lts-watch-v6.x label Jan 24, 2017

gibfahn mentioned this pull request Jan 31, 2017

deps: fix npm files from upgrade to 4.1.2 #11085

Closed

2 tasks

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

Fishrock123 self-assigned this Feb 14, 2017

gibfahn mentioned this pull request Feb 24, 2017

test: update test-npm to use test-npm-package.js #11540

Merged

4 tasks

gibfahn closed this Feb 24, 2017

gibfahn deleted the test-npm-fix branch June 3, 2017 14:34

MylesBorins pushed a commit that referenced this pull request Oct 23, 2017

test: update test-npm to use test-npm-package.js

6b1f75d

Deletes the old test-npm.sh script. PR-URL: #11540 Refs: #7867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins removed the lts-watch-v6.x label Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Update test-npm target #7867

[WIP] Update test-npm target #7867

gibfahn commented Jul 25, 2016 •

edited

Loading

mscdex Jul 25, 2016

gibfahn Jul 25, 2016

MylesBorins commented Jul 25, 2016

gibfahn commented Jul 25, 2016 •

edited

Loading

gibfahn commented Jul 25, 2016

MylesBorins Jul 25, 2016

gibfahn Jul 25, 2016

MylesBorins Jul 25, 2016

gibfahn Jul 25, 2016

MylesBorins Jul 25, 2016

richardlau commented Jul 25, 2016

richardlau Jul 25, 2016

Fishrock123 commented Jul 26, 2016 •

edited

Loading

iarna Jul 27, 2016

gibfahn Jul 28, 2016

iarna Jul 28, 2016 •

edited

Loading

gibfahn commented Dec 13, 2016 •

edited

Loading

bnoordhuis commented Dec 13, 2016

jkrems commented Dec 14, 2016

bnoordhuis commented Jan 24, 2017

gibfahn commented Jan 24, 2017 •

edited

Loading

bnoordhuis commented Jan 24, 2017

gibfahn commented Jan 24, 2017

bnoordhuis commented Jan 24, 2017

mscdex commented Jan 24, 2017

gibfahn commented Jan 25, 2017

mscdex commented Jan 25, 2017

gibfahn commented Jan 31, 2017

jkrems commented Jan 31, 2017

gibfahn commented Feb 24, 2017

[WIP] Update test-npm target #7867

[WIP] Update test-npm target #7867

Conversation

gibfahn commented Jul 25, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Known Issues

Manual testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Jul 25, 2016

gibfahn commented Jul 25, 2016 • edited Loading

gibfahn commented Jul 25, 2016

Results on win:

Results on Linux:

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

richardlau commented Jul 25, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iarna Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

gibfahn commented Dec 13, 2016 • edited Loading

bnoordhuis commented Dec 13, 2016

jkrems commented Dec 14, 2016

bnoordhuis commented Jan 24, 2017

gibfahn commented Jan 24, 2017 • edited Loading

bnoordhuis commented Jan 24, 2017

gibfahn commented Jan 24, 2017

bnoordhuis commented Jan 24, 2017

mscdex commented Jan 24, 2017

gibfahn commented Jan 25, 2017

mscdex commented Jan 25, 2017

gibfahn commented Jan 31, 2017

jkrems commented Jan 31, 2017

gibfahn commented Feb 24, 2017

gibfahn commented Jul 25, 2016 •

edited

Loading

gibfahn commented Jul 25, 2016 •

edited

Loading

Fishrock123 commented Jul 26, 2016 •

edited

Loading

iarna Jul 28, 2016 •

edited

Loading

gibfahn commented Dec 13, 2016 •

edited

Loading

gibfahn commented Jan 24, 2017 •

edited

Loading