Dedup duplicate update IDs for test environment #1695
Conversation
internal/workflow_testsuite_test.go
Outdated
| require.Equal(t, 0, intResult) | ||
| } | ||
| }, | ||
| env: env, |
There was a problem hiding this comment.
The current solution requires env to be passed to updateCallback, not ideal, but struggling to come up with an alternate solution
internal/internal_workflow_test.go
Outdated
| @@ -1417,6 +1418,10 @@ func (uc *updateCallback) Reject(err error) { | |||
| } | |||
|
|
|||
| func (uc *updateCallback) Complete(success interface{}, err error) { | |||
| // cache update result so we can dedup duplicate update IDs | |||
| if uc.env.impl.updateMap != nil { | |||
| uc.env.impl.updateMap[uc.env.impl.currentUpdateId] = success | |||
There was a problem hiding this comment.
The error would also need to be cached if we want to go with the caching approach
There was a problem hiding this comment.
the error here represents the users error they returned from the update handler
| @@ -229,6 +235,8 @@ type ( | |||
|
|
|||
| workflowFunctionExecuting bool | |||
| bufferedUpdateRequests map[string][]func() | |||
|
|
|||
| currentUpdateId string | |||
There was a problem hiding this comment.
What if there are multiple concurrent updates sent to the workflow? It looks like those would be lost.
| reject: func(err error) { | ||
| err := env.UpdateWorkflowByID("my-workflow-id", "update", updateID, newUpdateCallback( | ||
| env, | ||
| updateID, |
There was a problem hiding this comment.
@Quinn-With-Two-Ns how do you feel about solution? It doesn't feel ideal that we have to pass updateID in 2 places, but I can't think of any other way to handle async updates. updateCallback needs to know what the updateID to cache its result to env is, and this seems like the only way to accomplish that.
| @@ -1406,6 +1406,19 @@ type updateCallback struct { | |||
| accept func() | |||
| reject func(error) | |||
| complete func(interface{}, error) | |||
| env *TestWorkflowEnvironment | |||
There was a problem hiding this comment.
Hmm so this class is not for users, this whole file is for testing since it is post-fixed with *_test.go. The logic can't be in the updateCallback as we take an impl. from user. The logic needs to be in (env *testWorkflowEnvironmentImpl) updateWorkflow. We could potentially add a wrapper around the user interface. Does that make sense?
There was a problem hiding this comment.
Is user in this context referring to the user of the test suite?
There was a problem hiding this comment.
this whole file is for testing since it is post-fixed with *_test.go. The logic can't be in the updateCallback as we take an impl. from user
And would it be accurate to rephrase this as "we need to implement this within the test suite, not the individual test logic. updateCallback is a test specific implementation"?
There was a problem hiding this comment.
We could potentially add a wrapper around the user interface.
What interface are you referring to here? updateWorkflow?
There was a problem hiding this comment.
Is user in this context referring to the user of the test suite?
Yeah
And would it be accurate to rephrase this as "we need to implement this within the test suite, not the individual test logic. updateCallback is a test specific implementation"?
yeah, exactly
What interface are you referring to here?
UpdateCallbacks
What was changed
Added functionality in test environment to cache update
Why?
Match non-test behavior
Checklist
Closes Workflow Update in Test Environment should dedup updates by ID #1638
How was this tested:
new test written
Any docs updates needed?