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

regenerate mocks under client to allow dynamic returns based on arguments #1353

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

yinsidij
Copy link
Contributor

@yinsidij yinsidij commented Jan 15, 2024

What was changed

mocker for interfaces under client folder

Why?

Looks the mocks are old and only supports returning value rather than the function.
With updated mocker by latest mockery, (example https://github.com/temporalio/sdk-go/compare/master...yinsidij:yjiao/regenerate_mocks?expand=1#diff-baf21238e036d6550f43915acceadaf7448d1ff493453a0da0d41a232e20fa6fR217)
It allows dynamic return based on argument.

For example, with the newly generated mock, we could do following that provides more flexibility.

	mockClient.On("ExecuteWorkflow", mock.Anything, mock.AnythingOfType("internal.StartWorkflowOptions"), mock.AnythingOfType("string"), mock.Anything).Return(
		func(ctx context.Context, options client.StartWorkflowOptions, workflow string, newCustomerMetadata *datamodel.NewCustomerMetadata) (client.WorkflowRun, error) {
			if newCustomerMetadata.CustomerID == "bad" {
				return nil, fmt.Errorf("error")
			}
			return nil, nil
		},
	).Maybe()

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@yinsidij yinsidij requested a review from a team as a code owner January 15, 2024 08:10
@CLAassistant
Copy link

CLAassistant commented Jan 15, 2024

CLA assistant check
All committers have signed the CLA.

@cretz
Copy link
Member

cretz commented Jan 16, 2024

Please approve CLA and fix copyright headers of files

@yinsidij yinsidij changed the title Please help regenerate mocks to allow dynamic returns based on arguments regenerate mocks under client to allow dynamic returns based on arguments Jan 16, 2024
@yinsidij
Copy link
Contributor Author

Please approve CLA and fix copyright headers of files

signed CLA and iterated the PR.

@yinsidij
Copy link
Contributor Author

one CI job failed. somehow I cannot find a way to re trigger the job.

I locally ran this test and passed. Looks like rerun should be also resolve it.
image

Could you please help rerun the failed CI jobs?
@Quinn-With-Two-Ns @cretz

mocks/Client.go Outdated
@@ -162,23 +228,30 @@ func (_m *Client) DescribeWorkflowExecution(ctx context.Context, workflowID stri
}

// ExecuteWorkflow provides a mock function with given fields: ctx, options, workflow, args
func (_m *Client) ExecuteWorkflow(ctx context.Context, options client.StartWorkflowOptions, workflow interface{}, args ...interface{}) (client.WorkflowRun, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

All these internal.* need to be replaced by the appropriate exposed type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Quinn-With-Two-Ns
thanks for review.
Actually internal.WorkflowRun is equivalent to client.WorkflowRun. FYI.

WorkflowRun = internal.WorkflowRun

I will still make the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Quinn-With-Two-Ns iterated. Not sure why build-and-test is not started. Is it just slow?

Copy link
Contributor

Choose a reason for hiding this comment

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

It requires manual approval

@yinsidij yinsidij force-pushed the yjiao/regenerate_mocks branch from 16c1898 to d0a3fcc Compare January 17, 2024 00:36
Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution

@yinsidij
Copy link
Contributor Author

yinsidij commented Jan 17, 2024

Only those with write access to this repository can merge pull requests.

Thanks. Looks like I don't have permission to merge. What is the process for merging the PR ?

And I have another PR that completes the all existing mockers under mockers using latest mockery. #1361

We can do two merges. But happy to help merge with one PR here if you think it's necessary.

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit 50ab40d into temporalio:master Jan 17, 2024
12 checks passed
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.

4 participants