Skip to content

Commit

Permalink
Really fix flaky TestMultipleClients tests
Browse files Browse the repository at this point in the history
#599 went in a wrong direction, so I revert it here: it makes all `TestMultipleClients` tests flaky since not all members are necessarily joined after the updates

Instead, to fix the `TestMultipleClientsWithMixedLabelsAndExpectFailure` test, I add a couple more labeled members, that way, even if we reach 3 updates (which happened some times), we'll never get to 5 with a single member

Also, try to fix the `TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA1` test by closing GRPC connections. Maybe this (golang/go#36700) is the issue?
  • Loading branch information
julienduchesne committed Oct 9, 2024
1 parent 9bd6dd3 commit 42fcc5d
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 10 deletions.
3 changes: 1 addition & 2 deletions crypto/tls/test/tls_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,8 @@ func newIntegrationClientServer(
dialOptions = append([]grpc.DialOption{grpc.WithDefaultCallOptions(clientConfig.CallOptions()...)}, dialOptions...)

conn, err := grpc.NewClient(grpcHost, dialOptions...)
assert.NoError(t, err, tc.name)
require.NoError(t, err, tc.name)
require.NoError(t, err, tc.name)
defer conn.Close()

client := grpc_health_v1.NewHealthClient(conn)

Expand Down
13 changes: 5 additions & 8 deletions kv/memberlist/memberlist_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -590,12 +590,16 @@ func TestMultipleClientsWithMixedLabelsAndExpectFailure(t *testing.T) {
// 1) ""
// 2) "label1"
// 3) "label2"
// 4) "label3"
// 5) "label4"
//
// We expect that it won't be possible to build a memberlist cluster with mixed labels.
var membersLabel = []string{
"",
"label1",
"label2",
"label3",
"label4",
}

configGen := func(i int) KVConfig {
Expand All @@ -609,7 +613,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 %d members, got", len(membersLabel)))
require.Contains(t, err.Error(), fmt.Sprintf("expected to see at least %d updates", len(membersLabel)))
}

func TestMultipleClientsWithMixedLabelsAndClusterLabelVerificationDisabled(t *testing.T) {
Expand Down Expand Up @@ -720,7 +724,6 @@ 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,18 +736,12 @@ 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

0 comments on commit 42fcc5d

Please sign in to comment.