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

Refactor UpdateWorkflowExecution functional tests #4388

Merged
merged 4 commits into from May 24, 2023

Conversation

alexshtin
Copy link
Member

@alexshtin alexshtin commented May 24, 2023

What changed?
Refactor UpdateWorkflowExecution functional tests.

Why?
To reduce code duplication, and make intend clear.

How did you test it?
Run test.

Potential risks
No risks.

Is hotfix candidate?
No.

@alexshtin alexshtin force-pushed the feature/refactor-update-tests branch from 9712ab7 to fe9c63f Compare May 24, 2023 04:57
@alexshtin alexshtin marked this pull request as ready for review May 24, 2023 17:26
@alexshtin alexshtin requested a review from a team as a code owner May 24, 2023 17:26
Copy link
Contributor

@mmcshane mmcshane left a comment

Choose a reason for hiding this comment

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

For me I think TestVars is a bit much - some free functions would and assigning to local variables probably would have done just as well. But it does solve a debugability problem so thanks for that.

func New(testName string) TestVars {
return TestVars{
testName: testName,
kv: &sync.Map{},
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably would have held the sync.Map by value here and returned *TestVars

@alexshtin alexshtin force-pushed the feature/refactor-update-tests branch from 002da65 to 95023ac Compare May 24, 2023 21:59
@alexshtin alexshtin enabled auto-merge (squash) May 24, 2023 22:00
@alexshtin alexshtin merged commit c2aee40 into temporalio:master May 24, 2023
9 checks passed
@alexshtin alexshtin deleted the feature/refactor-update-tests branch May 24, 2023 23:42
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.

None yet

2 participants