pharmaR / riskmetric

Metrics to evaluate the risk of R packages
https://pharmar.github.io/riskmetric/
Other
156 stars 29 forks source link

`skip_on_cran()` tests will reduce code coverage readout in `assess_covr_coverage()` #337

Open PatrickRWright opened 3 months ago

PatrickRWright commented 3 months ago

Some package test files will include testthat::skip_on_cran(). A use case I am aware of is to reduce runtime when deploying to CRAN and thus prevent package rejection. In the context of test coverage this means that tests are likely functional but omitted on CRAN only for technical reasons.

In the current setup, packages may return artificially low scores with respect to code coverage.

Here's a related SO thread for some additional context:
https://stackoverflow.com/questions/76153396/how-to-include-tests-with-skip-on-cran-when-calling-covrpackage-coverage

Example (22.84% vs. 95.18%): This needs some minutes to complete.

library(dplyr)
library(tibble)
library(riskmetric)

system("wget -q https://cran.r-project.org/src/contrib/secuTrialR_1.1.1.tar.gz -P source_archives")
system("tar -zxf source_archives/secuTrialR_1.1.1.tar.gz -C source_archives")

st <- pkg_ref("source_archives/secuTrialR", source = "pkg_source")
st_assess <- st %>% 
  tibble::as_tibble() %>% 
  pkg_assess(assessments = list(assess_covr_coverage))

Sys.setenv(NOT_CRAN = "true")

st_not_cran <- pkg_ref("source_archives/secuTrialR", source = "pkg_source")
st_not_cran_assess <- st_not_cran %>% 
  tibble::as_tibble() %>% 
  pkg_assess(assessments = list(assess_covr_coverage))

Sys.setenv(NOT_CRAN = "")

st_assess$covr_coverage[[1]]$totalcoverage
# > [1] 22.84
st_not_cran_assess$covr_coverage[[1]]$totalcoverage
# > [1] 95.18
dgkf commented 3 months ago

This sounds reasonable to me!

I do anticipate that there will be other packages where changing this envvar will cause their coverage to fail. I'm imagining specifically situations where packages use skip_on_cran() to disable tests that run outside their own CI/CD processes - maybe requiring authenticated database access using secrets only available on CI, or packages not available on CRAN.

There are more appropriate ways of handling these situations (skip_on_ci(), skip_if(), specifying AdditionalRepositories), but skip_on_cran() can often be the most obvious path forward in such cases so it sometimes gets used as a catch-all.

I would say we should try to run tests as inclusively as possible, but then fall back to cran-style tests if there are issues.