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

How does it compare to testthat::with_mock? #8

Closed
gaborcsardi opened this issue Nov 7, 2016 · 11 comments

Comments

@gaborcsardi
Copy link
Member

@gaborcsardi gaborcsardi commented Nov 7, 2016

I am sorry, I know it is a stupid question. But could you please summarize how this is different? Maybe even in the README?

It is just hard for me to choose between them....

@nsfinkelstein

This comment has been minimized.

Copy link
Contributor

@nsfinkelstein nsfinkelstein commented Nov 8, 2016

@gaborcsardi That's a totally fair question.

There are a few differences.

The first thing to note is that with_mock uses C code to modify the stubbed-out function in place.

This has a few consequences. It means that for the duration of with_mock, the stubbed function is stubbed everywhere, for everything. There are some worlds in which this could get you in trouble, e.g. if you're parallelizing your tests. It's also documented as being 'abusive of R's internals' and 'experimental' - that said, it's seemed pretty reliable whenever I've used it.

Mockery's stub, by contrast, has a lighter touch. For one, it's written purely in R and works by manipulating environments, rather than the underlying code. This means that you specify a function from which to stub. The stub is only active from that function, and only from within the test function in which you called stub. That's the main difference.

Because it limits itself to the env of the test function, it doesn't have to do any cleanup, which leads to a nicer syntax and, in my opinion, more readable tests. You don't have to make your assertions in a code-block passed to the with_mock function - you can do them anywhere in the test after you've called stub.

The fact that we're not messing with the C code also seems to offer a speedup of 4 - 5 times as compared to with_mock - I'll publish some of those benchmarks in the future.

I wrote a blog post that explains a little more about how stub works if you're curious.

I agree this would be a useful section in the README. I'll try to add something to this effect next weekend. Please let me know if you have any other questions.

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

@gaborcsardi gaborcsardi commented Nov 8, 2016

Thanks! I have a feeling that just manipulating the environment is fragile, tbh, and fails in some (admittedly somewhat unusual) situations. I haven't looked at your implementation, but here are some hard cases:

  • Calling a function with do.call("func", ...), lapply("func", ...), etc.
  • Modifying a function within the locked environment of a package.
  • If the modified function uses the injected name already for something else (as well).

I am not saying that these fail, but they are much harder to handle with your approach, I think.

Anyway, variety is good, experimentation is also good. :) And thanks for the answers!

@nsfinkelstein

This comment has been minimized.

Copy link
Contributor

@nsfinkelstein nsfinkelstein commented Nov 8, 2016

@gaborcsardi Thanks for the ideas!

do.call and lapply used in that way don't cause any problems - I've just added some test cases to demonstrate here: 0b7fbb7.

When you say modifying a function within the locked environment of a package, can you elaborate, or possibly even provide a sample use case you would expect to fail? I'm also not sure exactly what you mean by If the modified function uses the injected name already for something else.

If you have any other ideas or any actual failure cases, I'd be happy to hear about them. I don't think the way we're handling stubbing is fragile, but if there are any failure cases I'd really like to seek them out.

To be clear, when I say we're 'manipulating environments' - we're not actually changing any of the underlying environments. We're making copies of everything and making changes in those copies. The only existing env we're actually changing is the test env when we assign the modified copy of the function back to the env of the test function, so that it gets called instead of the original.

@nsfinkelstein

This comment has been minimized.

Copy link
Contributor

@nsfinkelstein nsfinkelstein commented Nov 9, 2016

@gaborcsardi

One other difference you may be interested in, as per this thread #9, is that stub can stub out primatives, whereas with_mock cannot (despite the name of the issue).

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

@gaborcsardi gaborcsardi commented Nov 9, 2016

When you say modifying a function within the locked environment of a package, can you elaborate, or possibly even provide a sample use case you would expect to fail

I want to mock httr:::request_perform, but I am not calling it directly in my code. httr::GET calls it, directly, but that is an implementation detail that ideally I would not rely on. Don't you then need to unlock httr::GET?

I'm also not sure exactly what you mean by If the modified function uses the injected name already for something else.

myfun <- function() {
 c <- function() "blah"
 c()
 rm(c)
 c(1,2,3)
}

and I want to mock c. Not the internal c, the external one. This does not come up often, though.

TBH I think that manipulating environments to do the mocking is inherently fragile. I don't want to discourage you, I am sorry if this comes off rude.

@nsfinkelstein

This comment has been minimized.

Copy link
Contributor

@nsfinkelstein nsfinkelstein commented Nov 9, 2016

@gaborcsardi Not rude at all! I appreciate you taking the time to point out where you think there may be problems.

I want to mock httr:::request_perform, but I am not calling it directly in my code. httr::GET calls it, directly, but that is an implementation detail that ideally I would not rely on. Don't you then need to unlock httr::GET?

In this case, you don't want to mock out httr::request_perform directly, because httr::GET might use something else under the hood in the future and that would break your tests. It sounds like you want to just mock out httr::GET. Have I understood that correctly?

Stub works fine with this, and the way it works doesn't require unlocking any envs.

Let's say you have a function called my_func that calls httr::GET, then in your test code you would just have a line:

test_that('something', {
    stub(my_func, 'httr::GET', 'my return value')
    my_func()
    # make some assertions
})

Now httr::GET always returns 'my return value' when called from your my_func.

In the second case, stub actually also has no problems at all.

stub puts a thing called c in the env in which myfun executes. myfun then inserts and removes another thing called c in front of that, so by the time the final call to c is made, the stub is still in effect.

Check it out in this interactive session:

> myfun <- function() {
 c <- function() "blah"
 c()
 rm(c)
 c(1,2,3)
}
> myfun()
[1] 1 2 3
> library(mockery)
> stub(myfun, 'c', 'my return value')
> myfun()
[1] "my return value"

So far I haven't seen a case where mockery's stub fails where with_mock succeeds, and I have seen a few cases where the inverse is true, so I'm inclined to disagree that our approach is fragile, though I'm happy to agree that some approaches to mocking using the environments may be fragile. I do appreciate you putting the time into thinking about this though!

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

@gaborcsardi gaborcsardi commented Nov 9, 2016

In this case, you don't want to mock out httr::request_perform directly, because httr::GET might use something else under the hood in the future and that would break your tests. It sounds like you want to just mock out httr::GET. Have I understood that correctly?

Well, you are right that this is internal knowledge. But still, my general problem is that I want to mock functions that I don't call directly. So what if I want to mock httr::request_perform?

@nsfinkelstein

This comment has been minimized.

Copy link
Contributor

@nsfinkelstein nsfinkelstein commented Nov 10, 2016

@gaborcsardi Ah, I see what you're saying. No, mockery cannot do that at the moment. That said, I don't tend to think mocking out code from within third-party libraries in your tests is good practice - mocking out your call to the third party library in the first place seems safe in that it protects you from changes to the implementation.

I'll leave this issue open as a reminder to myself to write up the differences in the readme.

Thanks!

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

@gaborcsardi gaborcsardi commented Nov 10, 2016

protects you from changes to the implementation.

Sure. Personally, I am not so concerned about this. This is testing code. I would not do any kind of mocking in "real" code. If the testing code fails because of an implementation change, c'est la vie, I'll fix the tests. No real harm is done.

Anyway, there is some chance that with_mock will be forbidden on CRAN, because it possibly breaks the JIT compiler, which will be turned on by default in the next version. So it is good to have some alternatives!

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

@gaborcsardi gaborcsardi commented Nov 15, 2016

Thanks again for the answers!

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

@gaborcsardi gaborcsardi commented Nov 15, 2016

Btw. I am already making good use of mockery, so thanks much!
https://github.com/gaborcsardi/prettycode/blob/master/tests/testthat/test-print.R

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.