samples-go icon indicating copy to clipboard operation
samples-go copied to clipboard

RequestCancelExternalWorkflow Sample

Open mnichols opened this issue 3 years ago • 0 comments

I was trying to figure out the best way to test cancellations of Child Workflows. In the past, only cancelling the context was possible so I thought I'd try out RequestCancelExternalWorkflow.

I stumbled on some seeming inconsistencies I thought I'd highlight to verify whether a bug exists or not.

There are two tests:

  • Test_Cancel_NoMocky which does not mock out ChildWorkflow execution. The SetOnChildWorkflowCanceledListener is invoked. It is noteworthy that using OnRequestCancelExternalWorkflow fails, which I suppose makes sense but we should make clearer in docs that this API is only for mocked ChildWorkflow calls.

  • Test_Cancel_WithMocky mocks the MyChild workflow. The SetOnChildWorkflowCanceledListener is not invoked, though the SetOnChildWorkflowCompletedListener is invoked. This seems inconsistent to me. I ack that when someone uses OnRequestCancelExternalWorkflow they should Assert the expected call on that...however since tests evolve in projects and mocking is eventually introduced for children so the cancel listener no longer being called is hard to understand. We should at least call out that the listener is not called when using this API.

This sample shows how to use the RequestCancelExternalWorkflow API. There are some nuances using this not very obvious (to me):

  1. I observed that workflow tasks would fail with Unhandled Command errors (appear in workflow history) when the ChildWorkflow future was not resolved. Doing a simple .Get after calling RequestCancelExternalWorkflowExecution fixed this.
  2. Testing when using this API was not as straightforward as I'd hoped (see above).
  3. I found a bug when using OnWorkflow mock for workflows that do not have arguments...you must still pass in an argument (eg mock.Anything) or else the workflowInterceptor panics...seems like we are missing a check for params before spreading them somewhere.

mnichols avatar Dec 28 '22 05:12 mnichols