Add test code coverage tracking #591

Merged
merged 2 commits into from Nov 29, 2016

Projects

None yet

4 participants

@jimhester
Contributor

This adds test code coverage tracking and reports with covr and codecov.io.

The normal travis status is reported as soon as the build is successful, the coverage status is reported separately and only runs on successful builds. By default the coverage status fails if the PR is not covered by tests, which helps ensure new contributions are tested. This default can be changed if desired by modifying .codecov.yml.

The reports can be viewed by clicking on the README badge or directly at https://codecov.io/github/RcppCore/Rcpp and track results over time.

@eddelbuettel
Member

I am (cough, cough) moderately against and don't use it in any of my other packages. Fellow Rcpp team members may overvote me here.

This is a good example where consensus via discussion of an issue ticket is preferable over unsolicited PRs.

@codecov-io
codecov-io commented Nov 15, 2016 edited

Current coverage is 58.16% (diff: 100%)

No coverage report found for master at 8e90f89.

Powered by Codecov. Last update 8e90f89...066d658

@jimhester
Contributor

I am aware you don't use it, part of the reason for sending this was to have a discussion of why.

If you don't want to advertise your coverage I can understand that and am happy to remove the badge from the readme, you can still use the coverage reports internally to see where tests are missing and as a potential check on new PR if desired.

Conceptually a PR is just an issue with a branch/diff attached. Discussion can happen here just as easily as a regular issue. I don't really see the distinction other than more work on the PR contributors part.

@thirdwing
Member

I set up this in my own fork before (https://codecov.io/gh/thirdwing/Rcpp) and I have no strong opinion for this.

@eddelbuettel
Member

[ Minor aside: For discussions, we tend to prefer a) the mailing list or b) issue tickets, not PRs. That is why I added Contributing.md. ]

I am not religious about it but on balance am still against it because

  • impatience: I do want the Travis runs to finish ASAP to cover the main use case: r-release, on Linux.
  • madness of metrics: Goodhart's law applies somewhat: just by having it, we end up gaming it
  • tests are good, of course, and coverage measure are not useless either, but both give a (to me) somewhat tilted view

Again: if everybody here yells me to get with it, I will -- but so far I have not missed these. I am with KK on this: nice to have as an option in a branch to run on occasion. Just how every few months I update our raw count of tests in the README.md.

@eddelbuettel
Member
eddelbuettel commented Nov 15, 2016 edited

Conceptually a PR is just an issue with a branch/diff attached.

Sure, but I also (very informally, nothing serious) can see project progress / activity by counts of both, and co-mingling what should be an issue as a PR goes against that and meddles with the counts.

@jimhester
Contributor

The travis status updates before the coverage calculations are run, so if you are waiting on the from travis nothing will change.

I understand the concerns about gamification / coverage numbers, removing the badge may assuage some of that I hope.

You can always put the coverage calculations behind some kind of conditional if you don't want to run it on every build. Maybe an environment variable, commit message tag ala [ci skip], or have coverage run only for tagged releases.

My main impetus for this PR was because I was surprised there was no code exercising the exception logic from #579. I thought there might be other places in the codebase that might benefit from testing.

@eddelbuettel
Member

I do appreciate the PR. I have been toying with the idea of turning it on in some repos, but so far mostly convincing myself that I am going batty and then not done it.

Protecting it behind another if based on an env var is an option. But then that is even worse because then we get some measures which get stale. On balance I still stay away. So far.

As for lack of tests: Sure. We suck. Documentation is atrociously incomplete too. But we are just a bunch of unpaid hacks so the world gets what it pays for. (Half kidding. Maybe.)

.travis.yml
@@ -22,6 +22,9 @@ install:
script:
- ./run.sh run_tests
+after_success:
+ - R -e 'install.packages("covr")' -e 'covr::codecov()'
@eddelbuettel
eddelbuettel Nov 24, 2016 edited Member

Can you try this alternative which may be quicker as we use the .deb based Travis scheme:

after_success:
   # r-cran-covr is available in the Rutter PPAs we know about
   - ./run.sh install_aptget r-cran-covr        
   - R -e 'covr::codecov()'

That may cut down from the four minutes this currently adds.

@eddelbuettel
eddelbuettel Nov 26, 2016 Member

Even simpler now as run.sh has been updated:

after_success:
  - ./run.sh coverage
@eddelbuettel
eddelbuettel Nov 29, 2016 Member

@jimhester: Can you make the small change after which I'd be happy to merge?

@jimhester
jimhester Nov 29, 2016 Contributor

Done (066d658), sorry for the delay!

@eddelbuettel
eddelbuettel Nov 29, 2016 Member

And no worries, I gathered you were away for a few days.

@eddelbuettel eddelbuettel merged commit 90e9c7a into RcppCore:master Nov 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel
Member

Looks like it does cost us about four minutes as the package needs rebuilding and retesting. Oh well.

@eddelbuettel
Member

@jimhester Any idea why PR #601 gets no coverage info when this PR over at eddelbuettel/anytime had it? I though that was automatic now?

@jimhester
Contributor

I turned commenting off at https://github.com/RcppCore/Rcpp/blob/master/.codecov.yml, you just need to set comment: true if you want to turn it on.

@eddelbuettel
Member
eddelbuettel commented Dec 4, 2016 edited

I saw that file ... and copied it everywhere I turned coverage on, including here in anytime yet I still have comment in anytime. How come?

(I noticed one difference in final new-line, no final new-line). Is it picky?

@eddelbuettel
Member

@jimhester : In other words, am I missing something trivial in yaml here? Are files ignored when a newline is missing? Ie

edd@max:~/git/anytime(master)$ cat .codecov.yml 
comment: falseedd@max:~/git/anytime(master)$ 
edd@max:~/git/anytime(master)$ 

is this file silently ignored?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment