Support func comparison in mocks

[Wani4ka]

The problem

According to the Golang specification, functions are not comparable. This makes sense in general, because there are things like closures or struct methods, which may have non-obvious comparison behaviors, but speaking about about unit testing with mocks, this may result in such an unexpected behavior:

// Assume we have an interface which accepts function
type FuncCaller interface {
    CallFunc(func())
}

// And we want to mock it
func TestFuncCaller(t *testing.T) {
    f := func() {}
    caller := NewFuncCallerMock(t)
    caller.CallFuncMock.Expect(f).Return()
    caller.CallFunc(f) // this fails
}

When checking arguments passed, minimock refers to reflect.DeepEqual if no special conditions (like minimock.AnyContext) are met. It would be logical for this function to at least try to compare functions by their pointers, but the behavior reflects the specification, resulting in people unavailable to change this because of backwards compatibility.

The changes

What I propose is to make available some basic function comparisons when setting up mocks, so that in the example above the function argument would be checked by its pointer. To achieve this, I implemented a checkFunctions function, which with several more conditions compares unsafe pointers of two functions, returning true if they refer to same address in memory. Then I used it in minimock.Equal the same way as checkAnyContext is used, to ensure two functions are not compared using reflect.DeepEqual. This results in us being able to check for function arguments in mocks generated by minimock.

The caveats

So far I've found two cases when function comparison may not be so obvious and thus dangerous.

1. Closures and anonymous functions

Assume we have a function that returns another function:

func generateHelloWorldPrinter() func() {
    return func() {
        fmt.Println("Hello world")
    }
}

When we use it as a mock expectation, it will fail, because calling generator two times results in two different functions being created, even though their body is same (basic CS actually):

func TestFuncCaller(t *testing.T) {
    caller := NewFuncCallerMock(t)
    caller.CallFuncMock.Expect(generateHelloWorldPrinter()).Return()
    caller.CallFunc(generateHelloWorldPrinter()) // this fails
}

2. Same method, same type, different object

Assume we have a type with a method:

type MyType int

func (t MyType) Method() {}

Now, when we set up a mock using two different MyType instances, we get same Method address, so this test would succeed:

func TestFuncCaller(t *testing.T) {
    caller := NewFuncCallerMock(t)
    defer caller.MinimockFinish()
    var (
        t1 MyType = 1
        t2 MyType = 2
    )
    caller.CallFuncMock.Expect(t1.Method).Times(2).Return()
    caller.CallFunc(t1.Method)
    caller.CallFunc(t2.Method)
}

I believe the reason behind this behavior is something like virtual method tables, due to which there's no need to allocate additional memory for all those methods each time a new instance is created.

The conclusion

As I mentioned above, this change may break backwards compatibility if contributed directly into Go, because it breaks the language specification. But, in terms of writing unit tests (non-production code) this seems as a good tradeoff to be able to check if a correct function is passed. The approach of checking functions by their reference is successfully used in Lua programming language, and has the same nuances: closures are a case as well as vtables (they are called metamethods and managed explicitly), meaning these cases are not go-specific and can be understood well.

I also believe there is no need in implementing backwards compatibility here, because the only acceptable case that did not result in error before is the following:

func TestFuncCaller(t *testing.T) {
    caller := NewFuncCallerMock(t)
    caller.CallFuncMock.Expect(nil).Return()
    caller.CallFunc(nil)
}

[hexdigest]

Here's my take on this:

1. The caveats introduce unexpected behavior and this is exact reason why functions are not comparable in go. This unexpected behavior defeats the whole purpose of this change.
2. I think the use case is very rare and instead of comparing pointer to functions. I'd rather assert for checked function behavior.

Here's what I mean: whenever tested component returns a function that we want to compare with "expected" function we should just call this function and check the result and/or artifacts that it creates.

@zcolleen wdyt?

[zcolleen]

@Wani4ka thanks for your pr.
@hexdigest as for me, i met such cases that are described here. E.g. when you test some function which calls mock with variadic functional options:

type externalIf interface {
	Call(opts ...func())
}

type SomeStruct struct {
	externalCaller externalIf
}

func (s *SomeStruct) testedFunc(a bool) {
	if a == true {
		s.externalCaller.Call(otherPkg.OnTrueFunc)
	} else {
		s.externalCaller.Call(otherPkg.OnFalseFunc)
	}
}

This is dummy case to demonstrate when such behaviour could be used.

I'd rather assert for checked function behavior

I think its not always possible to call the checked function, as in example above (because OnTrueFunc may be calling some external dependencies or set some internal variable so you wont check it) .

So im not against of merging this pr. The only consern i have is backward compatibility, but i dont see any issues here with it.