Add String method to Mock to fix #423

[neilisaac]

Proposed fix for #423 and possibly #625 if it's the same issue.

See comment on Mock.String for explanation.

[glesica]

OK, this looks pretty harmless to me. I'd like to get some confirmation that it actually fixes the problems various people seem to be having before we merge it, though. I'll ping some of the people who have brought up this issue and get feedback here.

[bhcleek]

The test I created for #625, master...bhcleek:race-test, still fails.

In any case, I don't see how this PR could solve the problem caused by a mutex not being held since the mutex is not locked in the new String method.

[neilisaac]

This avoids the race in #423 by avoiding traversing the mock.Mock data structure. mock.Arguments.Diff uses fmt.Sprintf, which will use reflection to traverse the arguments, triggering the race detector. This PR simply avoids that traversal by providing a trivial String implementation.

This should fix a common special case of a more general issue in testify/mock. This special case comes up in some of my code where a mocked interface takes an interface type as an argument which is also mocked in the same test. The test case I added in this PR fails go test -race ./... in master.

@bhcleek's test case here master...bhcleek:race-test demonstrates the general case of this bug: doing recursive string formatting in mock.Arguments.Diff can cause races that the user can't easily avoid (since it still does the string formatting first regardless of what kind of argument matcher is being used.) This PR does not tackle the general case issue of recursive reads in the diff function.

[neilisaac]

I thought about this some more. I think a holistic fix is tricky if you want to be able to print the actual value passed when it doesn't match an expected call.

In general, there is a data race in tests that have concurrent access to a data structure passed as an argument to a mock that's traversed in its own String implementation. context.WithValue violates this as in @bhcleek's example.

Here's a simple test case for the general problem:

func TestConcurrentSyncArgument(t *testing.T) {
	m := &Mock{}
	m.On("TestConcurrentSyncArgument", Anything).Return()

	barrier := make(chan bool)

	s := struct{ x int }{}

	go func() {
		s.x++
		close(barrier)
	}()

	m.Called(&s)

	<-barrier

	m.AssertExpectations(t)
}

This test case will still fail with this PR, but I think it's worth merging since it fixes the more special case where passing mocks to mocks always breaks.

[neilisaac]

Is there any objection to merging this? As noted, it doesn't fix the general case, but does fix a special case that crops up when you pass a mock to a mock in tests with concurrency (as demonstrated by the test case in this PR.)

[xorcare]

It is very strange to implement the stringer method through a pointer and not through a value. most often this method is implemented by value.

[neilisaac]

@xorcare the Mock holds a mutex, so we can't pass by value or the race detector will detect the copy

[neilisaac]

Despite not fixing the general case, this PR fixes a real issue demonstrated in this unit test: https://github.com/stretchr/testify/pull/694/files#diff-d4376f2727e58f552d2da8aa2c4deb7fR1501.

This library does string formatting on arguments to Mock.On, and Mock contains a Mutex, which the race detector detects. It seems wrong that you can't use a mock as an argument to another mock, so can we merge this fix so I can remove workarounds in my tests?

[danielbh]

Any updates on this merge please? It would be very helpful for us. Thank you.

[7fold]

We are experiencing issues with this as well.