R6 methods, and functions called by R6 methods

[gaborcsardi]

Stubbing does not seem to work for them. I'll add a reproducible example later, my current one is rather big....

[gaborcsardi]

OK, I realize that this will never work, because you assign back the function to the calling environment, and the R6 method will be called from a different environment.

It is a pity, though.

[nsfinkelstein]

@gaborcsardi Thanks for filing this bug report!

I'd still be interested in looking into this.

Consider this: we assign the function that points to the mock function to the environment that the R6 method is called from, but we add some code to that function so that when it's done executing once (or a user-chosen number of times) it removes itself from the calling environment and reinstates the original function.

If you'd be willing to post a short example, I'll take a look at putting something together (though it might not be until early January).

[gaborcsardi]

I think this is error prone, because if an error happens (e.g. a test case fails), the mock function will not be called the requested number of times (or at all).

It is also different from what you have now, so it would be confusing.

[nsfinkelstein]

I agree on both counts. I wouldn't put this in unless I can find a way around the first issue, and I believe the confusion can be minimized in the code. The interface to the user shouldn't change at all, modulo perhaps an optional parameter to stub.

[nsfinkelstein]

@gaborcsardi

So there are actually a few reasonable-ish ways of dealing with this.

To mock out calls to R6 methods from other functions, we can mock out the $ primative in the calling function so that the call to the R6 method is mangled into something that can be easily mocked (and all other usages of $ remain unchanged). This is very similar to what we do with ::, and can be done reliably.

To mock out calls to functions from within R6 methods, we note the following. R6 objects are actuallly just environments, and R6 methods are just closures. I think it's reasonable to think that it's okay to mock things out of closures without having those mocks disappear at the end of the test. This is how most mocking libraries in other languages work - we can leave it up to the user to provide fresh instances of each object for each test, which is good practice anyway so state (like this) doesn't get messed up. Alternatively, we can mock out $ in the test function, in which case the mocks wouldn't carry over between tests, even if the same exact object is used.

I'll start working on this soon.

[nsfinkelstein]

Here are the test cases I'll be working from https://github.com/n-s-f/mockery/blob/380d81e1439969908a767e484939450b17b1b9ad/tests/testthat/test_stub.R#L197-L230

Let me know if your use-case is not covered by those.

[gaborcsardi]

Thanks! My use case is this I believe:
https://github.com/n-s-f/mockery/blob/380d81e1439969908a767e484939450b17b1b9ad/tests/testthat/test_stub.R#L207-L212

So it is covered.

[nsfinkelstein]

Great, so all tests are passing in this branch: https://github.com/n-s-f/mockery/tree/r6-stubbing-nsf

However I still want to clean up the code (I introduced some duplication) and add some more tests around the edge cases, so I'll probably merge this next weekend.

Incidentally, I found some very strange behavior in R such that unless this line, which shouldn't do anything, is there, the closure seems to break, in the sense that all the closures created by that function take the value of the last parameter passed to the closure generating function! I may look into this more before merging as well.

[gaborcsardi]

Cool. I'll test it later today. As for the mysterious line, it is because of the lazy evaluation of function arguments.

…

On 26 Nov 2016 13:58, "Noam Finkelstein" ***@***.***> wrote: Great, so all tests are passing in this branch: https://github.com/n-s-f/ mockery/tree/r6-stubbing-nsf However I still want to clean up the code (I introduced some duplication) and add some more tests around the edge cases, so I'll probably merge this next weekend. Incidentally, I found some very strange behavior in R such that unless this line <https://github.com/n-s-f/mockery/blob/r6-stubbing-nsf/R/stub.R#L105>, which shouldn't do anything, is there, the closure seems to break, in the sense that all the closure created by that function take the value of the last parameter passed to the closure generating function! I may look into this more before merging as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoTQADQGSLmgflHpvEYe6hZkZ2DyWh7ks5rCDsVgaJpZM4K6j6W> .

[nsfinkelstein]

@gaborcsardi Did you have any issues with this? I'm planning on merging later today.

Thanks.

[gaborcsardi]

Haven't tried yet, sorry. I'll try later today.

[nsfinkelstein]

No problem, thanks. I just found a minor bug I'm working through as well, so no rush.

[nsfinkelstein]

That bug is gone. If you have a chance to test it for your use case today, I'll merge the code after that. Otherwise I'll merge it at the end of the day.

Thanks again for letting me know about the missing feature!

[gaborcsardi]

So, how do you actually restore the original state?

[nsfinkelstein]

The path I went with is that I don't change the state. Instead I stub the $ operator out in the env of the test to mangle the name before evaluating the function call, and I insert a function with the mangled name (with the appropriate function mocked out) back into the test env.

[nsfinkelstein]

I'm going to merge the code above. If it solves your use case, please close the issue. Otherwise please elaborate further.

Thanks!

[frbl]

First of all, thanks for the great work with mockery! I'm sorry to reopen this issue, but I had some issues getting this example to work in my own code. I've created a MWE in #18.

The thing is that the mocking works fine whenever you're not calling other objects from within the code. If you do call other objects, for some reason it can't find them (hence the failing specs). Note the print statement in the spec in the PR. Its output is:

DONE ===========================================================================
Rerunning tests:  test_stub.R
stub: .................................<some_class>
  Public:
    clone: function (deep = FALSE)
    external_method: function ()
1...........

Failed -------------------------------------------------------------------------
1. Error: stub works with R6 methods that have other objects in them (@test_stub.R#226)
object 'other' not found
1: expect_equal(obj$method_with_other(), "stub has been called") at mockery/tests/testthat/test_stub.R:226
2: compare(object, expected, ...)
3: obj$method_with_other()
4: other$external_method at mockery/tests/testthat/test_stub.R:203
5: eval(parse(text = code), eval_env) at mockery/R/stub.R:110
6: eval(parse(text = code), eval_env)

As you can see it can actually print the object, but calling it right thereafter raises an error.

Any ideas?

[nsfinkelstein]

Thanks for opening the issue. I think this is solved by the code I developed for #17. If I recall, that code just needed a little bit of cleaning up. If you're up for looking through it, you can see if that solves your problem. Otherwise I'll take a closer look this weekend.

[nsfinkelstein]

This is a different problem than I thought. I will fix it in your pull request.

[nsfinkelstein]

@frbl This is fixed in #18. Please verify that this resolved the problem. If so, please close this issue.

Thanks!

[nsfinkelstein]

I'm sorry - I was wrong about this again. The issue is that everything that uses $ or :: gets evaluated in the functions calling environment, where object doesn't exist. This is tricky because it's hard to gain access to the execution environment. I'll revert that pull request and think about this more over the weekend. Let me know if you have any ideas.

[nsfinkelstein]

Happy to report this is fixed on master. The relevant commit is 563b24f.

[frbl]

Hi @n-s-f,

Thanks a lot for the super fast response. I will test it out as soon as possible, which is probably tomorrow, and I'll report back.

Thanks,
Frank

[nsfinkelstein]

Sounds good, thanks!

[nsfinkelstein]

@frbl Have you had a chance to see if this solves you problem? I think the tests I included are the same as the specs that failed in your example, but I'm curious if there are further issues.

[frbl]

Hi @n-s-f, sorry for the super late response. I've finally tried it and it seems to work! I did something wrong on my first try, but after fixing that, it seems that this is fixed. Thanks a lot for doing this! I'll implement it in more tests, and I'll let you know if I run into any problems.

[nsfinkelstein]

@frbl Thanks for testing.

Would you mind adding some failing specs that demonstrate the issue?

I've just pushed a commit (db24bc0) that adds back the exact specs that you added in originally.

I can run these with no failures on master, so there must be some additional issue not covered by these specs.

Thanks.

[frbl]

No there was something wrong in my code, and I fixed it. I removed my old comment, so never mind. It works :-) I'm implementing it now for some other specs, and I'll let you know when something breaks.

[nsfinkelstein]

@frbl Great! Thanks for opening the issue.