Address testthat deprecation warnings + drop mockery #589

olivroy · 2024-12-12T20:33:43Z

Replace mockery with testthat new mocking approach
Replace deprecated testthat::with_mock() with testthat::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 mock httr::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.

jimhester · 2024-12-12T21:07:48Z

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.

olivroy

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.

olivroy · 2024-12-12T21:09:35Z

NAMESPACE

+importFrom(httr,RETRY)
+importFrom(httr,content)
+importFrom(httr,upload_file)


Required to use testthat mocking https://testthat.r-lib.org/reference/local_mocked_bindings.html?q=local_mock#namespaced-calls

olivroy · 2024-12-12T21:12:58Z

R/compiled.R

-  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)


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().

So for base functions you can assign a NULL binding in the package rather than using a wrapper function. See https://testthat.r-lib.org/reference/local_mocked_bindings.html?q=local_mock#base-functions. This works because when the name is used as a function call the NULL assigned value is not a callable function, so it is not included in the lookup and the base version gets used instead. Then when the mocking happens it can assign a function to the variable in the package's namespace.

tests/testthat/test-S7.R

olivroy · 2024-12-12T21:18:39Z

tests/testthat/test-coveralls.R

-
-  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
+          )
+        )
+      }
    )
  )
 })


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 · 2024-12-13T13:34:56Z

tests/testthat/test-gcov.R

+  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())


Inside parse_gcov(), I replaced readLines() by read_lines_impl() to be able to mock readLines() only inside parse_gcov()

olivroy · 2025-01-09T13:16:34Z

@jimhester Let me know if you need more explanation for this PR

from testthat changelog explaining the removal of with_mock()

with_mock() and local_mock() have been unconditionally deprecated as they will no longer work in future versions of R (r-lib/testthat#1999).

More on transition to new functions from mockery https://www.tidyverse.org/blog/2023/10/testthat-3-2-0/#mocking

Oh from r-lib/asciicast#59, ~~I see that I could improve this without touching NAMESPACE (by adding package to with_mocked_bindings()~~.

Nope, from testthat docs

We don't recommend using this to mock functions in other packages, as you should not modify namespaces that you don't own.

Will leave as is

jimhester

I made a few comments on how the mocking was setup, but overall looks good.

Can you please also add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

R/covr.R

jimhester · 2025-01-13T16:57:59Z

R/compiled.R

-  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)


So for base functions you can assign a NULL binding in the package rather than using a wrapper function. See https://testthat.r-lib.org/reference/local_mocked_bindings.html?q=local_mock#base-functions. This works because when the name is used as a function call the NULL assigned value is not a callable function, so it is not included in the lookup and the base version gets used instead. Then when the mocking happens it can assign a function to the variable in the package's namespace.

R/compiled.R

olivroy commented Dec 12, 2024

View reviewed changes

olivroy marked this pull request as ready for review December 12, 2024 21:21

This was referenced Dec 13, 2024

Misc cleanups #590

Open

Use cli instead of crayon for console output #591

Open

olivroy added 4 commits December 13, 2024 08:14

Replace mockery with testthat mocking

53ea0ba

Requires adding some helper functions to be able to mock certain base functions and functions used within specific functions only.

Use importFrom to be able to mock httr functions and unnamespace calls

b1c40c2

Use with_mocked_bindings() in test-codecov and test-coveralls

38e1635

Replace with_mock() -> with_mocked_bindings()

8eadd93

olivroy force-pushed the crayon branch from f56374d to 8eadd93 Compare December 13, 2024 13:19

olivroy changed the title ~~Import cli instead of crayon to display messages + use testthat new mocking approach~~ Address testthat deprecation warnings + drop mockery Dec 13, 2024

olivroy commented Dec 13, 2024

View reviewed changes

jimhester requested changes Jan 13, 2025

View reviewed changes

olivroy and others added 3 commits January 13, 2025 15:21

Address comments

4f1366a

Update utils.R

f1729b0

Update NEWS.md

b5e55ae

olivroy requested a review from jimhester January 14, 2025 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address testthat deprecation warnings + drop mockery #589

Address testthat deprecation warnings + drop mockery #589

olivroy commented Dec 12, 2024 •

edited

Loading

jimhester commented Dec 12, 2024

olivroy left a comment •

edited

Loading

olivroy Dec 12, 2024

olivroy Dec 12, 2024

jimhester Jan 13, 2025

olivroy Dec 12, 2024 •

edited

Loading

olivroy Dec 13, 2024

olivroy commented Jan 9, 2025 •

edited

Loading

jimhester left a comment

jimhester Jan 13, 2025

Address testthat deprecation warnings + drop mockery #589

Are you sure you want to change the base?

Address testthat deprecation warnings + drop mockery #589

Conversation

olivroy commented Dec 12, 2024 • edited Loading

jimhester commented Dec 12, 2024

olivroy left a comment • edited Loading

Choose a reason for hiding this comment

olivroy Dec 12, 2024

Choose a reason for hiding this comment

olivroy Dec 12, 2024

Choose a reason for hiding this comment

jimhester Jan 13, 2025

Choose a reason for hiding this comment

olivroy Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

olivroy Dec 13, 2024

Choose a reason for hiding this comment

olivroy commented Jan 9, 2025 • edited Loading

jimhester left a comment

Choose a reason for hiding this comment

jimhester Jan 13, 2025

Choose a reason for hiding this comment

olivroy commented Dec 12, 2024 •

edited

Loading

olivroy left a comment •

edited

Loading

olivroy Dec 12, 2024 •

edited

Loading

olivroy commented Jan 9, 2025 •

edited

Loading