diff --git a/mock/mock.go b/mock/mock.go index d5eb1ef55..46c2453eb 100644 --- a/mock/mock.go +++ b/mock/mock.go @@ -947,7 +947,15 @@ func (args Arguments) Diff(objects []interface{}) (string, int) { actualFmt = "(Missing)" } else { actual = objects[i] - actualFmt = fmt.Sprintf("(%[1]T=%[1]v)", actual) + // Note: avoid %v format specifier for pointer arguments. The %v format + // specifier traverses the data structure, and for situations where the + // argument is a pointer (that may be updated concurrently) this can result + // in the mock code causing a data race when running go test -race. + if isPtr(actual) { + actualFmt = fmt.Sprintf("(%[1]T=%[1]p)", actual) + } else { + actualFmt = fmt.Sprintf("(%[1]T=%[1]v)", actual) + } } if len(args) <= i { @@ -955,7 +963,15 @@ func (args Arguments) Diff(objects []interface{}) (string, int) { expectedFmt = "(Missing)" } else { expected = args[i] - expectedFmt = fmt.Sprintf("(%[1]T=%[1]v)", expected) + // Note: avoid %v format specifier for pointer arguments. The %v format + // specifier traverses the data structure, and for situations where the + // argument is a pointer (that may be updated concurrently) this can result + // in the mock code causing a data race when running go test -race. + if isPtr(expected) { + expectedFmt = fmt.Sprintf("(%[1]T=%[1]p)", expected) + } else { + expectedFmt = fmt.Sprintf("(%[1]T=%[1]v)", expected) + } } if matcher, ok := expected.(argumentMatcher); ok { @@ -1250,3 +1266,8 @@ func funcName(opt interface{}) string { n := runtime.FuncForPC(reflect.ValueOf(opt).Pointer()).Name() return strings.TrimSuffix(path.Base(n), path.Ext(n)) } + +// isPtr indicates if the supplied value is a pointer. +func isPtr(v interface{}) bool { + return reflect.ValueOf(v).Kind() == reflect.Ptr +} diff --git a/mock/mock_test.go b/mock/mock_test.go index b80a8a75b..044a087d1 100644 --- a/mock/mock_test.go +++ b/mock/mock_test.go @@ -1911,6 +1911,42 @@ func Test_MockReturnAndCalledConcurrent(t *testing.T) { wg.Wait() } +type argType struct{ Question string } + +type pointerArgMock struct{ Mock } + +func (m *pointerArgMock) Question(arg *argType) int { + args := m.Called(arg) + return args.Int(0) +} + +// Exercises calling a mock with a pointer value that gets modified concurrently. Prior to fix +// https://github.com/stretchr/testify/pull/1598 this would fail when running go test with the -race +// flag, due to Arguments.Diff printing the format with specifier %v which traverses the pointed to +// data structure (that is being concurrently modified by another goroutine). +func Test_CallMockWithConcurrentlyModifiedPointerArg(t *testing.T) { + m := &pointerArgMock{} + m.On("Question", Anything).Return(42) + + ptrArg := &argType{Question: "What's the meaning of life?"} + + // Emulates a situation where the pointer value gets concurrently updated by another thread. + wg := sync.WaitGroup{} + wg.Add(1) + go func() { + defer wg.Done() + ptrArg.Question = "What is 7 * 6?" + }() + + // This is where we would get a data race since Arguments.Diff would traverse the pointed to + // struct while being updated. Something go test -race would identify as a data race. + value := m.Question(ptrArg) + assert.Equal(t, 42, value) + wg.Wait() + + m.AssertExpectations(t) +} + type timer struct{ Mock } func (s *timer) GetTime(i int) string {