mock: avoid data races in Arguments.Diff

[petergardfjall]

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.

It should be noted that it is heavily inspired by #1229, but since that PR hasn't seen any updates in over a year I decided to start fresh.

Summary

Avoids testify mocks causing data races for (concurrently accessed) pointer arguments when running go test -race.

Changes

Arguments.Diff now uses %p to produce a "presentable string" for pointers rather than %v, since the latter traverses the pointed to data structure.

Motivation

We need to test our code for race conditions with go test -race, and we cannot have testify be the offender of data races.

Related issues

#1597

[petergardfjall]

Friendly reminder. 🙂

[petergardfjall]

Any chance of this being accepted?

[ndk]

It would be great to see the fix in master branch

[brackendawson]

This solution only works for reflect.Pointer kinds, but there are other kinds which are inherent pointers, such as maps.

Consider this test:

type mapArgMock struct{ Mock }

func (m *mapArgMock) Question(arg map[string]string) int {
	args := m.Called(arg)
	return args.Int(0)
}

func Test_CallMockWithConcurrentlyModifiedMapArg(t *testing.T) {
	m := &mapArgMock{}
	m.On("Question", Anything).Return(42)

	mapArg := map[string]string{"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()
		mapArg["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(mapArg)
	assert.Equal(t, 42, value)
	wg.Wait()

	m.AssertExpectations(t)
}

IsPtr needs to be replaced with a more comprehensive test for pointyness.

I also fear the wrath of the users passing complex struct pointers with small errors into mocks, they won't see the diff any more. But I probably care more about fixing the data race.

Is this the only way to fix the issue? Given that we consider equivelant instances of structs to be equal, could On be made to create a deep copy of the argument?

PS. Sorry for my tardiness on this review.

[brackendawson]

Thinking more on the above, does IsPtr need to recurse deep objects up to the first pointer shaped object to catch pointer fields in non-pointer structs? It's turtles all the way down.

[petergardfjall]

Thinking more on the above, does IsPtr need to recurse deep objects up to the first pointer shaped object to catch pointer fields in non-pointer structs? It's turtles all the way down.

Yes, I think you are right. The IsPtr only works for pointer arguments, but not for other "mutable" types such as Map, Slice, Array. So a more comprehensive check would be something like:

// isMutable indicates if the supplied value is of a mutable type.
func isMutable(v interface{}) bool {
	switch reflect.ValueOf(v).Kind() {
	case reflect.Array, reflect.Chan, reflect.Map, reflect.Ptr, reflect.Slice:
		return true
	}
	return false
}

And then it still won't catch the case where a struct value (not pointer) gets passed which contains a nested pointer field that gets updated.

Not sure how to solve this as recursing down v in search of a mutable/"pointerish" field traverses the value which is what we were trying to avoid to begin with (then we might as well just use %v). 🤷

[brackendawson]

It is possible to write a test to show if any leaf of the structure is "pointerish" without reading those leaves, but I like it about as much as you do.

I considered if we could make a deep copy of the object rather than relying on the pointers but this isn't possible. Even with reflection you cannot copy unexported struct fields, but unexported struct fields' values are used when comparing structs with reflect.DeepEqual (and with ==).