Skip to content

Commit

Permalink
Refactor unrelated tests to use generalized anti-flake measures - pass …
Browse files Browse the repository at this point in the history 
…#1
  • Loading branch information
@analogue
analogue committed Jan 8, 2024
1 parent a25ebf8 commit 0a2eedd
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 232 deletions.
67 changes: 10 additions & 57 deletions control-plane/config-entries/controllersv2/consul_resource_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,6 @@ func TestConsulResourceController_CreatesConsulResource(t *testing.T) {
testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) {
c.Experiments = []string{"resource-apis"}
})
resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

r := c.reconciler(fakeClient, testClient.Cfg, testClient.Watcher, logrtest.New(t))
namespacedName := types.NamespacedName{
Expand All @@ -163,7 +156,7 @@ func TestConsulResourceController_CreatesConsulResource(t *testing.T) {
require.False(t, resp.Requeue)

req := &pbresource.ReadRequest{Id: c.resource.ResourceID(constants.DefaultConsulNS, constants.DefaultConsulPartition)}
res, err := resourceClient.Read(ctx, req)
res, err := testClient.ResourceClient.Read(ctx, req)
require.NoError(t, err)
require.NotNil(t, res)
require.Equal(t, c.resource.GetName(), res.GetResource().GetId().GetName())
Expand Down Expand Up @@ -289,20 +282,13 @@ func TestConsulResourceController_UpdatesConsulResource(t *testing.T) {
testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) {
c.Experiments = []string{"resource-apis"}
})
resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

// We haven't run reconcile yet, so we must create the resource
// in Consul ourselves.
{
resource := c.resource.Resource(constants.DefaultConsulNS, constants.DefaultConsulPartition)
req := &pbresource.WriteRequest{Resource: resource}
_, err := resourceClient.Write(ctx, req)
_, err := testClient.ResourceClient.Write(ctx, req)
require.NoError(t, err)
}

Expand All @@ -329,7 +315,7 @@ func TestConsulResourceController_UpdatesConsulResource(t *testing.T) {

// Now check that the object in Consul is as expected.
req := &pbresource.ReadRequest{Id: c.resource.ResourceID(constants.DefaultConsulNS, constants.DefaultConsulPartition)}
res, err := resourceClient.Read(ctx, req)
res, err := testClient.ResourceClient.Read(ctx, req)
require.NoError(t, err)
require.NotNil(t, res)
require.Equal(t, c.resource.GetName(), res.GetResource().GetId().GetName())
Expand Down Expand Up @@ -411,20 +397,13 @@ func TestConsulResourceController_DeletesConsulResource(t *testing.T) {
testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) {
c.Experiments = []string{"resource-apis"}
})
resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

// We haven't run reconcile yet, so we must create the config entry
// in Consul ourselves.
{
resource := c.resource.Resource(constants.DefaultConsulNS, constants.DefaultConsulPartition)
req := &pbresource.WriteRequest{Resource: resource}
_, err := resourceClient.Write(ctx, req)
_, err := testClient.ResourceClient.Write(ctx, req)
require.NoError(t, err)
}

Expand All @@ -443,7 +422,7 @@ func TestConsulResourceController_DeletesConsulResource(t *testing.T) {

// Now check that the object in Consul is as expected.
req := &pbresource.ReadRequest{Id: c.resource.ResourceID(constants.DefaultConsulNS, constants.DefaultConsulPartition)}
_, err = resourceClient.Read(ctx, req)
_, err = testClient.ResourceClient.Read(ctx, req)
require.Error(t, err)
require.True(t, isNotFoundErr(err))
}
Expand Down Expand Up @@ -485,11 +464,6 @@ func TestConsulResourceController_ErrorUpdatesSyncStatus(t *testing.T) {
c.Experiments = []string{"resource-apis"}
})

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

// Stop the server before calling reconcile imitating a server that's not running.
_ = testClient.TestServer.Stop()

Expand Down Expand Up @@ -568,13 +542,6 @@ func TestConsulResourceController_SetsSyncedToTrue(t *testing.T) {
testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) {
c.Experiments = []string{"resource-apis"}
})
resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

reconciler := &TrafficPermissionsController{
Client: fakeClient,
Expand All @@ -590,7 +557,7 @@ func TestConsulResourceController_SetsSyncedToTrue(t *testing.T) {
{
resource := trafficpermissions.Resource(constants.DefaultConsulNS, constants.DefaultConsulPartition)
req := &pbresource.WriteRequest{Resource: resource}
_, err := resourceClient.Write(ctx, req)
_, err := testClient.ResourceClient.Write(ctx, req)
require.NoError(t, err)
}

Expand Down Expand Up @@ -648,13 +615,6 @@ func TestConsulResourceController_DoesNotCreateUnownedResource(t *testing.T) {
testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) {
c.Experiments = []string{"resource-apis"}
})
resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

unmanagedResource := trafficpermissions.Resource(constants.DefaultConsulNS, constants.DefaultConsulPartition)
unmanagedResource.Metadata = make(map[string]string) // Zero out the metadata
Expand All @@ -663,7 +623,7 @@ func TestConsulResourceController_DoesNotCreateUnownedResource(t *testing.T) {
// in Consul ourselves, without the metadata indicating it is owned by the controller.
{
req := &pbresource.WriteRequest{Resource: unmanagedResource}
_, err := resourceClient.Write(ctx, req)
_, err := testClient.ResourceClient.Write(ctx, req)
require.NoError(t, err)
}

Expand Down Expand Up @@ -694,7 +654,7 @@ func TestConsulResourceController_DoesNotCreateUnownedResource(t *testing.T) {

// Now check that the object in Consul is as expected.
req := &pbresource.ReadRequest{Id: trafficpermissions.ResourceID(constants.DefaultConsulNS, constants.DefaultConsulPartition)}
readResp, err := resourceClient.Read(ctx, req)
readResp, err := testClient.ResourceClient.Read(ctx, req)
require.NoError(t, err)
require.NotNil(t, readResp.GetResource())
opts := append([]cmp.Option{
Expand Down Expand Up @@ -756,13 +716,6 @@ func TestConsulResourceController_doesNotDeleteUnownedConfig(t *testing.T) {
testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) {
c.Experiments = []string{"resource-apis"}
})
resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

reconciler := &TrafficPermissionsController{
Client: fakeClient,
Expand All @@ -780,7 +733,7 @@ func TestConsulResourceController_doesNotDeleteUnownedConfig(t *testing.T) {
// in Consul ourselves, without the metadata indicating it is owned by the controller.
{
req := &pbresource.WriteRequest{Resource: unmanagedResource}
_, err := resourceClient.Write(ctx, req)
_, err := testClient.ResourceClient.Write(ctx, req)
require.NoError(t, err)
}

Expand All @@ -799,7 +752,7 @@ func TestConsulResourceController_doesNotDeleteUnownedConfig(t *testing.T) {

// Now check that the object in Consul is as expected.
req := &pbresource.ReadRequest{Id: trafficpermissionsWithDeletion.ResourceID(constants.DefaultConsulNS, constants.DefaultConsulPartition)}
readResp, err := resourceClient.Read(ctx, req)
readResp, err := testClient.ResourceClient.Read(ctx, req)
require.NoError(t, err)
require.NotNil(t, readResp.GetResource())
opts := append([]cmp.Option{
Expand Down
5 changes: 1 addition & 4 deletions control-plane/connect-inject/common/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/hashicorp/consul/proto-public/pbresource"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/helper/test"
"github.com/hashicorp/consul-k8s/control-plane/namespaces"
)
Expand Down Expand Up @@ -529,8 +528,6 @@ func Test_ConsulNamespaceIsNotFound_ErrorMsg(t *testing.T) {
testClient := test.TestServerWithMockConnMgrWatcher(t, func(c *testutil.TestServerConfig) {
c.Experiments = []string{"resource-apis"}
})
resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

id := &pbresource.ID{
Name: "foo",
Expand Down Expand Up @@ -566,7 +563,7 @@ func Test_ConsulNamespaceIsNotFound_ErrorMsg(t *testing.T) {
Data: data,
}

_, err = resourceClient.Write(context.Background(), &pbresource.WriteRequest{Resource: resource})
_, err := testClient.ResourceClient.Write(context.Background(), &pbresource.WriteRequest{Resource: resource})
require.Error(t, err)

s, ok := status.FromError(err)
Expand Down
28 changes: 11 additions & 17 deletions control-plane/connect-inject/controllers/endpointsv2/endpoints_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
"github.com/hashicorp/consul-k8s/control-plane/api/common"
inject "github.com/hashicorp/consul-k8s/control-plane/connect-inject/common"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/helper/test"
)

Expand Down Expand Up @@ -1754,9 +1753,6 @@ func TestEnsureService(t *testing.T) {
c.Experiments = []string{"resource-apis"}
})

resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
// Create the Endpoints controller.
Expand All @@ -1773,7 +1769,7 @@ func TestEnsureService(t *testing.T) {

// Set up test resourceReadWriter
rw := struct{ testReadWriter }{}
defaultRw := defaultResourceReadWriter{resourceClient}
defaultRw := defaultResourceReadWriter{testClient.ResourceClient}
rw.readFn = defaultRw.Read
rw.writeFn = defaultRw.Write
if tc.readFn != nil {
Expand All @@ -1797,34 +1793,34 @@ func TestEnsureService(t *testing.T) {
require.NoError(t, err)

// Get written resource before additional calls
beforeResource := getAndValidateResource(t, resourceClient, id)
beforeResource := getAndValidateResource(t, testClient.ResourceClient, id)

// Call a second time
err = ep.ensureService(context.Background(), &rw, tc.afterArgs.k8sUid, id, tc.afterArgs.meta, tc.afterArgs.consulSvc)
require.NoError(t, err)

// Check for change on second call to ensureService
if tc.expectWrite {
require.NotEqual(t, beforeResource.GetGeneration(), getAndValidateResource(t, resourceClient, id).GetGeneration(),
require.NotEqual(t, beforeResource.GetGeneration(), getAndValidateResource(t, testClient.ResourceClient, id).GetGeneration(),
"wanted different version for before and after resources following modification and reconcile")
} else {
require.Equal(t, beforeResource.GetGeneration(), getAndValidateResource(t, resourceClient, id).GetGeneration(),
require.Equal(t, beforeResource.GetGeneration(), getAndValidateResource(t, testClient.ResourceClient, id).GetGeneration(),
"wanted same version for before and after resources following repeat reconcile")
}

// Call several additional times
for i := 0; i < 5; i++ {
// Get written resource before each additional call
beforeResource = getAndValidateResource(t, resourceClient, id)
beforeResource = getAndValidateResource(t, testClient.ResourceClient, id)

err := ep.ensureService(context.Background(), &rw, tc.afterArgs.k8sUid, id, tc.afterArgs.meta, tc.afterArgs.consulSvc)
require.NoError(t, err)

if tc.expectAlwaysWrite {
require.NotEqual(t, beforeResource.GetGeneration(), getAndValidateResource(t, resourceClient, id).GetGeneration(),
require.NotEqual(t, beforeResource.GetGeneration(), getAndValidateResource(t, testClient.ResourceClient, id).GetGeneration(),
"wanted different version for before and after resources following modification and reconcile")
} else {
require.Equal(t, beforeResource.GetGeneration(), getAndValidateResource(t, resourceClient, id).GetGeneration(),
require.Equal(t, beforeResource.GetGeneration(), getAndValidateResource(t, testClient.ResourceClient, id).GetGeneration(),
"wanted same version for before and after resources following repeat reconcile")
}
}
Expand Down Expand Up @@ -2206,8 +2202,6 @@ func runReconcileCase(t *testing.T, tc reconcileCase) {
DenyK8sNamespacesSet: mapset.NewSetWith(),
},
}
resourceClient, err := consul.NewResourceServiceClient(ep.ConsulServerConnMgr)
require.NoError(t, err)

// Default ns and partition if not specified in test.
if tc.targetConsulNs == "" {
Expand All @@ -2220,9 +2214,9 @@ func runReconcileCase(t *testing.T, tc reconcileCase) {
// If existing resource specified, create it and ensure it exists.
if tc.existingResource != nil {
writeReq := &pbresource.WriteRequest{Resource: tc.existingResource}
_, err = resourceClient.Write(context.Background(), writeReq)
_, err := testClient.ResourceClient.Write(context.Background(), writeReq)
require.NoError(t, err)
test.ResourceHasPersisted(t, context.Background(), resourceClient, tc.existingResource.Id)
test.ResourceHasPersisted(t, context.Background(), testClient.ResourceClient, tc.existingResource.Id)
}

// Run actual reconcile and verify results.
Expand All @@ -2239,10 +2233,10 @@ func runReconcileCase(t *testing.T, tc reconcileCase) {
}
require.False(t, resp.Requeue)

expectedServiceMatches(t, resourceClient, tc.svcName, tc.targetConsulNs, tc.targetConsulPartition, tc.expectedResource)
expectedServiceMatches(t, testClient.ResourceClient, tc.svcName, tc.targetConsulNs, tc.targetConsulPartition, tc.expectedResource)

if tc.caseFn != nil {
tc.caseFn(t, &tc, ep, resourceClient)
tc.caseFn(t, &tc, ep, testClient.ResourceClient)
}
}

Expand Down
26 changes: 8 additions & 18 deletions control-plane/connect-inject/controllers/pod/pod_controller_ent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ package pod
import (
"context"
"testing"
"time"

mapset "github.com/deckarep/golang-set"
logrtest "github.com/go-logr/logr/testr"
Expand All @@ -27,7 +26,6 @@ import (
"github.com/hashicorp/consul-k8s/control-plane/api/common"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/metrics"
"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/helper/test"
)

Expand Down Expand Up @@ -672,14 +670,6 @@ func runControllerTest(t *testing.T, tc testCase) {
}
})

resourceClient, err := consul.NewResourceServiceClient(testClient.Watcher)
require.NoError(t, err)

require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
return err == nil
}, 5*time.Second, 500*time.Millisecond)

// Create the partition in Consul.
if tc.partition != "" {
testClient.Cfg.APIClientConfig.Partition = tc.partition
Expand Down Expand Up @@ -740,10 +730,10 @@ func runControllerTest(t *testing.T, tc testCase) {
}

workloadID := getWorkloadID(tc.podName, tc.expectedConsulNamespace, tc.partition)
loadResource(t, context.Background(), resourceClient, workloadID, tc.existingWorkload, nil)
loadResource(t, context.Background(), resourceClient, getHealthStatusID(tc.podName, tc.expectedConsulNamespace, tc.partition), tc.existingHealthStatus, workloadID)
loadResource(t, context.Background(), resourceClient, getProxyConfigurationID(tc.podName, tc.expectedConsulNamespace, tc.partition), tc.existingProxyConfiguration, nil)
loadResource(t, context.Background(), resourceClient, getDestinationsID(tc.podName, tc.expectedConsulNamespace, tc.partition), tc.existingDestinations, nil)
loadResource(t, context.Background(), testClient.ResourceClient, workloadID, tc.existingWorkload, nil)
loadResource(t, context.Background(), testClient.ResourceClient, getHealthStatusID(tc.podName, tc.expectedConsulNamespace, tc.partition), tc.existingHealthStatus, workloadID)
loadResource(t, context.Background(), testClient.ResourceClient, getProxyConfigurationID(tc.podName, tc.expectedConsulNamespace, tc.partition), tc.existingProxyConfiguration, nil)
loadResource(t, context.Background(), testClient.ResourceClient, getDestinationsID(tc.podName, tc.expectedConsulNamespace, tc.partition), tc.existingDestinations, nil)

namespacedName := types.NamespacedName{
Namespace: podNamespace,
Expand All @@ -762,14 +752,14 @@ func runControllerTest(t *testing.T, tc testCase) {
require.Equal(t, tc.expRequeue, resp.Requeue)

wID := getWorkloadID(tc.podName, tc.expectedConsulNamespace, tc.partition)
expectedWorkloadMatches(t, context.Background(), resourceClient, wID, tc.expectedWorkload)
expectedWorkloadMatches(t, context.Background(), testClient.ResourceClient, wID, tc.expectedWorkload)

hsID := getHealthStatusID(tc.podName, tc.expectedConsulNamespace, tc.partition)
expectedHealthStatusMatches(t, context.Background(), resourceClient, hsID, tc.expectedHealthStatus)
expectedHealthStatusMatches(t, context.Background(), testClient.ResourceClient, hsID, tc.expectedHealthStatus)

pcID := getProxyConfigurationID(tc.podName, tc.expectedConsulNamespace, tc.partition)
expectedProxyConfigurationMatches(t, context.Background(), resourceClient, pcID, tc.expectedProxyConfiguration)
expectedProxyConfigurationMatches(t, context.Background(), testClient.ResourceClient, pcID, tc.expectedProxyConfiguration)

uID := getDestinationsID(tc.podName, metav1.NamespaceDefault, constants.DefaultConsulPartition)
expectedDestinationMatches(t, context.Background(), resourceClient, uID, tc.expectedDestinations)
expectedDestinationMatches(t, context.Background(), testClient.ResourceClient, uID, tc.expectedDestinations)
}
Loading

0 comments on commit 0a2eedd

Please sign in to comment.