get call count in thread safe way

[nbaztec]

Summary

Allows returning number of calls to a specific method in a thread safe way.

Changes

Adds a new method on the Mock object that returns the call count.

Motivation

There was a need to get the call count information on a given mock. Accessing the Calls member leads to a race condition as the lock is neglected.

Example usage (if applicable)

type client interface {
	Do(x int)
}

type owner struct {
	c client
}

func (o owner) Start() chan struct{} {
	done := make(chan struct{})
	go func() {
		for {
			select {
			case <-done:
				return
			case <-time.After(2 * time.Millisecond):
				o.c.Do(1)
			}
		}
	}()

	return done
}

type clientMock struct {
	mock.Mock
}

func (m *clientMock) Do(x int) {
	m.Called(x)
}

func TestOwnerCloses(t *testing.T) {
	m := &clientMock{}
	m.On("Do", mock.AnythingOfType("int"))
	defer m.AssertExpectations(t)

	o := owner{c: m}
	done := o.Start()
	time.Sleep(5*time.Millisecond)	// wait for some time, the client gets called finite amount of times
	close(done)
	time.Sleep(2*time.Millisecond)	// allow closing

	expected := m.NumberOfCalls("Do")
	time.Sleep(10*time.Millisecond)	// test if the client was called during the time
	actual := m.NumberOfCalls("Do")

	assert.Equal(t, expected, actual)
}

Related issues

Closes #964

[boyan-soubachov]

Please move the mutex locking right before the start of the critical section (i.e. before this line)

[boyan-soubachov]

Would it not make sense to move this unlock to right after the critical section (i.e. between lines 590,591)? My thought is that it being explicitly there will help the critical section unnecessarily ballooning by accident.

[nbaztec]

I'd ideally go with defer Unlock since it's more "failsafe" in case the function body panics, however in this case, I don't think the critical section will panic at all. As such the result is identical the Unlock can be moved to end. Though, as long as we're also aware of the fact that any newly introduced code prone to panic will introduce a potential bug in future.

thoughts?

[boyan-soubachov]

That’s a very good point. Fine to leave it as is then 🙂

…

On 27 Jul 2020, at 22:32, Nisheeth Barthwal ***@***.***> wrote:  @nbaztec commented on this pull request. In mock/mock.go: > @@ -578,6 +578,19 @@ func (m *Mock) IsMethodCallable(t TestingT, methodName string, arguments ...inte return false } +// NumberOfCalls returns the number of times a method was called +func (m *Mock) NumberOfCalls(methodName string) int { + m.mutex.Lock() + defer m.mutex.Unlock() I'd ideally go with defer Unlock since it's more "failsafe" in case the function body panics, however in this case, I don't think the critical section will panic at all. As such the result is identical the Unlock can be moved to end. Though, as long as we're also aware of the fact that any newly introduced code prone to panic will introduce a potential bug in future. thoughts? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[mvdkleijn]

LGTM

[mvdkleijn]

@boyan-soubachov If it looks good to you too, feel free to merge

[boyan-soubachov]

Could you add a test which covers the issue you fixed. I would imagine it would fail when run with go test -race and without the fix you've added in this PR?

[nbaztec]

Sure, added the test to demonstrate data race.

Commenting out the mutex yields a data race with go test -v -race -run Test_Mock_NumberOfCallsIsThreadSafe ./...

[boyan-soubachov]

@boyan-soubachov If it looks good to you too, feel free to merge

The code looks good to me but I think we should have a test that demonstrates the issue (without the fix) and how it's fixed.

[boyan-soubachov]

Thank you for your time and effort. @mvdkleijn , thoughts?

[mvdkleijn]

lgtm

[nbaztec]

Is this safe to get merged?

[ubaltaci]

Can we merge this? It seems impl agreed and tests there, also NumberOfCalls method itself should be nice addition to API?

[brackendawson]

This is not actually NumberOfCalls, it's more like NumberOfCallsWithMethodName. This PR is useful, but do we have to add another method here for people to count calls with certain arguments, or number of calls regardless of method name?

Perhaps something like I mentioned in #1128 is more useful?