Skip to content

test(destination): Replace sleeps with RetryFor in ClusterStoreTest#14250

Merged
cratelyn merged 2 commits into
mainfrom
alex/why-not-retry-five
Jul 17, 2025
Merged

test(destination): Replace sleeps with RetryFor in ClusterStoreTest#14250
cratelyn merged 2 commits into
mainfrom
alex/why-not-retry-five

Conversation

@adleong

@adleong adleong commented Jul 17, 2025

Copy link
Copy Markdown
Member

The cluster store tests use a time.Sleep to wait for events to propagate through the kubernetes API. However, this is flaky and unreliable and can cause test failures when event propagation is slow.

Replace the 50ms sleep with a RetryFor which checks for the desired state once per second for up to 30 seconds.

Signed-off-by: Alex Leong <alex@buoyant.io>
@adleong adleong requested a review from a team as a code owner July 17, 2025 01:43
Comment on lines +97 to +103
cs.RLock()
actualLen := len(cs.store)
cs.RUnlock()

if actualLen != len(tt.expectedClusters) {
return fmt.Errorf("expected to see %d cache entries, got: %d", len(tt.expectedClusters), actualLen)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These lines are duplicated below

@cratelyn cratelyn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thank you for tracking this down, @adleong. i really appreciate you taking the time to find this, and to open this pull request.

@cratelyn cratelyn merged commit 74d117a into main Jul 17, 2025
67 of 71 checks passed
@cratelyn cratelyn deleted the alex/why-not-retry-five branch July 17, 2025 15:47
@cratelyn

Copy link
Copy Markdown
Member

and likewise, thank you @alpeb for taking the time to review and apply changes to this pull request.

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