Fix Nexus test env to respect ScheduleToCloseTimeout #1636
Conversation
rodrigozhou
force-pushed
the
rodrigozhou/nexus-test-timeout
branch
2 times, most recently
from
45ff424 to
a3e3d3e
Compare
rodrigozhou
force-pushed
the
rodrigozhou/nexus-test-timeout
branch
from
a3e3d3e to
74ec134
Compare
| @@ -2363,6 +2367,37 @@ func (env *testWorkflowEnvironmentImpl) ExecuteNexusOperation(params executeNexu | |||
| } | |||
| env.runningNexusOperations[seq] = handle | |||
|
|
|||
| var opID string | |||
| if params.options.ScheduleToCloseTimeout > 0 { | |||
| // Timer to fail the nexus operation due to schedule to close timeout. | |||
There was a problem hiding this comment.
Hrmm, do we usually mimic timeouts like these in test suites? How does activity execution work in the test suite with regards to schedule to close timeout? Everything LGTM and will mark approved, just want to make sure this is normal for our test suite.
There was a problem hiding this comment.
Looks like some timeouts are broken in the test env (according to @rodrigozhou who looked into it). AFAIC, this is part of the behavior for nexus operations. It's enforced in the real server and the Java test server, don't see a good reason not to enforce it here.
There was a problem hiding this comment.
I think we need to understand/confirm how timeouts are expected to work in the Go test suite for external calls. I admit being unfamiliar. Do activities in the test suite respect their timeouts? (they might, I just want to check)
There was a problem hiding this comment.
When an activity is run by the workflow test environment (not mocked) we do propagate the timeout in the activities environment and rely on the activity to return to fail the operation which isn't how the real server works of course.
| @@ -2887,6 +2923,10 @@ func (h *testNexusOperationHandle) completedCallback(result *commonpb.Payload, e | |||
| // startedCallback is a callback registered to handle operation start. | |||
| // Must be called in a postCallback block. | |||
| func (h *testNexusOperationHandle) startedCallback(opID string, e error) { | |||
| if h.started { | |||
There was a problem hiding this comment.
How can we get duplicate starts?
There was a problem hiding this comment.
If the timer and start request race.
| @@ -960,6 +1047,77 @@ func TestWorkflowTestSuite_WorkflowRunOperation_WithCancel(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestWorkflowTestSuite_NexusSyncOperation_ScheduleToCloseTimeout(t *testing.T) { | |||
| sleepDuration := 500 * time.Millisecond | |||
| op := temporalnexus.NewSyncOperation( | |||
There was a problem hiding this comment.
A sync operation should also have a task timeout of 10s right? Are we enforcing that?
There was a problem hiding this comment.
No. Good call, we should set the Request-Timeout header here as well.
I'll submit a PR for that.
* Fix Nexus test env to respect ScheduleToCloseTimeout * address comments
What was changed
Fix Nexus test env to respect
ScheduleToCloseTimeoutand return operation time out error.Why?
Users can test the
ScheduleToCloseTimeoutwhen writing their tests.Checklist
Closes
How was this tested: