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.

Sign up for GitHub

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

Jump to bottom

Integrate Codecov to the CI build #5317 #5318

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Integrate Codecov to the CI build #5317 #5318

merged 1 commit into from
Feb 10, 2017

Conversation

wkurniawan07
Copy link
Member

Fixes #5317

Let's hope I get this configured correctly.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 60ea4d0 on 5317-coveralls into * on master*.

@wkurniawan07
Copy link
Member Author

Presenting our first coverage report! 🎉 🎉

@wkurniawan07 wkurniawan07 added the s.ToReview The PR is waiting for review(s) label May 12, 2016
@kanghj kanghj added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels May 12, 2016
@kanghj
Copy link
Member

kanghj commented May 16, 2016

Let's merge this soon?

@damithc damithc added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels May 23, 2016
@wkurniawan07
Copy link
Member Author

@damithc do we want to include the test classes in the coverage report, or just the main classes? Including just the main classes will drop the coverage down to just about 60% though, and that's already excluding the un-coverable classes like Servlets and Filters.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c612e49 on 5317-coveralls into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9bae4bb on 5317-coveralls into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 394cde3 on 5317-coveralls into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2590bed on 5317-coveralls into * on master*.

@wkurniawan07 wkurniawan07 added this to the V5.79 milestone Jun 2, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9d98c04 on 5317-coveralls into * on master*.

@wkurniawan07 wkurniawan07 modified the milestones: V5.80, V5.79 Jun 9, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4bf7555 on 5317-coveralls into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b2ab07c on 5317-coveralls into * on master*.

@wkurniawan07 wkurniawan07 modified the milestones: V5.81, V5.80 Jun 16, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2853567 on 5317-coveralls into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 46fd5f5 on 5317-coveralls into * on master*.

@wkurniawan07 wkurniawan07 force-pushed the 5317-coveralls branch 2 times, most recently from 3ead22a to a02dd1f Compare June 23, 2016 08:49
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a02dd1f on 5317-coveralls into * on master*.

@wkurniawan07 wkurniawan07 modified the milestones: V5.82, V5.81 Jun 24, 2016
@damithc
Copy link
Contributor

damithc commented Jan 13, 2017

se-edu uses coveralls. Is this problem specific to TEAMMATES?

@wkurniawan07
Copy link
Member Author

I suspect Java 7. Need to see how things develop on their side.

@wkurniawan07
Copy link
Member Author

Actually codecov looks like the better choice (reference, reference, reference, reference). They did lose out in popularity but that's hardly a minus point.

One thing for sure, their bot beats coveralls' hands-down.

@damithc
Copy link
Contributor

damithc commented Jan 13, 2017

We can try codecove. It's ok for different projects to use different tools.

@codecov-io
Copy link

codecov-io commented Jan 27, 2017 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@9b5d8fd). Click here to learn what that means.

@@            Coverage Diff            @@
##             master    #5318   +/-   ##
=========================================
  Coverage          ?   61.07%           
=========================================
  Files             ?      449           
  Lines             ?    25724           
  Branches          ?     4192           
=========================================
  Hits              ?    15711           
  Misses            ?     8998           
  Partials          ?     1015

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b5d8fd...f22b350. Read the comment docs.

@wkurniawan07 wkurniawan07 changed the title Integrate Coveralls to the CI build #5317 Integrate Codecov to the CI build #5317 Jan 27, 2017
@wkurniawan07
Copy link
Member Author

@damithc should I integrate Codecov now? Doing this means we're sort of enforcing code coverage which I'm not sure we're ready for.

@damithc
Copy link
Contributor

damithc commented Jan 30, 2017

Doing this means we're sort of enforcing code coverage which I'm not sure we're ready for.

I guess we can override the CI failure on coverage drop?

@wkurniawan07
Copy link
Member Author

Codecov doesn't inherently enforce any coverage (Coveralls also, I believe); the thing I am more concerned with is now all contributors must live with Codecov reporting their code status and if we do not make good use of them it will be quite a nuisance.
If we want to enforce coverage, then we have to make it a rule and communicate it to everyone.

@damithc
Copy link
Contributor

damithc commented Jan 30, 2017

Coveralls an be configured not to post coverage updates as comments. Instead, they show up as a CI check (similar to travis status), which is less intrusive (see below). May be Codecov has the same feature?
image

@wkurniawan07
Copy link
Member Author

I need to explore Codecov a bit more for that.

@wkurniawan07 wkurniawan07 force-pushed the 5317-coveralls branch 6 times, most recently from 82283b9 to ea4a1a9 Compare February 8, 2017 05:56
@wkurniawan07
Copy link
Member Author

@damithc we should be ready to try this.

@damithc
Copy link
Contributor

damithc commented Feb 8, 2017

@damithc we should be ready to try this.

Can go ahead.

@wkurniawan07 wkurniawan07 added this to the V5.96 milestone Feb 8, 2017
@wkurniawan07 wkurniawan07 merged commit 3a9a40f into master Feb 10, 2017
@wkurniawan07 wkurniawan07 deleted the 5317-coveralls branch February 10, 2017 05:33
@wkurniawan07
Copy link
Member Author

wkurniawan07 commented Feb 10, 2017 •
edited
Loading

Post-mortem:
image
I have configured three different status checks:

  • codecov/patch: coverage of all the lines introduced in the PR
  • codecov/project: default setting, i.e. production code coverage; this is also what is shown in the README.
  • codecov/project/all: coverage of ALL codes (production + test - client script)

Coveralls allows no such configuration, and this is only a fraction of what Codecov allows, so choosing Codecov is a correct decision.

@damithc
Copy link
Contributor

damithc commented Feb 10, 2017

Looks nice 👍

@wkurniawan07 wkurniawan07 mentioned this pull request Feb 10, 2017
Closed
karthikaacharya pushed a commit to karthikaacharya/teammates that referenced this pull request Feb 23, 2017
@wkurniawan07 @karthikaacharya
[TEAMMATES#5317] Integrate Codecov to the CI build (TEAMMATES#5318)
f46f308
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kanghj kanghj Awaiting requested review from kanghj

Assignees

@kanghj kanghj

Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V5.96
Development

Successfully merging this pull request may close these issues.
5 participants
@wkurniawan07 @coveralls @kanghj @damithc @codecov-io