golang / mock

GoMock is a mocking framework for the Go programming language.
Apache License 2.0
9.3k stars 611 forks source link

return value depend on input value #34

Closed rantibi closed 6 years ago

rantibi commented 8 years ago

Is there way do return value depend on the input? I want to do something like the following pseudo-code var response string mock.EXPECT().SomeMethod(gomock.Any()).AnyTimes().Do(func(input string) { response = input + input }).Return(response)

I got deep into the code, and it look like it unsupported, I there other way to do it?

dsymonds commented 8 years ago

Have you tried making that anonymous function have a return value, instead of using .Return?

rantibi commented 8 years ago

yes, it doesn't work. you can see here why

dsymonds commented 8 years ago

Ah, right.

Maybe you just want a hand-written fake instead? You're stretching the bounds of what gomock is really intended for.

rantibi commented 8 years ago

I can write my own interface implementation, but then II need to add empty implementation to all the method in the interface, witch it can be a lt of work, also I loss the call count and etc of the controller. I think that Mockito for Java have this option, and it very useful.

stevvooe commented 8 years ago

Maybe you just want a hand-written fake instead? You're stretching the bounds of what gomock is really intended for.

What exactly is the intended use case of gomock? Seems like it is more than reasonable to pick up the return value of EXPECT().Do. I can't imagine any other purpose for a mocking package.

mattklein123 commented 8 years ago

+1 on this. One other option would be to allow Return() to take a function, and if the function has the same parameters as the called function, the passed functor is called to generate the return value with the passed in parameters. This is basically what Invoke() does in C++ gmock. I'm new to Go and it would take me a bit to figure out how to use reflect to do this, but I think this is a major missing feature of the library.

bhcleek commented 8 years ago

This package already supports what's being asked for here. Use Do to set an action that examines the input values and then calls Return to set the return values that should be returned by the call:

ctrl := gomock.NewController()
v := NewFoo(ctrl)
quux := v.EXPECT().Quux(gomock.Any())
quux.Do(func(id int){
  if id > 0 {
    quux.Return(nil)
  } else {
    quux.Return(errors.New("id was <= 0"))
}
})

And, of course, Do can still be used to register additional actions that may be necessary for the test.

I would not want the return value of the action to used as the return value of the call.

stevvooe commented 8 years ago

I would not want the return value of the action to used as the return value of the call.

Why not? I am very confused by this line of reasoning.

bhcleek commented 8 years ago

Because the actions may have nothing to do with return values. For instance, the mock may have return values registered already, and the action registered with Do may log to help diagnose problems:

ctrl := gomock.NewController()
v := NewFoo(ctrl)
quux := v.EXPECT().Quux(gomock.Any())
quux.Do(func(i int){
  if i > 0 {
    t.Logf("unexpected non-zero value (%d)", i)
  }
})
call.Return(nil)

The action here gives useful information for debugging a test, and I wouldn't want its return value to be used by the mock.

stevvooe commented 8 years ago

@bhcleek How is that better than just returning nil?

bhcleek commented 8 years ago

The current implementation gives us more flexibility without sacrificing the ability of the action to specify what the return values of the call should be (it just needs to call Return to set the return value).

There are certainly cases where it would make sense to use the return values of the action as the return values of the call, but why force that restriction on all use cases?

stevvooe commented 8 years ago

@bhcleek I am confused about how having functions work as expected in the rest of language is all of a suddenly a restriction. This subtlety not only affects usability but also adoption. I just don't get why one needs to learn "this one trick" above to get anything useful done. What does that achieve?

Do you have examples of where this can do things that a regular function cannot?

bhcleek commented 8 years ago

There cannot be any examples of where this can do things that a regular function cannot, because the action is a regular function. The advantage of the current implementation is that it allows the action to decorate the Call value without have to know what the return value of the mocked method should be.

aliuygur commented 7 years ago

is this package support conditional return? if yes, can anyone write an example?

bhcleek commented 7 years ago

@alioygur it does. see https://github.com/golang/mock/issues/34#issuecomment-244477422

chris-r commented 6 years ago

I tried a variation of the solution above, and I got the zero-value returned. When I added .AnyTimes() and called it more than once, the first call set the return value of the second call, and so on. So the method above is apparently not valid in the version of gomock available to go get. If the above should work, is this a bug?

My test:

func TestTryIt(t *testing.T) {
    ctrl := gomock.NewController(t)
    defer ctrl.Finish()
    a := mock.NewMockAdder(ctrl)
    c := a.EXPECT().Add(gomock.Any()).AnyTimes()
    c.Do(func(v int) {
        c.Return(v + 1)
    })
    tryIt(a)
}

In main:

type Adder interface {
    Add(int) int
}

func tryIt(a Adder) {
    fmt.Println(a.Add(11), a.Add(10), a.Add(9)
}

My output:


=== RUN   TestTryIt
0 12 11
--- PASS: TestTryIt (0.00s)
PASS
ok      tm      0.044s
?       tm/mock_adder   [no test files]
sivakku commented 6 years ago

@bhcleek - looks like the example you mentioned doesn't work, with out the below fix? Was it ever working? any way to take this fix in?

https://github.com/golang/mock/pull/67

sivachandran commented 6 years ago

@bhcleek Changing return value inside Do function doesn't work as you mentioned. I see there are two PRs (by @bhcleek and @dmvk) in opened but wondering why they are not merged. I too need this functionality.

bhcleek commented 6 years ago

My earlier assertion was mistaken. I thought I had seen it work once, but it doesn't work now; I'm not sure if something changed or if I misremembered. There are two options that do work, though.

The first is that Do can be used to set the return of the next invocation by calling the Return.

The second thing that works is to setup the calls that you expect instead of trying to mutate what will be returned on the next invocation. Note that this is completely valid:

mymock.EXPECT().Foo().Return(1)
mymock.EXPECT().Foo().Return(2)
sivachandran commented 6 years ago

@bhcleek But both of the options that you mentioned doesn't satisfy the requirement of returning values based on the input. May I know what the problem you are seeing with mutating values within Do function or using the Do function return value?

sivakku commented 6 years ago

I think i like the other idea proposed in PR, let Do be what it does today and expose DoAndReturn() that returns values based on mutating input.

Minor issue which can be addressed is calling Return inside Do() has data race issue that it releases lock() an calls Return(), which is addressable.

Now if some one can merge this pull request: we can make some forward progress of where we want to get to. https://github.com/golang/mock/pull/115

josh-tepper commented 6 years ago

@sivakku @bhcleek @dsymonds

I'm a relatively new user who was also missing dynamic return values. In my experience with other frameworks, it's a great way to deal with more complex/hybrid objects that can't be handled with "pure" mocking. Further, sometimes specifying more exact return values overly constrains the way a function can run and makes tests brittle. Two thoughts:

  1. The idea of calling Call.Return(..) inside of a Do(..) function seems problematic to me (as enabled by PR https://github.com/golang/mock/pull/115). This has two problems, aside from being cumbersome: (a): The Call object really has a separate concern -- test configuration -- not the management of runtime state. (b): It is seemingly impossible to properly guard against concurrent modifications to the call object, as follows:
    • The Do(..) action is executed while NOT holding the Controller's mutex. Locking ANY mutex shared between mocked calls is problematic: E.g., sometimes a user will want a mocked call to block for a period of time to simulate a some real action (this doesn't imply non-determinism -- it could block until some other function is called). This is currently possible to achieve with a Do(..) function that blocks. Holding the mutex would break this: If the tested function is using a second mocked object which shares the same controller/call object, it won't be able to make calls during this period. At best, the test won't be able to simulate the behavior it was intending, and in the worst case, this would produce a deadlock.
    • This is true, even if there was a separate lock to guard the call object. Since Call.Return(..) can be called at any point during the Do(..) function, this lock would similarly have to be held for the entirety of the action until the return values were retrieved from the call object. As noted above any shared mutex (per controller, per call, etc) would undermine the ability to simulate blocking calls.
    • The only way that I can think of to make calling Return(..) safe during Do(..) would be to represent per-thread (or per-action) return values in the call object. Evaluating the return value would involve first checking the go-routine local value, then the initial set value. However, this is very counter to the go premiss of concurrency -- there should be a single per object state synchronized by locks or shared by channels.
  2. I also like the idea of introducing another function. Rather than calling it DoAndReturn, I would further differentiate the name, e.g., Invoke(..). Any call can have a Do(..) action -- however, calls would only be able to have one of Invoke(..) or Return(..). Setting both Invoke/Return should trigger a runtime panic. This has the advantage of separating the concerns. A Do(..) action similar to the one described by bhcleek in https://github.com/golang/mock/issues/34#issuecomment-244514905 can exist to check params in ways that independent matchers would find difficult/impossible. And the same call can also have one of either Invoke(..) or Return(..) to handle the other concern: the return values.

If others agree with this sentiment, I am happy to put this in a PR.

bhcleek commented 6 years ago

@josh-tepper I agree with all that you've said here, but I'm not a maintainer and have no more sway than any other user. Hopefully, though, the maintainers can weigh in with their opinions soon.

josh-tepper commented 6 years ago

Who would be the appropriate maintainer to \@ mention on this?

balshetzer commented 6 years ago

I am the project maintainer

josh-tepper commented 6 years ago

Nice to meet you Hesky! I actually think that we might have met when I was at Google NY. In any case, see above. There's been a discussion about ways to provide a delegate function to compute return values -- a feature that a lot of other frameworks provide. Let me know your thoughts. Was thinking of contributing a PR along the lines of my suggestions in https://github.com/golang/mock/issues/34#issuecomment-341994784

balshetzer commented 6 years ago

@josh-tepper

Hi Josh!

Take a look at #126.

This is my take on how to add dynamic return values (I'd love to just change Do but that will surely break someone). It doesn't have the extra checks that you mentioned. I wasn't sure how necessary they are. In my version actions just override each other. For example in call.DoAndReturn(f).Return(ret) the Return just overrides the DoAndReturn.

This makes sense to me but let me know what you think.

josh-tepper commented 6 years ago

Sure thing -- I have some commitments and won't be able to take a look until the weekend. Will let you know what I think.

josh-tepper commented 6 years ago

See my comments on https://github.com/golang/mock/pull/126. This implementation is IMO better than my suggestion