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 test code coverage tracking #591
Conversation
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. |
Current coverage is 58.16% (diff: 100%)
|
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. |
I set up this in my own fork before (https://codecov.io/gh/thirdwing/Rcpp) and I have no strong opinion for this. |
[ 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
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. |
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. |
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 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. |
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 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.) |
@@ -22,6 +22,9 @@ install: | |||
script: | |||
- ./run.sh run_tests | |||
|
|||
after_success: | |||
- R -e 'install.packages("covr")' -e 'covr::codecov()' |
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.
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.
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.
Even simpler now as run.sh
has been updated:
after_success:
- ./run.sh coverage
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.
@jimhester: Can you make the small change after which I'd be happy to merge?
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.
Done (066d658), sorry for the delay!
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.
And no worries, I gathered you were away for a few days.
Looks like it does cost us about four minutes as the package needs rebuilding and retesting. Oh well. |
@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? |
I turned commenting off at https://github.com/RcppCore/Rcpp/blob/master/.codecov.yml, you just need to set |
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? |
@jimhester : In other words, am I missing something trivial in yaml here? Are files ignored when a newline is missing? Ie
is this file silently ignored? |
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.