Address testthat deprecation warnings + drop mockery #589
Conversation
|
Hi, thanks for working on the changes, but before you go further could you provide some motivation for the changes. Particularly if they are going to touch a lot of files and be largely cosmetic vs functional. |
There was a problem hiding this comment.
Here are some explanations of what I did.
I decided to run styler on test-codecov.R and test-coveralls.R, because the indentation was quite inconsistent and we have an automated way to do it. It produces a more difficult to read diff, but at least the code style is consistent.
| importFrom(httr,RETRY) | ||
| importFrom(httr,content) | ||
| importFrom(httr,upload_file) |
There was a problem hiding this comment.
Required to use testthat mocking https://testthat.r-lib.org/reference/local_mocked_bindings.html?q=local_mock#namespaced-calls
| if (!file.exists(file)) { | ||
| if (!source_file_exists(file)) { | ||
| return(NULL) | ||
| } | ||
|
|
||
| lines <- readLines(file) | ||
| lines <- read_lines_impl(file) | ||
| source_file <- rex::re_matches(lines[1], rex::rex("Source:", capture(name = "source", anything)))$source | ||
|
|
||
| # retrieve full path to the source files | ||
| source_file <- normalize_path(source_file) | ||
|
|
||
| # If the source file does not start with the package path or does not exist ignore it. | ||
| if (!file.exists(source_file) || !grepl(rex::rex(start, rex::regex(paste0(rex::escape(package_path), collapse = "|"))), source_file)) { | ||
| if (!source_file_exists(source_file) || !grepl(rex::rex(start, rex::regex(paste0(rex::escape(package_path), collapse = "|"))), source_file)) { | ||
| return(NULL) |
There was a problem hiding this comment.
These changes are required to remove the mockery dependency. I kept the commit self-contained.
But basically, with mockery::stub(), we can mock a function contained to a function. For example, mocking file.exists() only inside parse_gcov(). However, testthat::local_mocked_bindings() doesn't provide this ability.
The workaround I found is to create a wrapper function for file.exists() (in this case source_file_exists().) that I will only use inside parse_gcov(). This way, I can mock source_file_exists() to only mock file.exists() inside parse_gcov().
|
|
||
| withr::with_envvar(c(ci_vars, "CI_NAME" = "FAKECI"), | ||
| with_mock( | ||
| `httr:::RETRY` = function(...) list(...), | ||
| `httr::content` = identity, | ||
| `httr::upload_file` = function(file) readChar(file, file.info(file)$size), | ||
|
|
||
| res <- coveralls(coverage = cov), | ||
| json <- jsonlite::fromJSON(res$body$json_file), | ||
|
|
||
| expect_equal(nrow(json$source_files), 1), | ||
| expect_equal(json$service_name, "fakeci"), | ||
| expect_match(json$source_files$name, rex::rex("R", one_of("/", "\\"), "TestS4.R")), | ||
| expect_equal(json$source_files$source, read_file("TestS4/R/TestS4.R")), | ||
| expect_equal(json$source_files$source_digest, '1233f2eca5d84704101cb9d9b928f2e9'), | ||
| expect_equal(json$source_files$coverage[[1]], | ||
| c(NA, NA, NA, NA, NA, NA, 5, 2, NA, 3, NA, NA, NA, NA, NA, NA, NA, NA, NA, NA, | ||
| NA, NA, NA, NA, 1, NA, NA, NA, NA, NA, 1, NA, NA, NA, NA, NA, 1, NA)) | ||
| withr::with_envvar( | ||
| c(ci_vars, "CI_NAME" = "FAKECI"), | ||
| with_mocked_bindings( | ||
| RETRY = function(...) list(...), | ||
| content = identity, | ||
| upload_file = function(file) readChar(file, file.info(file)$size), | ||
| code = { | ||
| res <- coveralls(coverage = cov) | ||
| json <- jsonlite::fromJSON(res$body$json_file) | ||
|
|
||
| expect_equal(nrow(json$source_files), 1) | ||
| expect_equal(json$service_name, "fakeci") | ||
| expect_match(json$source_files$name, rex::rex("R", one_of("/", "\\"), "TestS4.R")) | ||
| expect_equal(json$source_files$source, read_file("TestS4/R/TestS4.R")) | ||
| expect_equal(json$source_files$source_digest, "1233f2eca5d84704101cb9d9b928f2e9") | ||
| expect_equal( | ||
| json$source_files$coverage[[1]], | ||
| c( | ||
| NA, NA, NA, NA, NA, NA, 5, 2, NA, 3, NA, NA, NA, NA, NA, NA, NA, NA, NA, NA, | ||
| NA, NA, NA, NA, 1, NA, NA, NA, NA, NA, 1, NA, NA, NA, NA, NA, 1, NA | ||
| ) | ||
| ) | ||
| } | ||
| ) | ||
| ) | ||
| }) |
There was a problem hiding this comment.
An example of replacing with_mock() with with_mocked_bindings().
In short, we cannot have namespaced functions. And we give it a series of functions to mock. And then provide which code to run with the mocking. Very similar to withr::with_*() syntax.
I didn't understand why internal covr functions were fully qualified, due to r-lib/testthat#734 (comment), but it no longer seems necessary.
Requires adding some helper functions to be able to mock certain base functions and functions used within specific functions only.
olivroy
changed the title
| with_mocked_bindings( | ||
| expect_equal(parse_gcov("hi.c.gcov"), numeric()), | ||
| read_lines_impl = function(x) { | ||
| " -: 0:Source:simple.c" | ||
| } | ||
| ) | ||
|
|
||
| mockery::stub(parse_gcov, "readLines", | ||
| " -: 0:Source:simple.c") | ||
| expect_equal(parse_gcov("hi.c.gcov"), numeric()) |
There was a problem hiding this comment.
Inside parse_gcov(), I replaced readLines() by read_lines_impl() to be able to mock readLines() only inside parse_gcov()
testthat::with_mock()withtestthat::with_mocked bindings(). (Addresses warnings logged in https://github.com/r-lib/covr/actions/runs/12316159627/job/34375845219#step:6:185)They turned more challenging to fix than I expected.
I therefore removed the mockery dependency and
testthat::with_mock()usage, but that required a bit of refactoring, such as importing from{httr}namespace to be able to mockhttr::content,httr::RETRY, etc.Kept everything related to mocking here to address testthat 3.2.2 deprecation warnings and sent other things to other PRs.