fix: compare functional option names for indirect calls

[arjun-1]

Summary

Closes Functional Options testing broken for indirect calls #1380. The fix is consists of not comparing the the function names, as it is already asserted when comparing expectedValues and actualValues

Changes

• Adds test Test_Mock_AssertExpectationsFunctionalOptionsTypeIndirectly which would fail without the fix
• Adds test Test_Mock_AssertExpectationsFunctionalOptionsType_Diff to examine behavior of slightly different functional options in expected vs actual.
• Changes the funcName implementation to only compare the relevant last part, if applicable. This aids in reading the diff that is produced when the function names don't. match (for go1.20 or higher)

Motivation

The choice was made to omit the comparison of function names completely, instead of comparing only the relevant last part. The reason is that go1.19 and lower don't give enough any meaningful comparable part in the function name. Nevertheless the most relevant part of the function name is included in the diff result. Without it, the diff yielded in Test_Mock_AssertExpectationsFunctionalOptionsType_Diff would have resulted in (go1.20):

mock.Test_Mock_AssertExpectationsFunctionalOptionsType_Diff.OpNum &{num:0 str:1} != mock.Test_Mock_AssertExpectationsFunctionalOptionsType_Diff.OpNum &{num:1 str:}

The current implementation instead yields a diff:

OpStr &{num:0 str:1} != OpNum &{num:1 str:}

Though there could be added benefit in displaying the full location of the function, I can revert that part if desired.

Related issues

Closes #1380

[arjun-1]

Looks like the approach to compare a relevant part only fails for older Go versions, I'll rethink that
Done

[arjun-1]

Hey @dolmen I believe you were involved in reviewing related change #1381. Anything specific required for this review?

[pmokeev]

@dolmen can you have a look at this PR? I've also encountered the same problem

[brackendawson]

This PR would regress this highly contrived test:

package kata_test

import (
	"testing"

	"github.com/stretchr/testify/mock"
)

type MyStruct struct {
	A []byte
}

type Opt func(*MyStruct)

func WithString(s string) Opt {
	return func(m *MyStruct) {
		m.A = []byte(s)
	}
}

func WithBytes(b []byte) Opt {
	return func(m *MyStruct) {
		m.A = b
	}
}

type myMockFactory struct {
	mock.Mock
}

func (m *myMockFactory) New(opts ...Opt) *MyStruct {
	return m.Called(opts).Get(0).(*MyStruct)
}

func TestIfy(t *testing.T) {
	mockFactory := &myMockFactory{}
	mockFactory.Test(t)
	mockFactory.On("New", mock.FunctionalOptions(WithBytes([]byte("this")))).Return(&MyStruct{})
	mockFactory.New(WithString("this"))
}

The existing function name checking doesn't sit well with me either because it stripped the package name off. Can we use reflect.Value.Pointer to check that it's the same function? The docs suggest that multiple functions can share a code pointer but I don't know in what cases that happens. If it's not practical lets drop to testing only the behaviour as you propose.

On the diff output, we should show what we compare, which in the current state of this PR is just the behaviour, so I'd like the diff to look somewhat like:

Diff: 0: FAIL:  &MyStruct{A:1 B:0} != &MyStruct{A:0 B:1} 

[brackendawson]

Considering the diff some more, the function name could make a mistake more obvious to the user. But I'd want some clearer separation between the function name and the bahaviour because it's quite hard to grok, how about:

Diff: 0: FAIL:  WithB(*MyMock) -> &MyMock{A:0 B:1} != WithA(*MyMock) -> &MyMock{A:1 B:0}

[brackendawson]

The docstring for FunctionalOptions should also be changed to state that only the behaviour of the functional option argument is compared, and that functional options with identical outcomes compare as equal. Currently it says:

FunctionalOptions returns an [FunctionalOptionsArgument] object containing
the expected functional-options to check for.

For example:

  Assert(t, FunctionalOptions(foo.Opt1("strValue"), foo.Opt2(613)))

[arjun-1]

@brackendawson I agree with you that the regression you mention is not ideal. Originally I removed the function name comparison (only focussing on behavior) to make the tests (fixing the bug of this issue) pass for older Go versions older than 1.20. The function name retrieved via FuncForPC yields false unexpected calls for these versions. For example

github.com/stretchr/testify/mock.Test_Mock_AssertExpectationsFunctionalOptionsType_Indirectly.func1
github.com/stretchr/testify/mock.TheExampleMethodFunctionalOptionsIndirect.func1

I investigated the solution of using Pointer() directly on the reflected value. The pointer value is again falsely different, as I believe it points to the inner closed anonymous function (func1).

I explored a different comparison using FileLine. It's imperfect as two different function values on the same line would not be detected as equal. But it is the only solution that prevents the regression you mention (see Test_Mock_AssertExpectationsFunctionalOptionsType_Diff_Func) and fixes the original issue (Test_Mock_AssertExpectationsFunctionalOptionsType_Indirectly) for all Go versions.

The result is now a comparison based on function file line, and if that is the same a comparison based on function behavior. Let me know if the docstring for FunctionalOptions should still be updated.

[brackendawson]

Thanks for pulling on all those threads. I'm happy to take a function matching strategy that falsely matches different functions with the same behaviour defined on the same line because the original match also wasn't complete, it considered the same function defined in different packages to be equal.

• Your proposed change fixes the described bug, anonymous functions now match. They are actually quite a common code pattern to use.
• Different functions with the same behaviour still fail (unless they are defined on the same line).
• Different functions defined in different packages with the same name and the same behaviour now fail. This is faithful to our function's promise and is such a contrived example that I'm not concerned by the change to behaviour.

I'm confident that this is a safe enough way to resolve the issue. Thanks for all your work here. I'm also going to give other maintainers some time to review.