Prevent with_mock from touching base R packages
hadley opened this issue · 21 comments
As it breaks things with the byte code compiler (and it's just a bad idea in general)
Hm... Does trace() work with byte code?
@krlmlr I doubt it.
But nevertheless, if you want to trace to do the mocking, how do you force the mocked function to return? And if you manage to do that, how do you make sure that the mocked function falls back if it is called from the bytecode compiler or from a finalizer?
Technical difficulties aside, I agree with @hadley that it is a bad idea, and not just for base packages, but anything that your package under testing is not calling directly. And then we might as well use sg else instead of in-place mocking. Imo.
We could build or own version of trace() that works the way we need to. I've counted ~150 hits in CRAN packages, so we should at least provide an alternative. Original PR for reference: #160.
@sainathadapa @lbartnik @n-s-f: CC-ing you as the authors of stubthat and mockery.
@krlmlr It does not matter if we write our own trace(), because the fundamental problem cannot be solved: we cannot make the mocking temporary, and keep the original function for the byte compiler (or other "similar" R code).
Btw. 150 hits don't mean 150 packages, and this issue is about base packages only, anyway. So that could be only a handful of packages.
We simply have to break this because it's not going to work with the next version of R. I think it might be best to deprecate (or soft deprecate) with_mock and encourage people to use alternative packages. (I think mocking in R is complex and opinionated enough to be worth its own package)
I haven't tried yet, but it might be possible to just fix with_mock() so that it works with byte-compiled code, and at the same time soft-deprecate it and move it somewhere else.
As for as temporary fix, you can turn off the byte compiler in with_mock. @kalibera was not very happy with this as the "final" solution, but maybe it is OK as a temporary workaround, and with_mock can also give a warning. This would give people some time to update.
@gaborcsardi: What do you mean by "turning off the byte compiler in with_mock()"? I thought the failures come from testthat's inability to replace the functions that already have been compiled to bytecode in the base packages?
AFAIK the problem is that the byte compiler (which is implemented in R currently) calls the mocked function.
I can't see how to possibly fix with_mock(). I suspect that the byte compiler uses inlining, this means that there's no way to catch all occurrences:
testthat::with_mock(`base::inherits` = function(...) stop("oops"), as.person("Some Body"))
## Calls stop("oops"), but from the wrong location -- a helper function instantiated (but not byte-compiled) in this functionHow about:
Warn thatwith_mock()won't work in R >= 3.4.0 (or R >= 4.0?), but keep it functional for now (to simplify CRAN release)- ~~Forbid mocking anything but functions defined in the package under test~~~
@hadley: I've tried with_mock() with current R-devel, and I saw problems probably related to the inlining performed by the byte compiler. What else breaks in R-devel? I'm asking because I'm not sure it's specific to base packages.
So this is fine? #543 (comment)
I think mocking base packages is absolutely bad/impossible, and in general, I think mocking anything outside the current package is likely to fail, and I'm not even sure about mocking within the current package.
@kalibera can give more details.
I think we've sorted this out in #543 by now. I think mocking in the current package can be implemented reliably (with a shadow environment), but perhaps best outside testthat.
@n-s-f Thanks for the heads up, I'll need to revisit your implementation. I was thinking about something stupid like:
shadow_env <- new.env(parent = asNamespace(getPackage()))
for (func_name in namespace_funcs()) {
func <- get_func(func_name)
environment(func) <- shadow_env
shadow_env[["func_name"]] <- func
}
update_shadow_env(shadow_env, mocks)and then evaluating expressions in the shadow env (or even temporarily patching the search path). As far as I see, this would give you mocking with infinite depth right away, but I might me missing a point.
That would require temporarily patching the search path, just like devtools::load_all() does.
Avoiding cleanup is nice, although I'm pretty sure that parallel workers are sufficiently isolated from each other to not affect each other's search path. In the very basic scenario, where I mock one of my functions and don't need to hande callbacks from other packages, my approach doesn't need any cleanup either. So, swapping out namespaces can be added as necessary, if at all.
One thing to consider is that we're using mocking here for our tests as well. If we used a function from a package that imports testthat, we'd need to suggest that package and lazy-load it. The easiest thing might be a separate package that specializes on this kind of task; that package could then also import pkgload which handles namespace tasks.
I have implemented a drop-in replacement for with_mock(), most original tests pass.
- Uses a shadow environment that is temporarily inserted just before the evaluation environment in the environment chain.
- Infinite depth, mocks can still access and update local variables
- Cleanup is still necessary, because changing the parent environment is permanent.
- Callbacks from other packages are not handled yet.
- Need to document how the mocking works internally.
This reimplementation and stub() can well coexist, but I feel we need something that closer matches the original behavior.
Please be aware that you cannot freely insert new environments. This is a problem that several packages that manipulate environments have run into, mostly unit-testing frameworks. Simplified a bit, the byte-code compiler requires that you first have local environments (all environments up to topenv), then namespace and namespace-imports environments, then globalenv, and then global environments (non-namespace). See e.g. compiler:::frameTypes for more details. Testing with these settings should reveal problems with insertion of invalid environments: R_COMPILER_SUPPRESS_ALL=false R_JIT_STRATEGY=4. The cleanest way to mock I think would be through code generation: you would modify the call sites of functions.
@kalibera: Thanks. I'm inserting a local environment between topenv and the parent frame -- a replica of the namespace environment. It's created by new.env() and not by a function call, though. Would that still be a problem for the byte compiler?
Not sure from your description, but if you were inserting local environment inside other local environments, this particular check would be fine. "Fine" but you are doing low-level operations like replicating a namespace into something that looks like a namespace but isn't quite. And the other problems remain. I think mocking especially implemented this way is a bad thing. I could understand mocking via code modification (before the code is loaded) of the current package.
It does not matter how the environment is created.