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.

Sign up for GitHub

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

Jump to bottom

Fix flaky TestMultipleClientsWithMixedLabelsAndExpectFailure #599

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Fix flaky TestMultipleClientsWithMixedLabelsAndExpectFailure #599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/find-flaky-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ jobs:
find-flaky-tests:
# Only run this workflow when the comment contains '/find-flaky-tests' or it's manually triggered
if: contains(github.event.comment.body, '/find-flaky-tests') || github.event_name == 'workflow_dispatch'
fail-fast: false
strategy:
fail-fast: false
Comment on lines -11 to +12
Copy link
Member Author

@julienduchesne julienduchesne Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll get it right sometime... I managed to start a run anyways since "workflow dispatch" seems to use the workflow config from the branch, while a workflow triggered from issue comments takes the workflow config from main (while checking out the branch to run tests)

matrix:
runs: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20] # Run the build workflow 20 times
uses: ./.github/workflows/test-build.yml
Expand Down
9 changes: 8 additions & 1 deletion kv/memberlist/memberlist_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ func TestMultipleClientsWithMixedLabelsAndExpectFailure(t *testing.T) {

err := testMultipleClientsWithConfigGenerator(t, len(membersLabel), configGen)
require.Error(t, err)
require.Contains(t, err.Error(), fmt.Sprintf("expected to see at least %d updates, got", len(membersLabel)))
require.Contains(t, err.Error(), fmt.Sprintf("expected to see %d members, got", len(membersLabel)))
}

func TestMultipleClientsWithMixedLabelsAndClusterLabelVerificationDisabled(t *testing.T) {
Expand Down Expand Up @@ -721,6 +721,7 @@ func testMultipleClientsWithConfigGenerator(t *testing.T, members int, configGen
firstKv := clients[0]
ctx, cancel := context.WithTimeout(context.Background(), casInterval*3) // Watch for 3x cas intervals.
updates := 0
gotMembers := 0
firstKv.WatchKey(ctx, key, func(in interface{}) bool {
updates++

Expand All @@ -733,12 +734,18 @@ func testMultipleClientsWithConfigGenerator(t *testing.T, members int, configGen
"tokens, oldest timestamp:", now.Sub(time.Unix(minTimestamp, 0)).String(),
"avg timestamp:", now.Sub(time.Unix(avgTimestamp, 0)).String(),
"youngest timestamp:", now.Sub(time.Unix(maxTimestamp, 0)).String())
gotMembers = len(r.Members)
return true // yes, keep watching
})
cancel() // make linter happy

t.Logf("Ring updates observed: %d", updates)

// We expect that all members are in the ring
if gotMembers != members {
return fmt.Errorf("expected to see %d members, got %d", members, gotMembers)
}

if updates < members {
// in general, at least one update from each node. (although that's not necessarily true...
// but typically we get more updates than that anyway)
Expand Down