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

Mocking #159

Closed
krlmlr opened this issue Aug 22, 2014 · 11 comments · Fixed by #160
Closed

Mocking #159

krlmlr opened this issue Aug 22, 2014 · 11 comments · Fixed by #160

Comments

@krlmlr
Copy link
Member

krlmlr commented Aug 22, 2014

Useful to test functions that normally interact with the file system or the web, such as install_github in devtools. Also to speed up integration tests that involve heavy computation.

As suggested in https://github.com/hadley/devtools/pull/475/files#r16603091 . Proof of concept is available in devtools, a robust implementation will perhaps add a new environment to the search path.

Was suggested before in #93. @lbartnik: Are you still interested?

Was also discussed in r-lib/devtools#388. CC: @wch.

@wch
Copy link
Member

wch commented Aug 22, 2014

I like the idea. One possibility is to add a mocking environment as a child of the calling environment, and then create a child of that in which to evaluate the expression. This has the advantage of not modifying anything from the calling env, but it has the disadvantage of not evaluating the code in exactly the same environment that it would otherwise have run in.

For example:

with_mock <- function(mock_expr, expr, env = parent.frame()) {
  mock_env <- new.env(parent = env)
  # Turn an unevaluated expression into quoted expression, and eval
  eval(substitute(mock_expr), mock_env)

  eval_env <- new.env(parent = mock_env)
  eval(substitute(expr), eval_env)
}

f <- function() 1
f()
# 1

with_mock(f <- function() 2, f() * 10)
# 20

# Original f is unchanged
f()
# 1

Here's how it's not exactly the same as running expr in the calling environment:

pryr::parenvs()
#   label                      name
# 1 <environment: R_GlobalEnv> ""  

with_mock(NULL, pryr::parenvs())
#   label                      name
# 1 <environment: 0x4a044e0>   ""  
# 2 <environment: 0x4a04a70>   ""  
# 3 <environment: R_GlobalEnv> ""  

So things like <- will have different behavior with this version.

@hadley
Copy link
Member

hadley commented Aug 22, 2014

@wch will that work when mocking S3 methods? i.e. will the generic look in the right place?

@wch
Copy link
Member

wch commented Aug 22, 2014

Yes, if I correctly understand what you mean:

with_mock(print.list <- function(x) cat("hello\n"), print(list()) )
# hello

@wch
Copy link
Member

wch commented Aug 22, 2014

One more thought - it could be even simpler without creating two new environments. Just create one in which to do everything. Then it's basically just eval:

f <- function() 1
f()
# 1

eval(quote({
    f <- function() 2
    f() * 10
  }),
  envir = new.env()
)
# 20

f()
# 1

You could of course wrap this up in a function to make it a little easier to use.

@wch
Copy link
Member

wch commented Aug 22, 2014

Unfortunately, neither of these will work if you're calling the mocked function indirectly:

f <- function() 1
f()
# 1

g <- function() f()

with_mock(f <- function() 2, g() * 10)
# 10

I can't think of a better way than what you have already, @krlmlr, but that feels like it's risky in terms of unintended side-effects.

@krlmlr
Copy link
Member Author

krlmlr commented Aug 24, 2014

@wch: Which side-effects do you have in mind?

To be on the safe side, we could create a temporary copy of the package's environment (??? not sure about terminology here), patch it, edit the search path for the call and restore the search path afterwards. The package's environment is supposed to be read-only after all, even for the tests.

@gaborcsardi
Copy link
Member

Haven't looked at the details, but this package seems to support mocking with testthat: https://github.com/robertzk/testthatsomemore

EDIT: not really, actually, sorry, I should have looked at it first.....

@wch
Copy link
Member

wch commented Aug 26, 2014

@krlmlr One side effect is that the binding is left unlocked, but that should be fixable. I didn't have anything else specific in mind, so maybe it's OK. I could imagine that if you mock a stateful object (e.g., an environment or top-level list that gets modified by function calls) it might not restore properly, but that would be a pretty rare case.

@jimhester
Copy link
Member

I can get your examples to work (including side effects) if I clone the environment and then update each objects calling environment to the new environment. I don't know how expensive copying the environment is though, maybe very???

with_mock <- function(mock_expr, expr, env = parent.frame()) {
  mock_env <- list2env(as.list.environment(env))

  eval(substitute(mock_expr), mock_env)

  for(object in objects(env)){
    environment(mock_env[[object]]) <- mock_env
  }

  # Turn an unevaluated expression into quoted expression, and eval
  eval(substitute(expr), mock_env)
}

f <- function() 1
f()
# 1

g <- function() f()
g()
# 1

with_mock(f <- function() 2, g() * 10)
# 20

with_mock(print.list <- function(x) cat("hello\n"), expr=print(list()))
# hello

with_mock(list(f = function() 2), pryr::parenvs())
#   label                         name
# 1 <environment: 0x7fb63eaedb20> ""
# 2 <environment: 0x7fb63eaeeae8> ""
# 3 <environment: R_GlobalEnv>    ""

f()
# 1

@wch
Copy link
Member

wch commented Aug 27, 2014

Copying the environment shouldn't be expensive... I think it would be better to make the mock_env a sibling environment of calling environment (AKA parent.frame()) though. Also, the environment assignment should only be done to functions.

Still, there are some corner cases that this won't quite work correctly with. For example, suppose you have a top-level object that's an environment, list containing functions -- this won't be able to find the functions and modify their environments. Also, environments and reference class objects will be copied to the mock environment by reference instead of by value. So I think @krlmlr's suggestion is probably the least bad one so far. :)

@krlmlr
Copy link
Member Author

krlmlr commented Aug 27, 2014

Implementation in #160.

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

Successfully merging a pull request may close this issue.

5 participants