-stub flag

[kaedys]

Added a -stub flag that removes the panic behavior if an unset call is made. With this flag, unset methods return zero values instead. For the function signature:

Four(string, bool, int) (string, error)

The generated code without -stub (ie. with the current version of Moq) is:

// Four calls FourFunc.
func (mock *MyInterfaceMock) Four(in1 string, in2 bool, in3 int) (string, error) {
    if mock.FourFunc == nil {
        panic("moq: MyInterfaceMock.FourFunc is nil but MyInterface.Four was just called")
    }
    callInfo := struct {
        In1 string
        In2 bool
        In3 int
    }{
        In1: in1,
        In2: in2,
        In3: in3,
    }
    lockMyInterfaceMockFour.Lock()
    mock.calls.Four = append(mock.calls.Four, callInfo)
    lockMyInterfaceMockFour.Unlock()
    return mock.FourFunc(in1, in2, in3)
}

With the -stub flag in this PR, the generated code is:

// Four calls FourFunc.
func (mock *MyInterfaceMock) Four(in1 string, in2 bool, in3 int) (_r0 string, _r1 error) {
    if mock.FourFunc != nil {
        callInfo := struct {
            In1 string
            In2 bool
            In3 int
        }{
            In1: in1,
            In2: in2,
            In3: in3,
        }
        lockMyInterfaceMockFour.Lock()
        mock.calls.Four = append(mock.calls.Four, callInfo)
        lockMyInterfaceMockFour.Unlock()
        _r0, _r1 = mock.FourFunc(in1, in2, in3)
    }
    return
}

The -stub generate code will return the default zero values for all return types if the function field in the mock struct is unset, rather than panicking. This is defined as a command line flag (that is off by default) to avoid changing the generator's behavior for existing user. Fixes #36.

Note to reviewers: I made a duplicate of the template variable to avoid cluttering the existing template. The only lines that I changed in the duplicated template string are line numbers 157-178 in the diff. All other lines are identical to the existing template string.

[kaedys]

Hmm, tests failing because of changed function signature in New(), will correct and update PR.

[matryer]

Could we adjust the _r* variables to out1, out2, and out3 etc?

[kaedys]

Sure, that seems reasonable given the names of the input arguments. Tbh, I don't even remember why I used the _r# style. I'll try to remember to handle it on monday and get a new commit pushed.

[matryer]

thanks @kaedys

[kaedys]

Updated. New output:

// Four calls FourFunc.
func (mock *MyInterfaceMock) Four(in1 string, in2 bool, in3 int) (out1 string, out2 error) {
    if mock.FourFunc != nil {
        callInfo := struct {
            In1 string
            In2 bool
            In3 int
        }{
            In1: in1,
            In2: in2,
            In3: in3,
        }
        lockMyInterfaceMockFour.Lock()
        mock.calls.Four = append(mock.calls.Four, callInfo)
        lockMyInterfaceMockFour.Unlock()
        out1, out2 = mock.FourFunc(in1, in2, in3)
    }
    return
}

[hcliff]

to take this one step further why do we need the nil check? Most of my uses here are no-op, returning zero values is fine but I would like to track the calls

[kaedys]

The nil check is the entire point of this. Current behavior is that if the function field is nil when the function is called, the mock panics (which happened because it would panic anyway when you tried to call the nil function, and a specific and verbose panic is more obvious). The change in this PR is that, if you generate the code with the -stub flag, you are specifically asking for the behavior where calls to unset fields are functionally equivalent to those fields being set to trivial no-op returns, except without you having to manually set every field to said trivial no-ops.

If you want to track calls to a function, then set a function that tracks those calls. This PR is designed for people that don't care whether a particular method is called or not (unless they set something to the field that specifically checks if it was called), and just want to avoid all of the boilerplate setting trivial no-op functions to all of those fields just so their tests don't panic when calling functions that they don't care about.

A good example of this would be, for example, the io.Writer interface. If you don't check the integer argument returned by Write(), and you're testing some other part of the code that requires that Write call to return a nil error, then returning the default values (0 and nil, respectively) for those arguments is the desired behavior. Currently, that requires you to write something like this:

mockWriter.WriteFunc = func([]byte) (int, error) {
    return 0, nil
}

For this interface, that's pretty trivial, though it's still functionally useless code just to get around the existing panic behavior. But if the type you're mocking has, say, 30 methods, and the current test only need non-default return values for 1 of them, then you have to write up to 29 trivial boilerplate function literals just to avoid the panics.

This PR is designed to side step that behavior as an option. It is not designed around the case where you want to track function calls, any more than the existing case is designed to handle tracking function calls, you have to do that yourself if you want that behavior. This PR is purely about optionally avoiding excessive boilerplated function definitions when your tests don't require non-default return values from the functions.

As I've stated previously in this PR, this is most useful for methods that only return an error, because most of the tests of code utilizing the mocked interface will want the default nil return value for that, with only the test (or couple tests) specifically testing that method call itself and the behavior when it returns a non-nil error requiring a non-default return value. And this is an extremely common signature for methods, and therefore a highly common use case.

[hcliff]

good shout, perhaps if the non-panicing behavior is desired for “just tracking” we can/should support that with a different flag without feature creeping this PR

On Apr 13, 2018, at 23:23, Kaedys notifications@github.com wrote:

The nil check is the entire point of this. Current behavior is that if the function field is nil when the function is called, the mock panics (which happened because it would panic anyway when you tried to call the nil function, and a specific and verbose panic is more obvious). The change in this PR is that, if you generate the code with the -stub flag, you are specifically asking for the behavior where calls to unset fields are functionally equivalent to those fields being set to trivial no-op returns, except without you having to manually set every field to said trivial no-ops.

If you want to track calls to a function, then set a function that tracks those calls. This PR is designed for people that don't care whether a particular method is called or not (unless they set something to the field that specifically checks if it was called), and just want to avoid all of the boilerplate setting trivial no-op functions to all of those fields just so their tests don't panic when calling functions that they don't care about.

A good example of this would be, for example, the io.Writer interface. If you don't check the integer argument returned by Write(), and you're testing some other part of the code that requires that Write call to return a nil error, then returning the default values (0 and nil, respectively) for those arguments is the desired behavior. Currently, that requires you to write something like this:

mockWriter.WriteFunc = func(_ []byte) (int, error) { return 0, nil } For this interface, that's pretty trivial, though it's still functionally useless code just to get around the existing panic behavior. But if the type you're mocking has, say, 30 methods, and the current test only need non-default return values for 1 of them, then you have to write up to 29 trivial boilerplate function literals just to avoid the panics.

This PR is designed to side step that behavior as an option. It is not designed around the case where you want to track function calls, any more than the existing case is designed to handle tracking function calls, you have to do that yourself if you want that behavior. This PR is purely about optionally avoiding excessive boilerplated function definitions when your tests don't require non-default return values from the functions.

As I've stated previously in this PR, this is most useful for methods that only return an error, because most of the tests of code utilizing the mocked interface will want the default nil return value for that, with only the test (or couple tests) specifically testing that method call itself and the behavior when it returns a non-nil error requiring a non-default return value. And this is an extremely common signature for methods, and therefore a highly common use case.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[kaedys]

good shout, perhaps if the non-panicing behavior is desired for “just tracking” we can/should support that with a different flag without feature creeping this PR

Adding a feature that automatically tracks calls (and arguments) wouldn't be a bad feature, in my opinion, but it's certainly out of scope for this PR, and completely unrelated to the no-panic feature. Honestly, it could be very easily implemented as well, simply by putting an addition field per method on the mock structure:

type MockFoo struct {
    FooFunc func(a int, b string, c []float)
    FooCalls [][]interface{}
}

func (mock *MockFoo) Foo(in1 int, in2 string, in3 []float) {
    // ...
    mock.FooCalls = append(mock.FooCalls, []interface{}{in1, in2, in3})
    // ...
    mock.FooFunc(in1, in2, in3)
    // ...
}

You could then check len(mock.FooCalls) to see the number of calls that occurred, or check the individual arguments to each call by index.

That said, might be feature creep for the overall library. And again, definitely out of scope for this PR.

[matryer]

Closing due to old age.

[breml]

I like this PR.

@kaedys Do you think you could rebase your branch and open a new PR for this?

[kaedys]

Sure, I can look at doing that when I have some bandwidth. Currently under a bit of a crunch, though, so it might be a fewish days.

[eduardsmetanin]

Sure, I can look at doing that when I have some bandwidth. Currently under a bit of a crunch, though, so it might be a fewish days.

@kaedys It would be really cool if you could get this PR merged! Thanks!