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

Add nyc and browser code coverage #169

Merged
merged 6 commits into from
Jul 4, 2018
Merged

Add nyc and browser code coverage #169

merged 6 commits into from
Jul 4, 2018

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Jul 1, 2018

Testing airtap/airtap#194. Closes #153.

@vweevers vweevers self-assigned this Jul 1, 2018
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",
Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

@ralphtheninja your call

Copy link
Member

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?"

Copy link
Member

@ralphtheninja ralphtheninja Jul 1, 2018

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?

Copy link
Member

Choose a reason for hiding this comment

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

level-faucet :)

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

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.

@vweevers
Copy link
Member Author

vweevers commented Jul 1, 2018

Changed Coveralls to fail if coverage drops:

image

@vweevers
Copy link
Member Author

vweevers commented Jul 1, 2018

Yes, it works! https://coveralls.io/jobs/37997408

@vweevers
Copy link
Member Author

vweevers commented Jul 1, 2018

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.

@ralphtheninja
Copy link
Member

Note that this setup doesn't test that coverage is 100% in each browser.

I think that's perfectly fine.

@vweevers
Copy link
Member Author

vweevers commented Jul 1, 2018

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",
Copy link
Member

Choose a reason for hiding this comment

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

What does --silent do?

Copy link
Member Author

@vweevers vweevers Jul 1, 2018

Choose a reason for hiding this comment

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

See above ;) #169 (comment)

@vweevers vweevers mentioned this pull request Jul 1, 2018
@vweevers
Copy link
Member Author

vweevers commented Jul 1, 2018

Ignore the failed build, I removed the coverage branch from airtap.

@ralphtheninja ralphtheninja changed the title Add nyc and browser code coverage [wip] Add nyc and browser code coverage Jul 4, 2018
@vweevers vweevers removed their assignment Jul 4, 2018
@vweevers
Copy link
Member Author

vweevers commented Jul 4, 2018

Good to merge, pending travis

@ralphtheninja ralphtheninja merged commit 03ca6d2 into master Jul 4, 2018
@ralphtheninja ralphtheninja deleted the coverage branch July 4, 2018 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants