Use Github Commit Status API

[saulshanabrook]

https://github.com/blog/1227-commit-status-api

http://developer.github.com/v3/repos/statuses/

Comments can get annoying...

[coagulant]

If you have a long-running pull request, comments do get annoying.
👍 for commit status on github.
And, btw, currently comments can be turned off, but it's not helping to see if coverage decreased or not.

[tessi]

This is the first thing I noted after I got coveralls working. 👍

[joernhees]

👍

[maul-esel]

👎
Only the most recent status is shown on Github, thus coveralls would be hiding the status attached e.g. by Travis-CI (which I deem more important). Also, a status of "error" or "success" is not really appropriate here, and - on a related matter - decreasing coverage would cause pull requests to warn about merge, which is also inappropriate IMO.

Suggestion: Instead, go with a simpler solution and post a comment (as now), but instead of adding new comments with new commits, update the first comment. This doesn't clutter up the pull request overview, and sends no notifications to watching users, while still having the report in-place.

[nickmerwin]

@maul-esel that's a great suggestion, I think that's the way to go.

[fphilipe]

Just a heads up: Code Climate seems to be using the status API and hoping to work with GitHub to get support for multiple statuses.

[maul-esel]

Any news on this?

[groovecoder]

👍 for maul-esel's comment

[hugovk]

I like that Coveralls shows the coverage of pull requests at GitHub.

However, rather than commenting (which can send an email and show up on the front page feed), it'd be nicer if it could show the inline notifications similar to those by Travis CI.

[tomprince]

Github now supports displaying multiple statuses.
https://github.com/blog/1935-see-results-from-all-pull-request-status-checks

[hawkw]

👍

[TomNaessens]

👍

[GrahamCampbell]

👍

[GrahamCampbell]

@maul-esel's comment is no longer valid after github improved their status stuff quite recently. Yey! :)

[keradus]

👍

[ceeram]

👍 yes please, that would be very nice

[nathany]

Using the multi-status stuff in GitHub would be great. Plus the comments only work with TravisCI, it would also be nice if status worked with other CI services like CircleCI. Otherwise there is no link from a pull request to the coverage information.

[sstok]

👍

[jasonmp85]

👍 for switching to the Status API now that GitHub can display multiple statuses for a commit/request.

[mockdeep]

Adding it to the status would be great, especially if we were able to configure a minimum coverage ratchet. A close second option would be to only comment on changes in coverage, rather than for every commit, so as to reduce the noise.

[TomNaessens]

Awesome, found it. Thank you! 👍 for this addition!

[GrahamCampbell]

Ping @keradus. This is live now.

[jasonmp85]

If the coverage decreases, the status will be "failure".

So there can be literally no regression in coverage? Not even 0.01%? Or is there some user-defined threshold?

[nickmerwin]

@jasonmp85 if the PR includes un-tested lines of code, shouldn't it raise a red flag?

Seems like the PR status is just a barometer; you can still use the auto-merge button, having a failure status won't prevent you from merging.

[jasonmp85]

@nickmerwin ideally, yes. In practice I'm writing a PostgreSQL extension in C and sometimes I handle errors that are very difficult to reliably reproduce in their regression test environment, which is pretty black-box. So some lines of error handling may work out to basically be sanity checks that you'd never expect to be triggered (and indeed probably can't trigger without a totally ruined DB), but need to be there to make sure the world is sane.

But I see the argument and it makes sense. Allowing even 0.01% regression means you can slowly dip into "bad" territory over time (boiling frog analogy, etc.). I know the merge button is only advisory, but I'll accept this behavior to push me to prevent regressions.

[nickmerwin]

@jasonmp85 I like the analogy. Also "0.01%" or any seemingly small percentage might not be the right metric since in a code base of 100K relevant lines that'd still be 10 lines of potentially bug ridden code.

Maybe we should specify the actual line #'s also, e.g.:

Coverage decreased (X%) to Y% with Z new missed lines.

[keradus]

No need for pinging @GrahamCampbell ;)

[peternewman]

@nickmerwin I appreciate what you're saying about never decreasing coverage, however we use protobufs and I suspect it must be that (or bison or other bits we use elsewhere), but our coverage can end up going up or down even if all that's been changed are text files, e.g. OpenLightingProject/ola#610 or OpenLightingProject/ola#555 which will mean we'll get false positives if we can't set a threshold. I know we can ignore it, but then you're into the boy who cried wolf territory.

[jasonmp85]

You're measuring coverage on generated code? Kinda weird, IMO…

On Tue, Feb 3, 2015 at 4:17 PM, peternewman [email protected]
wrote:

@nickmerwin https://github.com/nickmerwin I appreciate what you're
saying about never decreasing coverage, however we use protobufs and I
suspect it must be that (or bison or other bits we use elsewhere), but our
coverage can end up going up or down even if all that's been changed are
text files, e.g. OpenLightingProject/ola#610
OpenLightingProject/ola#610 or
OpenLightingProject/ola#555
OpenLightingProject/ola#555 which will mean
we'll get false positives if we can't set a threshold. I know we can ignore
it, but then you're into the boy who cried wolf territory.

—
Reply to this email directly or view it on GitHub
#25 (comment)
.

[jasonmp85]

@nickmerwin I'm not seeing this on my project… For instance, citusdata/pg_shard#72 was just pushed. It moves around just a few SQL files, so the coverage remained the same. I see the comment, but not the status. What's up?

[peternewman]

@jasonmp85 same here, I turned off the comments and no change. I've just tried turning off both sets, saving and re-enabling, so will see if that fixes it somehow.

Regarding coverage of generated code, I see your point, TBH we had the tests already, I just gathered the test data and pushed it to coveralls. However given breaking the generated code will break the program, surely we want to confirm the tests cover the generated code well too, hence wanting good coverage?

[nickmerwin]

@jasonmp85 looking into that now; the GH API is returning a 404 error when attempting to create a status on that PR.

[jasonmp85]

@peternewman I'm just saying that coverage of generated code is irrelevant. Your tests should still fail if it breaks, which means the build will fail independent of Coveralls. I use protobufs for another project and don't know what dead code will be in the files it generates, so can't trust on instrumenting it for coverage… But if it breaks, the build will fail, so I'm OK.

@nickmerwin There are a lot of people participating in this issue… should I open a new one, or take this to email, or?

Tried to figure out what other support options existed but don't feel like making a ZenDesk password or whatever.

[nickmerwin]

@jasonmp85 ok I figured it out -- the public oauth scope Coveralls has always asked for doesn't include the ability to write commit statuses; now it does.

Could you log out then log back in?

[jasonmp85]

That fixed it. The org needed that one extra permission, though it now also asked for essentially everything against my personal user. You guys should see whether that's a regression or something, but otherwise it's working great.

[peternewman]

@nickmerwin that fixed it for me too, it only asked for the commit status permission for me @jasonmp85 .

The change has raised one issue though @nickmerwin . Is there an API function within Coveralls to tell it to tell GitHub that it's getting ready to generate some coverage stats? FWIW we use https://github.com/eddyxu/cpp-coveralls . It sort of looks like the parallel build stuff may be relevant. Anyway, our current timeline is something like this:
Push to existing PR
Travis starts
PR reports Travis pending
Bulk of Travis finishes (we have coverage set to allowed failure as coverage is really just of interest to us, not crucial, it being a project with hardware, it can be harder to effectively test all coverage)
PR reports okay (Travis only)
Coverage finishes generating (it takes slightly longer than the standard make check)
Coverage calls into Coveralls
PR reports pending (Travis okay, Coveralls pending)
Coveralls calculates coverage
PR reports complete (Travis okay, Coveralls hopefully okay)

So in an ideal world, it would be great to have a Coveralls API, so at the start of the Travis coverage job, it calls into there, which then calls into GitHub to say Coveralls pending, which will only leave it slightly out of sync with Travis, and reduce the chance of builds having that confusing grey area where Travis is green, and Coveralls isn't listed as pending.

@jasonmp85 I take on board what your saying, and we're going to exclude the generated code from coverage, but playing devils advocate, if you don't have coverage of your generated code, how do you know if the tests are covering the bits you use and hence will fail if it breaks?

[nickmerwin]

@peternewman yes! cpp-coveralls will need to be updated to support the parallel option in the API post to Coveralls. That notifies us that we should wait until we receive a webhook to consider a build "done".

More info re: webhook here: https://coveralls.zendesk.com/hc/en-us/articles/203484329

[nickmerwin]

@jasonmp85 did it ask for private repo read/write access? I've seen a similar report and it's odd because we've only added the request for "commit:status" to our OAuth scopes. Looking for clues. Thanks!

[peternewman]

Thanks @nickmerwin so the parallel option is the right choice and will achieve what I'm asking for even if we aren't interested in an actual parallel build?

[nickmerwin]

@peternewman I'm assuming so yes, but would love to hear what you find testing out that flow. Keep me posted, thanks!

[jasonmp85]

It asked for everything. Not for my org (that just showed a dialog asking
for "modified access", i.e. just adding commit status), but for me. I was
kicked out and logged back in and it had a GitHub interstitial asking for
full access. Can't remember whether Coveralls had a public/private option
like many services or if it's all or nothing.

Honestly I avoided doing anything about it for a few hours before
eventually saying "screw it all my code's open source or org-owned anyhow"
and hit OK. But it never stopped asking for the full set of permissions.

—Jason

On Mon, Feb 9, 2015 at 11:13 AM, Nick Merwin [email protected]
wrote:

@jasonmp85 https://github.com/jasonmp85 did it ask for private repo
read/write access? I've seen a similar report and it's odd because we've
only added the request for "commit:status" to our OAuth scopes. Looking for
clues. Thanks!

—
Reply to this email directly or view it on GitHub
#25 (comment)
.