fix: fix IsMethodCallable

[gopherjit]

fixes method IsMethodCallable and respective unit test

Summary

fixes the method IsMethodCallable and respective unit test

Changes

calling findExpectedCall to check if the call is expected and repeatability is not equal to -1, if Yes return true, otherwise return true and false for the cases if the provided method was called and not called respectively.
fix respective unit test

Motivation

implementation of IsMethodCallable is not correct also the unit test is not written correctly

Related issues

Closes #1712

[gopherjit]

@brackendawson please review

[ccoVeille]

I don't know mocks package very well, but I'm surprised to see a signature change on a public API.

I mean t TestingT was the first argument was removed 🤔

Based on old code, I would say it was not used except for marking a method with t.Helper but it's not a reason to remove it I think

[brackendawson]

Yea, I mentioned in the issue that the TestingT is pointless here, but we cannot remove it because that is a breaking change.

[gopherjit]

Thanks @ccoVeille for looking at the code, I understand this is a breaking change for the API, but t testing.T was redundant for this function, we can discuss if we can accommodate a breaking change in the API with the benefit of little cleaner code, it is always a tradeoff.

[ccoVeille]

here is what I would have expected

Suggested change

	func (m *Mock) IsMethodCallable(t TestingT, methodName string, arguments ...interface{}) bool {
	if h, ok := t.(tHelper); ok {
	h.Helper()
	}
	func (m *Mock) IsMethodCallable(methodName string, arguments ...interface{}) bool {
	func (m *Mock) IsMethodCallable(_ TestingT, methodName string, arguments ...interface{}) bool {

[gopherjit]

@brackendawson @ccoVeille I have reintroduced the testing.T type argument.

[ccoVeille]

Here, I'm not able to tell if the new tests are OK.

I mean there were a lot of test that were removed, and I'm unsure why.

[gopherjit]

the unit test was mocking the function it was intended to test, see this line - https://github.com/stretchr/testify/pull/1718/files#diff-80fc02c2ad6c6b55c9dfd38ea5989af97040aeb73df5d9a5195332953276a96eL1721

[ccoVeille]

Then I feel like these two cases should be in separated t.Run

Here I feel like the code of the test is strange because it reuses result from previous one.

[gopherjit]

the tests are now in separate test runners

[ccoVeille]

LGTM 👍 in terms of code.

Let's wait for maintainers' feedbacks now

[brackendawson]

I don't think we need to call methodWasCalled because findExpectedCall will not return an expected call which has been fully satisfied.

I found this regression by writing the following testcase. I'd like to see something similar added in this PR.

package kata_test

import (
	"testing"

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

type MyMock struct {
	mock.Mock
}

func (m *MyMock) Do(i int) {
	m.Called(i)
}

func TestIfy(t *testing.T) {
	for name, expectedArg := range map[string]any{
		"1":              1,
		"Anything":       mock.Anything,
		"MatchedBy":      mock.MatchedBy(func(i int) bool { return i == 1 }),
		"AnythingOfType": mock.AnythingOfType("int"),
		"IsType":         mock.IsType(0),
	} {
		t.Run(name, func(t *testing.T) {
			expectedArg := expectedArg
			t.Parallel()

			m := &MyMock{}

			m.On("Do", expectedArg).Return().Times(2)
			assert.True(t, m.IsMethodCallable(t, "Do", 1))
			m.Do(1)
			assert.True(t, m.IsMethodCallable(t, "Do", 1))
			m.Do(1)
			assert.False(t, m.IsMethodCallable(t, "Do", 1))
		})
	}
}

After fixing this I think isArgsEqual() and methodWasCalled() are unused and should be removed.

[gopherjit]

I have removed usage of methodWasCalled in "IsMethodCallable"
I have removed isArgsEqual, didn't remove methodWasCalled as it is used at some other places.
The expected changes in unit tests are not clear to me.

[brackendawson]

Let me annotate the test with some comments, hopefully it becomes clear:

func TestIfy(t *testing.T) {
	// Test this with each kind of expected argumnet. It works with `1` (raw value)
	// in tstify v1.10.0 but not with any of the other kinds.
	for name, expectedArg := range map[string]any{
		"1":              1,
		"Anything":       mock.Anything,
		"MatchedBy":      mock.MatchedBy(func(i int) bool { return i == 1 }),
		"AnythingOfType": mock.AnythingOfType("int"),
		"IsType":         mock.IsType(0),
	} {
		t.Run(name, func(t *testing.T) {
			// Copy expectedArg to local scope before calling Parallel, solves the
			// capturing loop variable issue for go <v1.22.
			expectedArg := expectedArg
			t.Parallel()

			// Make a mock as usual
			m := &MyMock{}

			// Prime the mock as usual
			m.On("Do", expectedArg).Return().Times(2)
			
			// The "Do" call is now expected, it is expected twice.
			assert.True(t, m.IsMethodCallable(t, "Do", 1))
			
			// Call Do once
			m.Do(1)
			
			// The "Do" call is now expected once
			assert.True(t, m.IsMethodCallable(t, "Do", 1))
			
			// Call it again
			m.Do(1)
			
			// The "Do" call is now not expected, so it is not callable.
			assert.False(t, m.IsMethodCallable(t, "Do", 1))
		})
	}
}

[gopherjit]

Thanks @brackendawson, I have added the tests in similar fashion as suggested, please let me know if any other improvements can be made.

[gopherjit]

@brackendawson please review.