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

Support for mocking nexus operations #1666

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rodrigozhou
Copy link
Contributor

What was changed

Support for mocking nexus operations in the test framework.

Why?

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@bergundy
Copy link
Member

This PR could use some tests and there would be an example in the documentation showing how this may be used. I want to understand the experience you had in mind better before diving deeper into the review.

func (env *testWorkflowEnvironmentImpl) RegisterNexusAsyncOperationCompletion(
service string,
operation string,
operationID string,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this would be intuitive for users to specify the operationID at registration time.
Also don't we need locks to protect the map here?

Copy link
Contributor Author

@rodrigozhou rodrigozhou Oct 10, 2024

Choose a reason for hiding this comment

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

I wonder if this would be intuitive for users to specify the operationID at registration time.

The user needs to specify the operation id when specifying the return value of the mock call.

Also don't we need locks to protect the map here?

Why do we need to protect this map?

panic(fmt.Sprintf("mock of ExecuteNexusOperation failed to deserialize input"))
}

// rebuild the input as *nexus.LazyValue
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, after I call input.Consume on L3208, it closed the reader, so it can't call again, which is needed if the user didn't mock the operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, actually, I can see that the reader isn't really being used at this moment, so maybe I could just skip this...

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

LGTM, but would wait until @Quinn-With-Two-Ns takes a look

Comment on lines +579 to +598
// t.OnNexusOperation(
// service,
// HelloOperation,
// HelloInput{Message: "Temporal"},
// mock.Anything,
// ).Return(
// &nexus.HandlerStartOperationResultAsync{
// OperationID: "hello-operation-id",
// },
// nil,
// )
//
// t.RegisterNexusAsyncOperationCompletion(
// service,
// HelloOperation.Name(),
// "hello-operation-id",
// HelloOutput{Message: "Hello Temporal"},
// nil,
// 1*time.Second,
// )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to this, maybe we could have helper functions to mock sync and async operations. Example: for async operation, we could have something like this:

func (e *TestWorkflowEnvironment) MockNexusAsyncOperation(
	service any,
	operation any,
	input any,
	options any,
	result any,
	err error,
	delay time.Duration,
) *MockCallWrapper {
	operationID := uuid.New() // maybe also input?
	m := e.OnNexusOperation(service, operation, input, options).Return(
		&nexus.HandlerStartOperationResultAsync{
			OperationID: operationID,
		},
		nil,
	)
	e.RegisterNexusAsyncOperationCompletion(service, operation, operationID, result, err, delay)
	return m
}

And we could also have something similar for the sync which basically abstract the returned value type nexus.HandlerStartOperationResultSync.

Comment on lines +3164 to +3166
if h.env.onNexusOperationCanceledListener != nil {
h.env.onNexusOperationCanceledListener(h.params.client.Service(), h.params.operation)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how useful is the canceled listener here. It doesn't seem that the user can cancel a Nexus operation through the SDK.

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.

3 participants