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

RequestCancelExternalWorkflow Sample #240

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mnichols
Copy link

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.

return nil
})
s.env.SetOnChildWorkflowCanceledListener(func(workflowInfo *workflow.Info) {
childCancelInfo = workflowInfo
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not get called, presumably because we are mocking the ChildWorkflow...but note that the SetOnChildWorkflowCompletedListener does get called, so it is a little surprising (not obvious).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the go test suite code I think the current behavior is correct. Right now the workflow assumes the child WF will be blocked until it is signaled so when you call RequestCancelExternalWorkflow you know it will cancel the child WF. In this test since you mock the child WF, the child WF will instantly complete so your call to RequestCancelExternalWorkflow will not cancel the child WF since it is already complete (not running) so you don't get the callback.

s.env.SetOnChildWorkflowCompletedListener(func(workflowInfo *workflow.Info, result converter.EncodedValue, err error) {
childCompletedInfo = workflowInfo
})
s.env.OnRequestCancelExternalWorkflow(mock.Anything, childWorkflowID, "").Return(nil).Once()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must assert on this if you are mocking a child workflow and want to verify the cancellation of it. The cancellation listener does not get called.

s.True(s.env.IsWorkflowCompleted())
s.NoError(s.env.GetWorkflowError())
//s.True(cancelRequestCalled)
s.NotNil(childCancelInfo)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails because the cancellation listener isnt invoked.

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

Successfully merging this pull request may close these issues.

2 participants