r-lib / covr

Test coverage reports for R
https://covr.r-lib.org
Other
336 stars 117 forks source link

Missed coverage of function in a list. #556

Open dmurdoch opened 11 months ago

dmurdoch commented 11 months ago

In my recent PR https://github.com/igraph/rigraph/pull/1021 to igraph, the coverage tests said that the testing didn't include the changed code, even though it did.

I put together a simple package with the same structure of running functions that have been assigned into a list, and covr::package_coverage(type="examples") also claimed there was no coverage.

The example package had this code:

test1 <- function() {
x <- 1 +
     2 +
     3 +
     4
}

test <- list(test1)

and it exported test. The example for the help page for test ran this:

test[[1]]()

A version of the package that simply assigned test <- test1 and ran test() showed 100% coverage.

The full package is available here: https://github.com/dmurdoch/testpkg/tree/test_coverage .

Here's my session info:

R version 4.3.1 (2023-06-16)
Platform: x86_64-apple-darwin20 (64-bit)
Running under: macOS Monterey 12.7.2

Matrix products: default
BLAS:   /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib 
LAPACK: /Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/lib/libRlapack.dylib;  LAPACK version 3.11.0

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

time zone: America/Toronto
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets 
[6] methods   base     

other attached packages:
[1] covr_3.6.4    testpkg_0.1.1

loaded via a namespace (and not attached):
[1] compiler_4.3.1 lazyeval_0.2.2 rex_1.2.1     
[4] tools_4.3.1 
dmurdoch commented 11 months ago

I can see two kinds of solutions to this.

  1. Put the onus on the package writer: Allow a hook that (re)populates any lists or other containers after covr has installed its tracing code, and again when the code is removed.
  2. Modify covr to recursively handle lists.

2 is relatively straightforward, by adding a new type of record into the$replacements. The new type handles lists, not closures. A simple structure for it saves the environment where the list was found and the name of the list, and then has a list of replacements to make within that list, indexed by number rather than name since lists aren't always named.

A disadvantage of this is that lists aren't the only places where functions can be hidden -- packages could have environment objects, functions could be assigned as attributes of other objects, etc. Handling attributes wouldn't be hard if lists were handled, but handling environments recursively would have to watch out for infinite loops. Still, it's probably doable.

A disadvantage of 1 is that packages might need to be reorganized to do it, and it would be hard for the package writers to maintain.