Mocks do not retain knowledge of panicked calls

[Groxx]

In a nutshell: this test passes, despite the second call panicking.

package main

import (
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	"go.uber.org/cadence/activity"
	"go.uber.org/cadence/testsuite"
	"go.uber.org/cadence/workflow"
)

func init() {
	workflow.Register(work)
	activity.Register(act)
}

func work(ctx workflow.Context) (err error) {
	ctx = workflow.WithActivityOptions(ctx, workflow.ActivityOptions{
		ScheduleToStartTimeout: time.Minute,
		StartToCloseTimeout:    time.Minute,
	})
	// note two calls
	workflow.ExecuteActivity(ctx, act).Get(ctx, nil)
	workflow.ExecuteActivity(ctx, act).Get(ctx, nil)
	return nil
}

func act() error {
	return nil
}

func TestMocks(t *testing.T) {
	s := new(testsuite.WorkflowTestSuite)
	env := s.NewTestWorkflowEnvironment()

	// note a single mocked call.  the second call will panic.
	env.OnActivity(act).Return(nil).Once()
	env.ExecuteWorkflow(work)

	assert.True(t, env.IsWorkflowCompleted())
	env.AssertExpectations(t)
}

Obviously I'm suppressing the error that's returned from the second .Get, but that should be irrelevant. In fact, the test passes whether I do that or not.

Everything except env.AssertExpectations(t) passing is fine. In this case the mock expectations do not match the calls, but because it panicked it didn't record the fact that it was called, so insufficiently-mocked behavior does not fail tests like it should.

---

So far as I can tell, the issue is this code in testify.mock:

func (m *Mock) MethodCalled(methodName string, arguments ...interface{}) Arguments {
	m.mutex.Lock()
	found, call := m.findExpectedCall(methodName, arguments...)

	if found < 0 {
		// we have to fail here - because we don't know what to do
		// as the return arguments.  This is because:
		//
		//   a) this is a totally unexpected call to this method,
		//   b) the arguments are not what was expected, or
		//   c) the developer has forgotten to add an accompanying On...Return pair.

		closestFound, closestCall := m.findClosestCall(methodName, arguments...)
		m.mutex.Unlock()

		if closestFound {
			panic(fmt.Sprintf("\n\nmock: Unexpected Method Call\n-----------------------------\n\n%s\n\nThe closest call I have is: \n\n%s\n\n%s\n", callString(methodName, arguments, true), callString(methodName, closestCall.Arguments, true), diffArguments(closestCall.Arguments, arguments)))
		} else {
			panic(fmt.Sprintf("\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()))
		}
	}

	if call.Repeatability == 1 {
		call.Repeatability = -1
	} else if call.Repeatability > 1 {
		call.Repeatability--
	}
	call.totalCalls++
	// ...
}

Since the panic occurs before the call count is incremented, it "forgets" that it failed.

So... granted this is basically a bug in testify (and I intend to bring it up with them), but it's much more relevant to Cadence code than most code since panics are suppressed and wrapped in even simple tests. Is there another mock framework that could be tried instead, or something that could be done to work around this?

[Groxx]

Ah, excellent news: https://github.com/stretchr/testify/issues/608

There is a way to inject testing.T into the mock, which should absolutely be done... but it's not yet released :| Bah. Once that's released, this should be done within Cadence (if possible). If not, at least it's exposed to callers since NewTestWorkflowEnvironment returns a *Mock - loads of docs plz! This is a fairly dangerous potential pitfall.

[yiminc-zz]

Will look into this issue once that mock testing.T support is released in testify.

[Groxx]

Testify had a release! 1.2.2 has this feature: https://github.com/stretchr/testify/blob/v1.2.2/mock/mock.go#L220-L239

I can probably make a pull request to upgrade and use it internally. Since this is exposed to callers via Mock and not Cadence, there shouldn't be any need to make a breaking change / require users to upgrade Testify. Users on older Testify versions just won't have the method.

(it unfortunately doesn't output logs when tests panic, like in #562, but that's mostly a separate issue)

[helenfufu]

Hi @Groxx! Sorry to dig up a very old issue, but we are facing a similar issue with unit tests passing despite underlying activity panics when there are missing expectations. Did you happen to come up with a clean workaround to enforce failures when there are panics back when you were looking at this?