Skip to content
21 changes: 19 additions & 2 deletions mock/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,23 @@ import (
// regex for GCCGO functions
var gccgoRE = regexp.MustCompile(`\.pN\d+_`)

// formatArg returns a safe string representation of v for use in Diff output.
// Pointer-like types (pointer, map, slice, chan) are formatted with %p
// (address only) to avoid data races when other goroutines concurrently
// modify them.
func formatArg(v interface{}) string {
if v == nil {
return "<nil>"
}
rv := reflect.ValueOf(v)
kind := rv.Kind()
switch kind {
case reflect.Map, reflect.Ptr, reflect.Slice, reflect.Chan:
return fmt.Sprintf("(%[1]T=%[1]p)", v)
}
return fmt.Sprintf("(%[1]T=%[1]v)", v)
}

// TestingT is an interface wrapper around *testing.T
type TestingT interface {
Logf(format string, args ...interface{})
Expand Down Expand Up @@ -977,15 +994,15 @@ func (args Arguments) Diff(objects []interface{}) (string, int) {
actualFmt = "(Missing)"
} else {
actual = objects[i]
actualFmt = fmt.Sprintf("(%[1]T=%[1]v)", actual)
actualFmt = formatArg(actual)
}

if len(args) <= i {
expected = "(Missing)"
expectedFmt = "(Missing)"
} else {
expected = args[i]
expectedFmt = fmt.Sprintf("(%[1]T=%[1]v)", expected)
expectedFmt = formatArg(expected)
}

if matcher, ok := expected.(argumentMatcher); ok {
Expand Down
78 changes: 75 additions & 3 deletions mock/mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2271,7 +2271,7 @@ func TestArgumentMatcherToPrintMismatchWithReferenceType(t *testing.T) {
defer func() {
if r := recover(); r != nil {
matchingExp := regexp.MustCompile(
`\s+mock: Unexpected Method Call\s+-*\s+GetTimes\(\[\]int\)\s+0: \[\]int\{1\}\s+The closest call I have is:\s+GetTimes\(mock.argumentMatcher\)\s+0: mock.argumentMatcher\{.*?\}\s+Diff:.*\(\[\]int=\[1\]\) not matched by func\(\[\]int\) bool\nat: \[[^\]]+mock\/mock_test.go`)
`\s+mock: Unexpected Method Call\s+-*\s+GetTimes\(\[\]int\)\s+0: \[\]int\{1\}\s+The closest call I have is:\s+GetTimes\(mock.argumentMatcher\)\s+0: mock.argumentMatcher\{.*?\}\s+Diff:.*\(\[\]int=0x[0-9a-f]+\) not matched by func\(\[\]int\) bool\nat: \[[^\]]+mock\/mock_test.go`)
assert.Regexp(t, matchingExp, r)
}
}()
Expand Down Expand Up @@ -2306,7 +2306,7 @@ func TestClosestCallFavorsFirstMock(t *testing.T) {

defer func() {
if r := recover(); r != nil {
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -2,4 \+2,4 @@\s+\(bool\) true,\s+- \(bool\) true,\s+- \(bool\) true\s+\+ \(bool\) false,\s+\+ \(bool\) false\s+}\s+Diff: 0: FAIL: \(\[\]bool=\[(true\s?|false\s?){3}]\) != \(\[\]bool=\[(true\s?|false\s?){3}\]\)`
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -2,4 \+2,4 @@\s+\(bool\) true,\s+- \(bool\) true,\s+- \(bool\) true\s+\+ \(bool\) false,\s+\+ \(bool\) false\s+}\s+Diff: 0: FAIL: \(\[\]bool=0x[0-9a-f]+\) != \(\[\]bool=0x[0-9a-f]+\)`
matchingExp := regexp.MustCompile(unexpectedCallRegex(`TheExampleMethod7([]bool)`, `0: \[\]bool{true, false, false}`, `0: \[\]bool{true, true, true}`, diffRegExp))
assert.Regexp(t, matchingExp, r)
}
Expand All @@ -2324,7 +2324,7 @@ func TestClosestCallUsesRepeatabilityToFindClosest(t *testing.T) {

defer func() {
if r := recover(); r != nil {
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -1,4 \+1,4 @@\s+\(\[\]bool\) \(len=3\) {\s+- \(bool\) false,\s+- \(bool\) false,\s+\+ \(bool\) true,\s+\+ \(bool\) true,\s+\(bool\) false\s+Diff: 0: FAIL: \(\[\]bool=\[(true\s?|false\s?){3}]\) != \(\[\]bool=\[(true\s?|false\s?){3}\]\)`
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -1,4 \+1,4 @@\s+\(\[\]bool\) \(len=3\) {\s+- \(bool\) false,\s+- \(bool\) false,\s+\+ \(bool\) true,\s+\+ \(bool\) true,\s+\(bool\) false\s+Diff: 0: FAIL: \(\[\]bool=0x[0-9a-f]+\) != \(\[\]bool=0x[0-9a-f]+\)`
matchingExp := regexp.MustCompile(unexpectedCallRegex(`TheExampleMethod7([]bool)`, `0: \[\]bool{true, true, false}`, `0: \[\]bool{false, false, false}`, diffRegExp))
assert.Regexp(t, matchingExp, r)
}
Expand Down Expand Up @@ -2461,7 +2461,79 @@ func TestIssue1785ArgumentWithMutatingStringer(t *testing.T) {
m.MethodCalled("Method", &mutatingStringer{N: 2})
m.AssertExpectations(t)
}
func Test_formatArg(t *testing.T) {
t.Parallel()

// Pointer types should use %p (address only) to avoid deep traversal
ptr := 42
ptrFmt := formatArg(&ptr)
assert.Contains(t, ptrFmt, "*int=0x", "pointer should use %p format")

// Map types should use %p to avoid concurrent map iteration crashes
m := map[string]int{"a": 1}
mapFmt := formatArg(m)
assert.Contains(t, mapFmt, "map[string]int=0x", "map should use %p format")

// Slice types should use %p to avoid deep traversal
slice := []int{1, 2, 3}
sliceFmt := formatArg(slice)
assert.Contains(t, sliceFmt, "[]int=0x", "slice should use %p format")

// Channel types should use %p
ch := make(chan int)
chFmt := formatArg(ch)
assert.Contains(t, chFmt, "chan int=0x", "chan should use %p format")

// Basic types should use %v (readable value)
assert.Equal(t, "(int=42)", formatArg(42))
assert.Equal(t, "(string=hello)", formatArg("hello"))
assert.Equal(t, "<nil>", formatArg(nil))
}

func Test_Arguments_Diff_RaceSafeFormatting(t *testing.T) {
// Verify that formatArg with %p for maps does not deeply traverse
// the map bucket array (which is what caused the crash in #1866).
// We verify this by checking the output format: a map formatted with
// %p shows only the header pointer, not the key-value contents.
t.Parallel()

m := map[string]int{"key": 1}
out := formatArg(m)

// Must contain the type and a hex address (not key/value pairs)
assert.Contains(t, out, "map[string]int=0x", "map should print header address only")
assert.NotContains(t, out, "key", "map should not expand contents via %p")

// Also verify it doesn't panic with a live map
assert.NotPanics(t, func() { formatArg(m) })
}

func Test_Arguments_Diff_RaceSafeFormatting_Slice(t *testing.T) {
// Verify slice %p formatting does not deep-traverse slice elements.
t.Parallel()

s := []int{101, 202, 303}
out := formatArg(s)

assert.Contains(t, out, "[]int=0x", "slice should print header address only")
// Use values that don't appear in hex addresses (101=0x65, 202=0xCA, 303=0x12F)
assert.NotContains(t, out, "101", "slice should not expand contents via %p")
assert.NotContains(t, out, "202", "slice should not expand contents via %p")
assert.NotContains(t, out, "303", "slice should not expand contents via %p")
assert.NotPanics(t, func() { formatArg(s) })
}

func Test_Arguments_Diff_RaceSafeFormatting_Pointer(t *testing.T) {
// Verify pointer %p formatting does not dereference the pointer.
t.Parallel()

val := 999
out := formatArg(&val)

assert.Contains(t, out, "*int=0x", "pointer should print address only")
assert.NotContains(t, out, "999", "pointer should not dereference via %p")
assert.NotPanics(t, func() { formatArg(&val) })
}
Comment on lines +2493 to +2523
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a problem with the tests.

Maybe I'm simply missing something and then I might be wrong.

These tests here are calling formatArg, and that's it.

While it could be considered to test this for formatArg regression, these tests are about formatArg not mock.Diff.

It means test functions like Test_Arguments_Diff_RaceSafeFormatting_Pointer are wrong named. They should be about formatArg.

But then it means the issue you wanted to fix for mock.Diff is not tested, and so regression could happen.

Think about a PR that would remove the call to formatArg in mock.Diff.
The test would pass as only formatArg is tested.

Once again, I might have missed something and I could be wrong.

It would be important to add a test that would replicate the issue you are trying to fix

Take a look at what @petergardfjal wrote in his PR

https://github.com/stretchr/testify/pull/1598/changes#diff-80fc02c2ad6c6b55c9dfd38ea5989af97040aeb73df5d9a5195332953276a96eR1927

Comment thread
ccoVeille marked this conversation as resolved.
func TestIssue1227AssertExpectationsForObjectsWithMock(t *testing.T) {
Comment thread
ccoVeille marked this conversation as resolved.
mockT := &MockTestingT{}
AssertExpectationsForObjects(mockT, Mock{})
Expand Down