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

Dedup duplicate update IDs for test environment #1695

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

Conversation

yuandrew
Copy link
Contributor

What was changed

Added functionality in test environment to cache update

Why?

Match non-test behavior

Checklist

  1. Closes Workflow Update in Test Environment should dedup updates by ID #1638

  2. How was this tested:
    new test written

  3. Any docs updates needed?

@@ -1406,6 +1406,19 @@ type updateCallback struct {
accept func()
reject func(error)
complete func(interface{}, error)
env *TestWorkflowEnvironment
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so this class is not for users, this whole file is for testing since it is post-fixed with *_test.go. The logic can't be in the updateCallback as we take an impl. from user. The logic needs to be in (env *testWorkflowEnvironmentImpl) updateWorkflow. We could potentially add a wrapper around the user interface. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is user in this context referring to the user of the test suite?

Copy link
Contributor Author

@yuandrew yuandrew Nov 5, 2024

Choose a reason for hiding this comment

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

this whole file is for testing since it is post-fixed with *_test.go. The logic can't be in the updateCallback as we take an impl. from user

And would it be accurate to rephrase this as "we need to implement this within the test suite, not the individual test logic. updateCallback is a test specific implementation"?

Copy link
Contributor Author

@yuandrew yuandrew Nov 5, 2024

Choose a reason for hiding this comment

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

We could potentially add a wrapper around the user interface.

What interface are you referring to here? updateWorkflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is user in this context referring to the user of the test suite?

Yeah

And would it be accurate to rephrase this as "we need to implement this within the test suite, not the individual test logic. updateCallback is a test specific implementation"?

yeah, exactly

What interface are you referring to here?

UpdateCallbacks

@yuandrew yuandrew marked this pull request as ready for review November 12, 2024 18:19
@yuandrew yuandrew requested a review from a team as a code owner November 12, 2024 18:19
}, 0)

env.RegisterDelayedCallback(func() {
env.UpdateWorkflow("update", "id", &updateCallback{
Copy link
Contributor

Choose a reason for hiding this comment

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

What if this update is sent before the first update completes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, added a mutex to the map to ensure the 2nd update must wait for the 1st update to complete

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.

Workflow Update in Test Environment should dedup updates by ID
2 participants