-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Worker-Versioning GA: Tests for AutoUpgrade workflows not bouncing back and forth. #8635
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| func (s *Versioning3Suite) TestAutoUpgradeWorkflow_NoBouncingBetweenVersions() { | ||
| if !s.useNewDeploymentData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this bool is always false, right? you should use useRevisionNumbers because that's the flag that controls this logic. in that case you don't need to set UseRevisionNumberForWorkerVersioning again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - actually, now that I think about this and read all your latter comments, it's best if we get this PR in after my other open PR goes in.
The reason why I say this is because I have done some refactoring there anyways, which shall reduce the clutter here.
| }}, []string{}, tqTypeWf, tqTypeAct) | ||
|
|
||
| // Wait until all task queue partitions know that v1 is current. | ||
| s.waitForDeploymentDataPropagation(tv1, versionStatusCurrent, false, tqTypeWf, tqTypeAct) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, later when we clean up all old tests we should maybe change this function to wait for the particular revision number propagation rather than status of the version.
| }) | ||
| s.NoError(err) | ||
|
|
||
| _, err = s.GetTestCluster().MatchingClient().UpdateTaskQueueUserData(context.Background(), &matchingservice.UpdateTaskQueueUserDataRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine you use this in other similar tests, no? seems like a good candidate to extract as a helper
| }, 10*time.Second, 100*time.Millisecond) | ||
| } | ||
|
|
||
| func (s *Versioning3Suite) TestAutoUpgradeWorkflows_NoBouncingBetweenVersions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is names same as above, is it testing the same scenario?
|
|
||
| func (s *Versioning3Suite) TestAutoUpgradeWorkflows_NoBouncingBetweenVersions() { | ||
| s.T().Skip("This test is flaky right now and shall be fixed in a future PR.") // TODO (Shivam) | ||
| if !s.useNewDeploymentData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
What changed?
Why?
How did you test it?
Potential risks
Note
Adds and updates worker versioning tests to validate auto-upgrade stability across rollbacks and transitions using revision-number mechanics.
tests/versioning_3_test.go):TestAutoUpgradeWorkflow_NoBouncingBetweenVersions: ensures a single workflow does not bounce between versions when user data is rolled back.TestWorkflowTQLags_DependentActivityStartsTransition: verifies activity on newer version triggers workflow transition when workflow TQ lags.TestAutoUpgradeWorkflows_NoBouncingBetweenVersions:UseRevisionNumberForWorkerVersioningand require new deployment data.CalculateTaskQueueVersioningInfo.Written by Cursor Bugbot for commit 0fb6385. This will update automatically on new commits. Configure here.