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

devtools::test() and R CMD check ok but covr throws error #420

Closed
GeoBosh opened this issue Mar 2, 2020 · 5 comments
Closed

devtools::test() and R CMD check ok but covr throws error #420

GeoBosh opened this issue Mar 2, 2020 · 5 comments

Comments

@GeoBosh
Copy link

GeoBosh commented Mar 2, 2020

My package lagged' passes devtools::test(), R CMD check, and builds successfully on TravisCI. However for some reason Coveralls was not getting coverage updates from Travis. It turned out that it was the 'covr' run after the build that was failing in the following line at the end of my .travis.yml

after_success:
  - Rscript -e 'covr::coveralls()'

More specifically, running the coverage tests locally I got:

library("covr")
package_coverage()
## ── 1. Failure: slMatrix (@test-pc20slMatrix.r#64)  ──────────────
## `nSeasons(msl) <- 5` threw an error with unexpected message.
## Expected match: "unable to find an inherited method for function 'nSeasons<-'"
## Actual message: "unable to find an inherited method for function ‘nSeasons<-’ for signature 
## ‘\"slMatrix\"’"

It is the quotes that cause the problem, but surprisingly for me, changing the quotes to dots in the expected regex didn't work:

── 1. Failure: slMatrix (@test-pc20slMatrix.r#64)  ────────
`nSeasons(msl) <- 5` threw an error with unexpected message.
Expected match: "unable to find an inherited method for function .nSeasons<-."
Actual message: "unable to find an inherited method for function ‘nSeasons<-’ for signature ‘\"slMatrix\"’"

It looks like there is a discrepancy between 'covr' and devtools here. I know that the above is not a 'minimal' reprex but it seems to convey the issue. The offending line above, now commented out is at the end of the file (see GeoBosh/lagged).

I resolved the issue by cutting off the regex just before the first quote but the expectation becomes less expressive this way.

@jimhester
Copy link
Member

The discrepancy comes because when R CMD check runs the tests is first sources the file tests-startup.R which contains options(useFancyQuotes = FALSE).

covr runs the tests using tools::testInstalledPackage(), which does not source this file (and therefore does not set this option).

Unfortunately there isn't an easy way to ensure this option is set in covr itself, however you could set it yourself in your tests/testthat.R file, which should resolve the discrepancy.

GeoBosh added a commit to GeoBosh/lagged that referenced this issue Mar 6, 2020
This resolves an issue concerning test expectations containing quotes -
'covr' was failing, when 'R CMD check' was fine, see
r-lib/covr#420.
@GeoBosh
Copy link
Author

GeoBosh commented Mar 6, 2020

Thanks, setting options(useFancyQuotes = FALSE) in tests/testthat.R is indeed a nice solution.

@r2evans
Copy link

r2evans commented Mar 31, 2021

@jimhester I know this is a closed issue, but this really is the canonical place (both the package and the issue) for this statement/request: please reconsider.

<sob-story>
I just spent the last four hours giting back and forth with a gitlab-ci instance trying to figure out why devtools::test(.) and covr::package_coverage(.) both succeeded on my laptop, and test(.) succeeded on CI but covr:: did not, and lo and behold it is this. I went so far as to change all the artifacts: notions in gitlab-ci and downloaded a complete image of the test environment, just to confirm that all files were identical (and no files were missing).

For some unclear reason, the default on windows is FALSE, whereas it is TRUE elsewhere. This (to me) is so "clearly" a failure (in quotes because, well, somebody had a great idea to justify it, but it just is not obvious to me at this moment).
</sob-story>

My frustration aside, I have a few proposals:

  1. add covr::package_coverage(..., useFancyQuotes=NA), where NA means change nothing; the point here is to provide a hint in the docs, even if you do not change the default behavior;
  2. after a deprecation period, either change option 1's default to FALSE; or
  3. go nuclear and set options(useFancyQuotes=FALSE).

Okay, perhaps number three is a bridge too far. Number one might be applicable to other covr::*_coverage functions as well, though I am not intimately familiar with the full package (yet) to best justify that point (my apologies).

Other options:

  1. add a mention of this in the github README.md;
  2. add a mention of this somewhere on covr.r-lib.org, perhaps with keywords like "quotes string failure windows linux useFancyQuotes" to improve SEO integration;
  3. help me (at least with moral support) convince R Core to change the default value somewhere, so that "R on platform A" behaves the same as "R on platform B".

Though I still think number one is the most-direct and painless without introducing breaking changes. It seems mostly to be harmless option-adding for the purposeful side-effect of documenting a weird situation.

Thank you!

(I was initially going to post the results from thank::you() directly, but I found the flashy gif to be a bit too demonstrative on this long post. Here it is anyway, a little more subdued, with my gratitude still behind it. https://media.giphy.com/media/fxI1G5PNC5esyNlIUs/giphy.gif)

jimhester added a commit that referenced this issue Mar 31, 2021
R CMD check sets this environment variable when running tests, but
tools::testInstalledPackage does not. Covr aims to be as consistent as
possible with R CMD check, so we should set it as well.

Fixes #420
@jimhester
Copy link
Member

Hi @r2evans, covr now sets the environment variable so this file will be sourced just like in R CMD check, this should make covr equivalent to running the tests in R CMD check in this respect.

Unfortunately we can only aim to be as compatible as possible with R CMD check, we cannot change R's defaults, so doing anything else is out of scope.

@r2evans
Copy link

r2evans commented Apr 8, 2021

Thanks @jimhester

lcolladotor added a commit to leekgroup/recount that referenced this issue Aug 10, 2021
Use the github version of covr (3.5.1.9003) to try to fix
https://github.com/leekgroup/recount/runs/3286906239?check_suite_focus=true#step:26:27 (covr 3.5.1)

r-lib/covr@ddbfbec
implements a fix to r-lib/covr#420 which maybe
explains this issue

Using the Bioconductor devel docker I cannot reproduce the error on the GHA
run using the same docker. That's when executing the test code manually, not
with covr (I'm still in the process of running R CMD check + covr there).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants