Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heartbeat timeout not raised while testing #1282

Open
miquelpuigmena opened this issue Oct 26, 2023 · 5 comments
Open

Heartbeat timeout not raised while testing #1282

miquelpuigmena opened this issue Oct 26, 2023 · 5 comments

Comments

@miquelpuigmena
Copy link

I want to see how an activity times out due to heartbeat timeout while testing. But it doesn't raise the Heartbeat timeout. See the tests in Steps to Reproduce the Problem section

Expected Behavior

I would expect to see the test failing due to HeartbeatTimeout

Actual Behavior

The tests are passing

Steps to Reproduce the Problem

  • make a folder with workflow.go and workflow_test.go below 👇
  • go test ./...

workflow.go

package hbtimeout

import (
	"context"
	"time"

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

type In struct {
	Sleep     int
	Heartbeat int
}

func Workflow(ctx workflow.Context, in In) error {
	options := workflow.ActivityOptions{
		StartToCloseTimeout: time.Second * 10,
		HeartbeatTimeout:    time.Second * time.Duration(in.Heartbeat),
		RetryPolicy: &temporal.RetryPolicy{
			MaximumAttempts: 1,
		},
	}

	ctx = workflow.WithActivityOptions(ctx, options)

	err := workflow.ExecuteActivity(ctx, Do, in.Sleep).Get(ctx, nil)
	if err != nil {
		return err
	}

	return nil
}

func Do(ctx context.Context, sleep int) error {
	time.Sleep(time.Second * time.Duration(sleep))
	return nil
}

workflow_test.go

package hbtimeout

import (
	"testing"
	"time"

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

type UnitTestSuite struct {
	suite.Suite
	testsuite.WorkflowTestSuite
}

func TestUnitTestSuite(t *testing.T) {
	suite.Run(t, new(UnitTestSuite))
}

func (s *UnitTestSuite) Test_Workflow() {
	env := s.NewTestWorkflowEnvironment()
	env.SetTestTimeout(time.Second * 20)
	env.RegisterActivity(Do)
	env.ExecuteWorkflow(Workflow, In{Sleep: 9, Heartbeat: 1})
	s.True(env.IsWorkflowCompleted())
	s.NoError(env.GetWorkflowError())
	env.AssertExpectations(s.T())
}

Specifications

  • Version:
  • Platform: testing locally
@gugabfigueiredo
Copy link

gugabfigueiredo commented Nov 3, 2023

as pointed out in the #1167 , activity timeouts are hardcoded to 10min in testWorkflowEnvironmentImpl.executeActivity

func (env *testWorkflowEnvironmentImpl) executeActivity(
	activityFn interface{},
	args ...interface{},
) (converter.EncodedValue, error) {

        ...

	parameters := ExecuteActivityParams{
		ExecuteActivityOptions: ExecuteActivityOptions{
			ScheduleToCloseTimeout: 600 * time.Second,
			StartToCloseTimeout:    600 * time.Second,
		},
		ActivityType: *activityType,
		Input:        input,
		Header:       env.header,
	}

        ...
}

heartbeatTimeouts are straight up ignored

as a workaround I have been testing for consistent heartbeat strategies with the following, changing the code in workflow_test.go:

func (s *UnitTestSuite) Test_Workflow() {
	env := s.NewTestWorkflowEnvironment()
	env.SetTestTimeout(time.Second * 1)
	env.RegisterActivity(Do)
        defer func() {
		err := recover()
		if r != nil {
			s.True(strings.HasPrefix(err.(string), "test timeout"))
		}
	}()
	env.ExecuteWorkflow(Workflow, In{Sleep: 9, Heartbeat: 2})
}

when heartbeats are set appropriately, they will keep test workflow alive and it is possible to test for consistent timeout error over longer workload.

@cretz
Copy link
Member

cretz commented Nov 3, 2023

The workflow test environment is meant mostly for just testing workflows though it has some activity support. You should use a TestActivityEnvironment to test activity behavior, and if you want to test how a workflow reacts to an activity result/failure, you can mock it. If you must test these together with real-world behavior semantics, you should use a real server (e.g. testsuite.StartDevServer).

@gugabfigueiredo
Copy link

gugabfigueiredo commented Nov 5, 2023

a TestActivityEnvironment is just a wrapper fortestWorkflowEnvironmentImpl (/internal/workflow_testsuite.go#L70)

	// TestActivityEnvironment is the environment that you use to test activity
	TestActivityEnvironment struct {
		impl *testWorkflowEnvironmentImpl
	}

which means that when running activities, the same limitations apply, this makes the test environment ignore developer setup and not behave as expected, as we cannot provide or set ExecuteActivityOptions.

[Edit]: In my case, I was just trying to validate heartbeat strategies considering different workloads

will try testsuite.StartDevServer, which at least I can write into tests to get the expected behavior

@gugabfigueiredo
Copy link

will try testsuite.StartDevServer, which at least I can write into tests to get the expected behaviour

This worked for my needs (see here).

I suppose then it comes to temporal developers wether there is a use case or not for normal configurations to the Activity and Workflow test environments.

Personally, I would argue in favour of test environment interface being able to deal with this, as it seems like a lot of work to validate implementation against a simple feature. The heartbeat timeout is still set by the internal implementation, its just set to zero value while other timeouts are hardcoded (/internal/internal_workflow_testsuite.go#L579):

	parameters := ExecuteActivityParams{
		ExecuteActivityOptions: ExecuteActivityOptions{
			ScheduleToCloseTimeout: 600 * time.Second,
			StartToCloseTimeout:    600 * time.Second,
		},
		ActivityType: *activityType,
		Input:        input,
		Header:       env.header,
	}

...

	scheduleTaskAttr.HeartbeatTimeout = &parameters.HeartbeatTimeout

...

@cretz
Copy link
Member

cretz commented Nov 6, 2023

a TestActivityEnvironment is just a wrapper for testWorkflowEnvironmentImpl [...] which means that when running activities, the same limitations apply

But it's a different level of exposed abstraction. The point is that if you want to test how your workflow reacts to a certain type of activity failure, mock that activity and return that error. If you want to test how your activity works regardless of workflow, use the activity environment. If you must test all behaviors of activity and workflow together based on real-time, it may not make sense to use an activity or time-skipping workflow environment, it may make sense to use a real combined one.

Some timeouts we do respect in the workflow environment however, but are on time-skipping basis not based on real time. I will leave this issue open to investigate if real-time heartbeat timeout can/should be respected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants