AssertNumberOfCalls on an Activity succeeds even if the Activity is not called the asserted number of times.

ClairePhi commented 1 month ago

Expected Behavior

I expect AssertNumberOfCalls to succeed only with one given “expectedCalls” value.

In the “Steps to reproduce” section below, there are two assertions to assert the number of times the Activity is Called: 0 and 1. I expect that:

At least one of the two assertions fails.
In particular, the first assertion fails.

Actual Behavior

It is possible for AssertNumberOfCalls to succeed with two “expectedCalls” values.

In the “Steps to reproduce” section below, the two assertions succeed and the test succeeds.

Steps to Reproduce the Problem

main.go:

package main

import (
   "context"
   "time"

   "go.temporal.io/sdk/workflow"
)

func MyWorkflow(ctx workflow.Context, input string) error {
   return workflow.ExecuteActivity(
      workflow.WithActivityOptions(ctx, workflow.ActivityOptions{
         StartToCloseTimeout: time.Minute,
      }),
      MyActivity,
      input).Get(ctx, nil)

}

func MyActivity(ctx context.Context, input string) (string, error) {
   return input, nil
}

main_test.go:

import (
    "testing"

    "github.com/stretchr/testify/mock"
    "github.com/stretchr/testify/require"
    "go.temporal.io/sdk/testsuite"
)

func Test_Workflow(t *testing.T) {
    testSuite := testsuite.WorkflowTestSuite{}
    env := testSuite.NewTestWorkflowEnvironment()

    env.OnActivity(MyActivity, mock.Anything, "input").Return("output", nil)

    env.ExecuteWorkflow(MyWorkflow, "input")

    require.True(t, env.IsWorkflowCompleted())
    require.NoError(t, env.GetWorkflowError())

    env.AssertNumberOfCalls(t, "MyActivity", 0)
    env.AssertNumberOfCalls(t, "MyActivity", 1)
}

Specifications

go.mod

module sandbox.com

go 1.22.3

require (
   github.com/stretchr/testify v1.9.0
   go.temporal.io/sdk v1.29.1
)

require (
   github.com/davecgh/go-spew v1.1.1 // indirect
   github.com/facebookgo/clock v0.0.0-20150410010913-600d898af40a // indirect
   github.com/gogo/protobuf v1.3.2 // indirect
   github.com/golang/mock v1.6.0 // indirect
   github.com/google/uuid v1.6.0 // indirect
   github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 // indirect
   github.com/grpc-ecosystem/grpc-gateway/v2 v2.22.0 // indirect
   github.com/nexus-rpc/sdk-go v0.0.10 // indirect
   github.com/pborman/uuid v1.2.1 // indirect
   github.com/pmezard/go-difflib v1.0.0 // indirect
   github.com/robfig/cron v1.2.0 // indirect
   github.com/stretchr/objx v0.5.2 // indirect
   go.temporal.io/api v1.38.0 // indirect
   golang.org/x/exp v0.0.0-20240525044651-4c93da0ed11d // indirect
   golang.org/x/net v0.28.0 // indirect
   golang.org/x/sync v0.8.0 // indirect
   golang.org/x/sys v0.24.0 // indirect
   golang.org/x/text v0.17.0 // indirect
   golang.org/x/time v0.3.0 // indirect
   google.golang.org/genproto/googleapis/api v0.0.0-20240822170219-fc7c04adadcd // indirect
   google.golang.org/genproto/googleapis/rpc v0.0.0-20240822170219-fc7c04adadcd // indirect
   google.golang.org/grpc v1.65.0 // indirect
   google.golang.org/protobuf v1.34.2 // indirect
   gopkg.in/yaml.v3 v3.0.1 // indirect
)

Additional comments

Impact

I think that testing that an Activity or a Workflow is Called once is a common use case. I also think that it is a common use case that Activities and Workflows are not named the same (see Additional comments section below).

Users of this function might incorrectly see their tests passing when they should not.

Possible fix

My understanding is that the behavior was introduced by that PR, which makes the assertion check if the number of Activities OR the number of Workflows were called N times. Here as we have 0 workflows named MyActivity, they were never called. Hence the assertion env.AssertNumberOfCalls(t, "MyActivity", 0) succeeds.

Here is an idea to fix that. This is only an attempt - I’m not familiar with the repo:

Revert that PR -> the bug will be fixed. It will assert that nbWorkflows + nbActivities == N

In order to take into account that issue -> create two methods:

func (e *TestWorkflowEnvironment) AssertNumberOfCallsActivity(t mock.TestingT, methodName string, expectedCalls int) bool
func (e *TestWorkflowEnvironment) AssertNumberOfCallsWorkflow(t mock.TestingT, methodName string, expectedCalls int) bool

If you agree on the principle, I can open a PR.

cretz commented 1 month ago

create two methods:

The linked PR at https://github.com/temporalio/sdk-go/pull/1371 created AssertActivityNumberOfCalls and AssertWorkflowNumberOfCalls, can you confirm they work for your use case?

ClairePhi commented 1 month ago

can you confirm they work for your use case?

Yes I think they would fit well for my second bullet point, thank you.

I still believe that env.AssertNumberOfCalls behavior could introduce errors in user tests. According to the method naming, if someone asserts that an Activity was called once, then they would expect the test to fail if the Activity was never called, and it doesn't.

cretz commented 1 month ago

https://github.com/temporalio/sdk-go/pull/1371 was maybe developed wrong. @Quinn-With-Two-Ns - thoughts?

Quinn-With-Two-Ns commented 1 month ago

AssertNumberOfCalls is inherently ambiguous since it doesn't differentiate between workflow, activity or nexus calls. If you are trying to assert on the number of activity calls you should use AssertActivityNumberOfCalls .

temporalio / sdk-go