-
-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
package.json
Outdated
@@ -36,9 +38,10 @@ | |||
"./immediate.js": "./immediate-browser.js" | |||
}, | |||
"scripts": { | |||
"test": "standard && node test.js | faucet", | |||
"test": "standard && nyc --silent node test.js | faucet", |
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.
FYI @ralphtheninja --silent
disables the coverage summary at the end of test output
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.
I think we should enable it and remove faucet.
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.
I'm okay with either. On the one hand, faucet reduces the output, on the other, the coverage summary is kinda nice (though when coverage matters, I prefer using nyc report --reporter=html
).
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.
@ralphtheninja your call
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.
It would be optimal if we could have both faucet
and the coverage report (maybe we can figure out a solution for this somehow?).
I just think it's really useful to have that report while testing locally. "Did I fuck up coverage? Did coverage change?"
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.
Maye we can just hack faucet
to let stuff pass through. I'm guessing it's faucet
that prevents the nyc
output?
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.
level-faucet
:)
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.
Probably difficult because tap-parser
considers the test to be complete after
1..735
# tests 735
# pass 735
# ok
Perhaps when tap-parser
emits "complete", we can switch stdin to pipe to stdout.
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.
@ralphtheninja it's a rabbit hole, mind if open an issue for later?
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.
@ralphtheninja it's a rabbit hole, mind if open an issue for later?
Oh definitely. Lets solve this later. I'm happy as long as I can get the coverage report. Let's just remove faucet
now and bring it back later.
Yes, it works! https://coveralls.io/jobs/37997408 |
Note that this setup doesn't test that coverage is 100% in each browser. It's cumulative. E.g. if you have this code: if (runningInChrome) {
// do this
} else {
// do that
} Coverage would be 50% if you only test Chrome, and 100% if you test Chrome and Firefox. |
I think that's perfectly fine. |
I agree :) |
package.json
Outdated
"test-browser-local": "airtap --no-coverage --local 9000 test.js", | ||
"coverage": "istanbul cover -i memdown.js ./node_modules/.bin/tape ./test.js && istanbul check-coverage --lines 90 --function 80 --statements 90 --branches 80", | ||
"report-coverage": "npm run coverage && istanbul-coveralls" | ||
"test": "standard && nyc --silent node test.js | faucet", |
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.
What does --silent
do?
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.
See above ;) #169 (comment)
Ignore the failed build, I removed the |
Good to merge, pending travis |
Testing airtap/airtap#194. Closes #153.