Skip to content

Conversation

@Shivs11
Copy link
Member

@Shivs11 Shivs11 commented Nov 13, 2025

What changed?

  • WISOTT

Why?

  • To be sure of the revision number mechanics we have implemented.

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests
  • added new unit test(s)
  • added new functional test(s)

Potential risks

  • None until we flip the DC switch :)

Note

Adds and updates worker versioning tests to validate auto-upgrade stability across rollbacks and transitions using revision-number mechanics.

  • Tests (tests/versioning_3_test.go):
    • New:
      • 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.
    • Updated:
      • TestAutoUpgradeWorkflows_NoBouncingBetweenVersions:
        • Enable UseRevisionNumberForWorkerVersioning and require new deployment data.
        • Use current user data version for rollback and verify via CalculateTaskQueueVersioningInfo.
        • Start idle pollers on old version to assert no task dispatch; complete and verify on new version.
        • Minor timing/propagation adjustments (e.g., removed an explicit wait).

Written by Cursor Bugbot for commit 0fb6385. This will update automatically on new commits. Configure here.

@Shivs11 Shivs11 marked this pull request as ready for review November 13, 2025 22:03
@Shivs11 Shivs11 requested review from a team as code owners November 13, 2025 22:03
}

func (s *Versioning3Suite) TestAutoUpgradeWorkflow_NoBouncingBetweenVersions() {
if !s.useNewDeploymentData {
Copy link
Contributor

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.

Copy link
Member Author

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)
Copy link
Contributor

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{
Copy link
Contributor

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() {
Copy link
Contributor

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

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