Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacement for expect_args() and expect_called() from mockery? #1889

Closed
fh-mthomson opened this issue Oct 27, 2023 · 5 comments
Closed

Replacement for expect_args() and expect_called() from mockery? #1889

fh-mthomson opened this issue Oct 27, 2023 · 5 comments

Comments

@fh-mthomson
Copy link

I'm experimenting migrating to testthat 3e. The transition path from with_mock() and local_mock() is clear (per deprecation messaging for testthat::with_mock() and docs for ?testthat::with_mocked_bindings_.

with_mock() and local_mock() are superseded in favour of with_mocked_bindings() and local_mocked_bindings().

However, I'm still relying on the following from the mockery package:

  • mockery::expect_called()
  • mockery::expect_args()
  • mockery::mock_args()
  • mockery::mock()

Are there testthat equivalents for these mockery functions?

PS forgive me if I'm oversimplifying!

@hadley
Copy link
Member

hadley commented Oct 30, 2023

Could you please explain what you're using them for?

@fh-mthomson
Copy link
Author

fh-mthomson commented Nov 14, 2023

At a high-level, testing how a lower-level helper function is called:

  • mockery::expect_called(): helper function is called (1A and 2A below)

if it is called, the expected arguments....

  • mockery::mock_args(): ... are passed through (2B below)
  • mockery::expect_args(): ... are passed through with the expected values (2C below)

To illustrate (made up examples):

library(testthat)

# function definitions ----
parent_function <- function(arg1, arg2) {
  if (arg1 == arg2) {
    arg3 <- ifelse(arg1 > 0, "pos", "neg")

    helper_function(
      arg1 = arg1,
      arg2 = arg2,
      arg3 = arg3
    )
  } else {
    "different"
  }
}

helper_function <- function(arg1, arg2, arg3 = NULL) {
  cli::cli_alert_info("{arg3}!")
  as.character(glue::glue("same: {arg1}=={arg2}"))
}

# behavior ----
parent_function(1, 2)
parent_function(1, 1)
parent_function(-1, -1)

# testing ----
text_same <- "same: 1==1"
text_diff <- "different"

## initial: mockery fns ----

### 1. helper fn NOT called ----
mock_helper <- mockery::mock(text_same)
with_mock(
  "helper_function" = mock_helper,
  result <- parent_function(1, 2)
)

expect_equal(result, text_diff)
mockery::expect_called(mock_helper, 0)

### 2. helper fn IS called ----
mock_helper <- mockery::mock(text_same)
with_mock(
  "helper_function" = mock_helper,
  result <- parent_function(1, 1)
)

expect_equal(result, text_same)

#### 2A: we only care if helper function is called ----
mockery::expect_called(mock_helper, n = 1)

#### 2B: we care about the names of the args passed through to the fn call ----
expect_true("arg3" %in% names(mockery::mock_args(mock_helper)[[1]]))

#### 2C: we care about the values of the args passed through to the helper ----
mockery::expect_args(
  mock_helper,
  n = 1,
  arg1 = 1,
  arg2 = 1,
  arg3 = "pos"
)

## (partially) revised to testthat mocks  ----

# clear: test expected *result* with mocked function (`local_mocked_bindings()` or `with_mocked_bindings()`). e.g., 
local_mocked_bindings(
  helper_function = mock_helper
)

expect_equal(parent_function(1, 2), text_diff)
expect_equal(parent_function(1, 1), text_same)

# unclear: how to test 2B (arg names) and 2C (arg values) above

@hadley
Copy link
Member

hadley commented Nov 14, 2023

I think I'm struggling to get a bigger here, as I don't generally see the need to test that a specific branch of a function is called. And when I do, I'll emit a signal and test for that:

library(testthat)
library(rlang, warn.conflicts = FALSE)

parent_function <- function(arg1, arg2) {
  if (arg1 == arg2) {
    signal(class = "arg_same")
    arg3 <- ifelse(arg1 > 0, "pos", "neg")

    helper_function(
      arg1 = arg1,
      arg2 = arg2,
      arg3 = arg3
    )
  } else {
    signal(class = "arg_different")
    "different"
  }
}

helper_function <- function(arg1, arg2, arg3 = NULL) {
  cli::cli_alert_info("{arg3}!")
  as.character(glue::glue("same: {arg1}=={arg2}"))
}

parent_function(1, 2) %>% 
  expect_equal("different") %>% 
  expect_condition(class = "arg_different")

parent_function(1, 1) %>% 
  expect_equal("same: 1==1") %>% 
  expect_condition(class = "arg_same")

Created on 2023-11-14 with reprex v2.0.2.9000

signal() is very fast, so unless it's in a really tight loop, I think it's fine to include in live code:

library(rlang)
bench::mark(signal(class = "foo"))
#> # A tibble: 1 × 6
#>   expression                     min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 "signal(class = \"foo\")"   38.9µs   41.5µs    23350.     294KB     35.1

Created on 2023-11-14 with reprex v2.0.2.9000

Similarly, I'm not entirely sure why you care what arguments are passed to the mock function, but if I did care, I'd just implement the specific test myself.

I guess what I find most confusing about this use of mocking is that you're not changing the behaviour of the mocked function at all, which is the hallmark of mocking for me. I think it would be useful to know more about why you are trying to test these behaviours, i.e. what is the problem that you are worried about?

@hadley
Copy link
Member

hadley commented Nov 29, 2023

@fh-mthomson does this help?

@fh-mthomson
Copy link
Author

fh-mthomson commented Jan 25, 2024

Thank you so much for the thoughts and suggestions! Sorry for the delayed reply -- this slipped down my priority list (its only called a handful of times in the codebase I've inherited).

The examples I provided are definitely oversimplifying, since the tests are very indirect, thus hard to distill down to a reprex that is "intuitive" for why we'd care about the branching/args in the first place.

The more I think about it, the more it feels like a code smell wherein we should refactor the tests to be more direct. I don't think they're solving the problem they're meant to test. Going to close, at least while I noodle on it further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants