avoid data races in Arguments.Diff

Fixes a concurrency issue that would lead to testify mocks producing data races
detected by go test -race. These data races would occur whenever a mock pointer
argument was concurrently modified. The reason being that Arguments.Diff uses
the %v format specifier to get a presentable string for the argument. This also
traverses the pointed-to data structure, which would lead to the data race.

Signed-off-by: Peter Gardfjäll <[email protected]>

Author: petergardfjall

mock/mock.go

@@ -947,15 +947,31 @@ 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 {
 			expected = "(Missing)"
 			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
+}

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
+// TODO:pr 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 {