Calling Called() in a goroutine can result in a call to t.FailNow()

[greg0ire]

Description

I'm using this library indirectly, through vektra/mockery. I have a mock that I use in a goroutine, and I don't expect any calls on it. However, because of a bug in my code, there sometimes is an unexpected call. This results in a call to t.FailNow(), which causes necessary cleanup to be skipped and the entire test suite to hang.

Looking at MethodCalled(), which is called by Called(), we can see several calls to m.fail():

testify/mock/mock.go

Lines 509 to 524 in 7c367bb

	m.fail("\n\nmock: Unexpected Method Call\n-----------------------------\n\n%s\n\nThe closest call I have is: \n\n%s\n\n%s\nDiff: %s\nat: %s\n",
	callString(methodName, arguments, true),
	callString(methodName, closestCall.Arguments, true),
	diffArguments(closestCall.Arguments, arguments),
	strings.TrimSpace(mismatch),
	assert.CallerInfo(),
	)
	} else {
	m.fail("\nassert: mock: I don't know what to return because the method call was unexpected.\n\tEither do Mock.On(\"%s\").Return(...) first, or remove the %s() call.\n\tThis method was unexpected:\n\t\t%s\n\tat: %s", methodName, methodName, callString(methodName, arguments, true), assert.CallerInfo())
	}
	}
	for _, requirement := range call.requires {
	if satisfied, _ := requirement.Parent.checkExpectation(requirement); !satisfied {
	m.mutex.Unlock()
	m.fail("mock: Unexpected Method Call\n-----------------------------\n\n%s\n\nMust not be called before%s:\n\n%s",

and fail() calls T.FailNow():

testify/mock/mock.go

Line 352 in 7c367bb

m.test.FailNow()

Looking at the documentation of FailNow(), we can read the following:

FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test.

I think both m.fail() and m.MethodCalled() should come with the same warning.

Step To Reproduce

create a goroutine that calls a mock, and performs an unexpected call on the mock, then closes a channel that should cause the main thread to hang until it is closed

// reproducer.go
package main

import "fmt"

type logger interface {
	Log(string)
}

func reproducer(log logger) {
	ch := make(chan int)
	go func() {
		ch <- 1
		log.Log("Hello")
		close(ch)
	}()

	for i := range ch {
		fmt.Println(i)
	}
}

// reproducer_test.go
package main

import (
	"testing"

	"github.com/stretchr/testify/mock"
)

type myMockedLogger struct {
	mock.Mock
}

func (m *myMockedLogger) Log(msg string) {
	m.Called(msg)
}

func TestReproducer(t *testing.T) {
	t.Run("Works fine", func(t *testing.T) {
		mock := &myMockedLogger{}
		mock.On("Log", "Hello").Return()
		mock.Test(t)
		reproducer(mock)
	})
	t.Run("Hangs forever", func(t *testing.T) {
		mock := &myMockedLogger{}
		mock.Test(t)
		reproducer(mock)
	})
}

Expected behavior

The channel should still get closed.

Actual behavior

The test hangs forever.

[dmcardle]

Looking at the documentation of FailNow(), we can read the following:

FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test.

I think both m.fail() and m.MethodCalled() should come with the same warning.

Good find!

I just stumbled onto this issue after discovering a similar problem with the require package's assertion functions, which also call testing.T.FailNow(). In my case, the fix was pretty simple — I just shuttle data back to the correct goroutine via a channel and check assertions there (second commit in rclone/rclone#8430).

I think that require also needs an explicit warning that these functions must only be called from the test-associated goroutine. I also found some relevant discussion from 2023 in issue #1499, but it's more focused on enforcing that calls come from the correct goroutines.

[brackendawson]

There isn't a fix to this issue in either package. With Mock.Called the issue is that the test has to be stopped because we don't know what to return. And there's no way for testify to get the failure back to the main test goroutine without blocking it.

There is a good workaround for concurrent code, which is to omit the call to Mock.Test. In this case Called will panic rather than calling t.FailNow, which will traverse back through your call stack and shouldn't deadlock. But you will get less user friendly errors.

The require package says it calls t.FailNow at the top of its docs, but this isn't clear enough. That should be expanded to explain the implications with respect to goroutines.

In the mock package I think the warning should go on the Mock.Test method, because the issue only exists if you use that.

Happy to accept a change to doc this.

[greg0ire]

@brackendawson I've created #1710 , please review it 🙏