[R-package] [docs] fix calculation of R test coverage (fixes #4919) #4922
Conversation
| @@ -265,10 +265,13 @@ The example below shows how to generate code coverage for the R package on a mac | |||
|
|
|||
| ```shell | |||
| # Install | |||
| sh build-cran-package.sh | |||
| sh build-cran-package.sh \ | |||
| --no-build-vignettes | |||
There was a problem hiding this comment.
This change isn't necessary to make calculating test coverage work, but it does speed up the process a lot.
Since these instructions are only about computing the coverage from tests, it isn't necessary to build the vignettes.
Line 272 in af5b40e
I found that skipping vignette building saved on the order of 60-90 seconds in the process of computing coverage.
| @@ -2,7 +2,13 @@ VERBOSITY <- as.integer( | |||
| Sys.getenv("LIGHTGBM_TEST_VERBOSITY", "-1") | |||
| ) | |||
|
|
|||
| CALCULATING_TEST_COVERAGE <- Sys.getenv("LIGHTGBM_TEST_COVERAGE") == "true" | |||
There was a problem hiding this comment.
Can we use built-in R_COVR env variable instead?
https://www.rdocumentation.org/packages/covr/versions/3.5.1/topics/in_covr
UPD: ... or even better in_covr() function directly like
if (require(covr)) {
testthat::skip_if(covr::in_covr())
}There was a problem hiding this comment.
oh nice. I like covr::in_covr() a lot better than this solution with environment variables, didn't know about that! Thank you!
updated in 1e1e491
There was a problem hiding this comment.
Great! Happy to help with some simplification.
I see you omitted if (require(covr)) part. What will happen if user doesn't have covr package installed but decided to run all tests with testthat?
There was a problem hiding this comment.
I guess failing CI tests illustrate that there will be an error in that case:
Error in `loadNamespace(x)`: there is no package called 'covr'
* checking for unstated dependencies in ‘tests’ ... � WARNING
'::' or ':::' import not declared from: ‘covr’
* checking tests ...
Running ‘testthat.R’ [26s/14s]
� � � ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
6. base:::doWithOneRestart(return(expr), restart)
── 2. Error (test_lgb.unloader.R:31:5): lgb.unloader finds all boosters and remo
<packageNotFoundError/error/condition>
Error in `loadNamespace(x)`: there is no package called 'covr'
Backtrace:
1. testthat::skip_if(condition = covr::in_covr(), message = "lgb.unloader() tests are skipped when calculating test coverage") test_lgb.unloader.R:31:4
3. base::loadNamespace(x)
4. base::withRestarts(stop(cond), retry_loadNamespace = function() NULL)
5. base:::withOneRestart(expr, restarts[[1L]])
6. base:::doWithOneRestart(return(expr), restart)
══ DONE ════════════════════════════════════════════════════════════════════════
Error: Test failures
Execution halted
There was a problem hiding this comment.
haha yeah I tried to update that too fast, you're right.
There was a problem hiding this comment.
just pushed one more commit adding {covr} to the Suggests dependencies (23fd47c), since R CMD check failed with the following:
* checking for unstated dependencies in ‘tests’ ... WARNING
'::' or ':::' import not declared from: ‘covr’
'library' or 'require' call not declared from: ‘covr’
There was a problem hiding this comment.
gah I forgot that adding a new Suggests dependency would mean it has to be installed in CI
* checking package namespace information ... OK
* checking package dependencies ... ERROR
Package suggested but not available: ‘covr’
https://github.com/microsoft/LightGBM/runs/4697717553?check_suite_focus=true
I don't think this change to support computing code coverage during development is worth adding a new dependency to CI jobs (which will make them take longer and increase the risk of failures).
I just pushed b97d37a, which proposes using the R_COVR environment variable.
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Fixes #4919.
See the discussion in #4919, especially #4919 (comment), for an explanation of the changes in this PR.
Running the instructions in the R package README as of this branch, I found that the test results from
{covr}match my expectations and seem correct.lightgbm coverage - 55.65% (click me)