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

Automatic test coverage reports with coveralls. #937

Merged
merged 1 commit into from
Jan 19, 2015
Merged

Conversation

jimhester
Copy link
Contributor

You will have to enable this repo on coveralls at https://coveralls.io/repos/new before merging this pull request for the report to work the first time.

@yihui
Copy link
Owner

yihui commented Jan 18, 2015

Thanks! This is very interesting and useful. One question, though: since I do not use testthat (I use my own tiny package testit instead), does covr still work in that case? Or actually, does covr really have to "endorse" a specific testing framework? I have noticed covr a while ago, and I have been waiting for a better documentation (e.g. a package vignette) about how it actually works. Otherwise it is completely magic to me, and I'm not sure if it really works in my case. I tried to read the source code, but it does not seem to be something that I can understand in a few minutes :)

@yihui
Copy link
Owner

yihui commented Jan 18, 2015

BTW, while I'm reading the help pages, I have a humble suggestion that you export as fewer functions as possible, e.g. I do not quite see the value of exporting clear_counters, count, key, and so on. Two reasons: 1. avoid names clashing with other packages; 2. exported objects are kind of like promises to users that these are mature objects to use and won't change much in the future.

install:
- ./travis-tool.sh github_package jimhester/covr
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps surprising to you again, I do not use travis-tool.sh, either, since I think it is trivially easy to set it up by myself, and I'm more familiar and comfortable with apt-get :)

@jimhester
Copy link
Contributor Author

covr just sources the files in your tests, inst/tests directories, so it should work with any testing framework. I verified it does work with testit and the knitr tests before submitting this pull request.

re: exporting functions that is a fair criticism, I un-exported them at r-lib/covr@e77b2a5 , thank you for the suggestion and I agree with your reasoning.

I had previously seen that you did not use r-travis, but forgot about that fact when I was submitting this pull request, my apologies, I modified the yaml with r-lib/devtools#681 and clearly did not examine the file before submitting, my apologies.

@yihui
Copy link
Owner

yihui commented Jan 18, 2015

No no I was not criticizing you exporting functions. It was just a suggestion :)

Now it seems everything is good. Could you squash your three commits into one? e.g. git reset --soft HEAD~3 (assuming you are on the covr branch) then commit and force push (https://github.com/yihui/knitr/blob/master/CONTRIBUTING.md)

BTW, I'd still love to see a vignette describing how covr works under the hood. I understand you may not want to spend time on it, but I'm very interested in your magic 👍

@jimhester
Copy link
Contributor Author

Ok I squashed everything into one commit! Let me know if there is anything else you need.

I think your vignette suggestion is a good one, a couple other people have wanted to know how it works as well. I will put one together soon.

yihui added a commit that referenced this pull request Jan 19, 2015
Automatic test coverage reports with coveralls.
@yihui yihui merged commit 2e5146c into yihui:master Jan 19, 2015
@yihui
Copy link
Owner

yihui commented Jan 19, 2015

Excellent. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants