Inconsistent behavior when asserting invocations

[embano1]

Expected Behavior

Invocation count assertions provided by the mocking library, such as Once() or Times(n) should not show different error/panic behavior affecting test results and always fail a test if the assertion does not hold.

Either it should be documented that these methods should not be used due to this behavior or fixed, e.g. when recovering from panic and failing the test instead of passing.

Actual Behavior

This activity is configured with retries. Under test, it throws PanicError in the logs when the code is executed multiple times (retries) but the test succeeds with exit code 0.

env.OnActivity("MyActivity", any, any).Return(nil, errors.New("test failure")).Once() 

This activity is also configured with retries but since no error is returned it is only invoked once. Under test, it fails the test with a mocking error indicating that the function was only called once, i.e. exit code != 0.

env.OnActivity("MyActivity", any, any).Return(nil, nil).Twice()

cc/ @cretz

Steps to Reproduce the Problem

See above

Specifications

• Version: go.temporal.io/sdk v1.10.0
• Platform: n/a

[alfa-alex]

Ran into the same issue (go.temporal.io/sdk v1.15.0).

What's your current position on this? Should the use of invocation count assertions be avoided at the moment?

[alfa-alex]

Actually for my use-case there was a relatively simple fix to this: Calling Test on the mock.

So, instead of the original

// NewTestWorkflowEnvironment creates a new instance of TestWorkflowEnvironment. Use the returned TestWorkflowEnvironment
// to run your workflow in the test environment.
func (s *WorkflowTestSuite) NewTestWorkflowEnvironment() *TestWorkflowEnvironment {
	return &TestWorkflowEnvironment{impl: newTestWorkflowEnvironmentImpl(s, nil)}
}

I'm now constructing the TestWorkflowEnvironment with:

// NewTestWorkflowEnvironmentWithTest creates a new instance of TestWorkflowEnvironment. Use the returned TestWorkflowEnvironment
// to run your workflow in the test environment.
func (s *WorkflowTestSuite) NewTestWorkflowEnvironmentWithTest(t mock.TestingT) *TestWorkflowEnvironment {
	env := &TestWorkflowEnvironment{impl: newTestWorkflowEnvironmentImpl(s, nil)}
	env.mock.Test(t)
	return env
}

in the workflow_testsuite.go. This does actually fail the test in the above case.

Well possible this has undesired side-effects, but maybe something to consider. An alternative to having a different constructor would of course be to add a Setter func to the TestWorkflowEnvironment, but it felt quite naturally to me to construct the env with the test.

[cretz]

Under test, it throws PanicError in the logs when the code is executed multiple times (retries) but the test succeeds with exit code 0.

IIUC the original issue correctly, the code properly panics at call time when the invocation count is wrong, but the user is not checking env.GetWorkflowError() to get the actual workflow error so it doesn't fail their test.

The test environment is not just for running with the Go testing library. A panic calling the mock, whether that's for the assertion not holding or the body of the mock panicking, will follow normal panic paths. It is up to the user whether they treat a panic in a workflow as an error in their chosen environment.

Alternatively, if you can't properly assert that the panic does not occur in the workflow for whatever reason, you can just count invocations inside the body of the mock and assert later.

Does that help? Or am I misunderstanding the concern?

[alfa-alex]

So you imply we should check the invocation count assertions via env.GetWorkflowError()? I thought that's what env.AssertExpectations(t) is for. The documentation reads:

// AssertExpectations  asserts that everything specified with OnActivity
// in fact called as expected.  Calls may have occurred in any order.

To give you a minimal working example, change the Test_Workflow test of the helloworld sample in the temporal-samples project to:

	testSuite := &testsuite.WorkflowTestSuite{}
	env := testSuite.NewTestWorkflowEnvironment()

	// Mock activity implementation
	env.OnActivity(Activity, mock.Anything, "Temporal").Return("", errors.New("some error")).Once()
	env.ExecuteWorkflow(Workflow, "Temporal")

	require.True(t, env.IsWorkflowCompleted())
	env.AssertExpectations(t)

Even though the output correctly indicates that Activity was called more than once, the test doesn't fail. And env.AssertExpectations returns true.

Am I misunderstanding the description of that function?

[cretz]

So you imply we should check the invocation count assertions via env.GetWorkflowError()? I thought that's what env.AssertExpectations(t) is for.

My mistake, yes, you can and should use that function if you want to assert the mocking expectations and you are in a test setting. I had forgotten about this helper. But you also want to check no error happened inside the workflow by checking the workflow error.

The docs for https://pkg.go.dev/github.com/stretchr/testify/mock#Mock.AssertExpectations basically calls https://github.com/stretchr/testify/blob/v1.8.0/mock/mock.go#L610-L612 which seems to suggest it only fails if there were fewer calls than expected, not more. I believe this is how that mocking library works - when invoking more than expected, error at callsite, when invoking fewer than expected, error at expectation assertion site.

[alfa-alex]

Yeah, it looks like a testify issue. I think it's been described here: https://github.com/stretchr/testify/issues/608.

Thank you!

---

Edit: To summarize, the invocation count assertions should probably best be avoided. Other than that you could use the following "solution" from a user side (if you need this in a test and you are testing a workflow run that is expected to fail, so you want to distinguish the errors):

err := env.GetWorkflowError()
var panicErr *temporal.PanicError
if ok := errors.As(err, &panicErr); ok && strings.HasPrefix(panicErr.Error(), "\nassert: mock:") {
	assert.Fail(t, "failed mock assertion")
}

[cretz]

invocation count assertions should probably best be avoided

Concur. And on a general best-practices note, the ease of adding counts can tempt one to overly assert counts leading to unnecessary expectations about the internals of the code under test. But if you had to manually count if you needed to confirm invocation counts, you'd be more willing to only do it where it was important to verify behavior.