Don't run workflowCancelHandler in test harness #1296
Conversation
| var canceledErr *CanceledError | ||
| if errors.As(err, &canceledErr) && env.workflowCancelHandler != nil { | ||
| env.workflowCancelHandler() | ||
| } |
There was a problem hiding this comment.
Can you help me understand this change? Is it just because As checks errors at every depth and if so, maybe we should change to just checking the exact error type instead of hierarchy? Can you confirm that there is no other internal way for a cancel to be sent to a test workflow than via TestWorkflowEnvironment.CancelWorkflow() (which appears to still invoke this callback)?
There was a problem hiding this comment.
This change is because canceling a workflow that has already returned is not valid. If the workflow function has already returned what valid effect can the cancel call have?
There was a problem hiding this comment.
it is also inconsistent with executing a workflow normally not in the test suite. Executing a workflow normally does not have this behavior so it is an inconsistency that leads to panics in the SDK
There was a problem hiding this comment.
👍 I was originally concerned that there was a user-facing callback that could be registered to this, but I have manually confirmed there is not.
Don't run workflowCancelHandler in test harness on completed workflows.
Quinn-With-Two-Ns
force-pushed
the
issue-1287
branch
from
96622d4 to
888ebd1
Compare
Don't run workflowCancelHandler in test harness on completed workflows. If a workflow is completed nothing else in the workflow function should run.
closes #1287