From 96820fac801dd39781d66ff683d1bf3a2073d25c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=BCttler?= Date: Thu, 29 Aug 2024 11:17:21 +0200 Subject: [PATCH] :seedling: fix flaky unit-tests (#1459) --- controllers/controllers_suite_test.go | 9 ++-- controllers/hcloudmachine_controller_test.go | 23 +++++--- .../hcloudremediation_controller_test.go | 6 +++ ...nerbaremetalremediation_controller_test.go | 2 +- controllers/hetznercluster_controller_test.go | 11 ++++ pkg/services/hcloud/client/client.go | 10 ++-- .../hcloud/client/fake/hcloud_client.go | 5 +- .../hcloud/client/fake/hcloud_client_test.go | 52 +++++++++---------- pkg/services/hcloud/client/mocks/Client.go | 26 ++++++++-- test/helpers/envtest.go | 5 ++ 10 files changed, 96 insertions(+), 53 deletions(-) diff --git a/controllers/controllers_suite_test.go b/controllers/controllers_suite_test.go index c3459c155..142a20d5d 100644 --- a/controllers/controllers_suite_test.go +++ b/controllers/controllers_suite_test.go @@ -34,7 +34,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller" infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1" - hcloudclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client" "github.com/syself/cluster-api-provider-hetzner/test/helpers" ) @@ -45,10 +44,9 @@ const ( ) var ( - testEnv *helpers.TestEnvironment - hcloudClient hcloudclient.Client - ctx = ctrl.SetupSignalHandler() - wg sync.WaitGroup + testEnv *helpers.TestEnvironment + ctx = ctrl.SetupSignalHandler() + wg sync.WaitGroup defaultPlacementGroupName = "caph-placement-group" defaultFailureDomain = "fsn1" @@ -64,7 +62,6 @@ var _ = BeforeSuite(func() { utilruntime.Must(clusterv1.AddToScheme(scheme.Scheme)) testEnv = helpers.NewTestEnvironment() - hcloudClient = testEnv.HCloudClientFactory.NewClient("") wg.Add(1) diff --git a/controllers/hcloudmachine_controller_test.go b/controllers/hcloudmachine_controller_test.go index a6450e2fe..c51c7e288 100644 --- a/controllers/hcloudmachine_controller_test.go +++ b/controllers/hcloudmachine_controller_test.go @@ -17,9 +17,7 @@ limitations under the License. package controllers import ( - "fmt" "testing" - "time" "github.com/hetznercloud/hcloud-go/v2/hcloud" . "github.com/onsi/ginkgo/v2" @@ -36,6 +34,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1" + hcloudclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client" "github.com/syself/cluster-api-provider-hetzner/pkg/utils" ) @@ -320,7 +319,10 @@ var _ = Describe("HCloudMachineReconciler", func() { Context("Basic test", func() { Context("correct server", func() { + var hcloudClient hcloudclient.Client + BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() // remove bootstrap infos capiMachine.Spec.Bootstrap = clusterv1.Bootstrap{} Expect(testEnv.Create(ctx, capiMachine)).To(Succeed()) @@ -365,26 +367,23 @@ var _ = Describe("HCloudMachineReconciler", func() { }, timeout).Should(BeTrue()) }) - It("creates the HCloud machine in Hetzner 1 (flaky)", func() { + It("creates the HCloud machine in Hetzner 1", func() { By("checking that no servers exist") - Eventually(func(start time.Time) bool { + Eventually(func() bool { servers, err := hcloudClient.ListServers(ctx, hcloud.ServerListOpts{ ListOpts: hcloud.ListOpts{ LabelSelector: utils.LabelsToLabelSelector(map[string]string{hetznerCluster.ClusterTagKey(): "owned"}), }, }) if err != nil { - fmt.Printf("flaky test. ListServers failed: %s\n", err.Error()) return false } if len(servers) != 0 { - fmt.Printf("flaky test. There are still servers: %+v\n", servers) return false } - fmt.Printf("flaky test. OK after %s\n", time.Since(start).String()) return true - }, 2*timeout, interval).WithArguments(time.Now()).Should(BeTrue()) + }, timeout, interval).Should(BeTrue()) By("checking that bootstrap condition is not ready") @@ -517,7 +516,10 @@ var _ = Describe("HCloudMachineReconciler", func() { }) Context("without network", func() { + var hcloudClient hcloudclient.Client + BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() hetznerCluster.Spec.HCloudNetwork.Enabled = false Expect(testEnv.Create(ctx, hetznerCluster)).To(Succeed()) Expect(testEnv.Create(ctx, hcloudMachine)).To(Succeed()) @@ -543,7 +545,10 @@ var _ = Describe("HCloudMachineReconciler", func() { }) Context("without placement groups", func() { + var hcloudClient hcloudclient.Client + BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() hetznerCluster.Spec.HCloudPlacementGroups = nil Expect(testEnv.Create(ctx, hetznerCluster)).To(Succeed()) @@ -609,7 +614,9 @@ var _ = Describe("HCloudMachineReconciler", func() { }) Context("with public network specs", func() { + var hcloudClient hcloudclient.Client BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() hcloudMachine.Spec.PublicNetwork = &infrav1.PublicNetworkSpec{ EnableIPv4: false, EnableIPv6: false, diff --git a/controllers/hcloudremediation_controller_test.go b/controllers/hcloudremediation_controller_test.go index c1910e11d..7fee59861 100644 --- a/controllers/hcloudremediation_controller_test.go +++ b/controllers/hcloudremediation_controller_test.go @@ -30,6 +30,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1" + hcloudclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client" hcloudutil "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/util" "github.com/syself/cluster-api-provider-hetzner/pkg/utils" ) @@ -186,6 +187,11 @@ var _ = Describe("HCloudRemediationReconciler", func() { }) Context("Basic test", func() { + var hcloudClient hcloudclient.Client + BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() + }) + It("creates the hcloudRemediation successfully", func() { Expect(testEnv.Create(ctx, hcloudRemediation)).To(Succeed()) diff --git a/controllers/hetznerbaremetalremediation_controller_test.go b/controllers/hetznerbaremetalremediation_controller_test.go index 633be58bc..7950cb4cc 100644 --- a/controllers/hetznerbaremetalremediation_controller_test.go +++ b/controllers/hetznerbaremetalremediation_controller_test.go @@ -257,7 +257,7 @@ var _ = Describe("HetznerBareMetalRemediationReconciler", func() { Expect(testEnv.Cleanup(ctx, hetznerBareMetalRemediation, hetznerBaremetalMachine)).To(Succeed()) }) - It("should not remediate if no annotations is present in the hetznerBaremetalMachine (flaky)", func() { + It("should not remediate if no annotations is present in the hetznerBaremetalMachine", func() { Expect(testEnv.Create(ctx, hetznerBaremetalMachine)).To(Succeed()) Expect(testEnv.Create(ctx, hetznerBareMetalRemediation)).To(Succeed()) diff --git a/controllers/hetznercluster_controller_test.go b/controllers/hetznercluster_controller_test.go index 1a1cce888..c60a70537 100644 --- a/controllers/hetznercluster_controller_test.go +++ b/controllers/hetznercluster_controller_test.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/event" infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1" + hcloudclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client" "github.com/syself/cluster-api-provider-hetzner/pkg/utils" "github.com/syself/cluster-api-provider-hetzner/test/helpers" ) @@ -333,6 +334,12 @@ var _ = Describe("Hetzner ClusterReconciler", func() { }) Context("load balancer", func() { + var hcloudClient hcloudclient.Client + + BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() + }) + It("should create load balancer and update it accordingly", func() { Expect(testEnv.Create(ctx, instance)).To(Succeed()) @@ -735,8 +742,10 @@ var _ = Describe("Hetzner ClusterReconciler", func() { Context("HetznerMachines belonging to the cluster", func() { var bootstrapSecret *corev1.Secret + var hcloudClient hcloudclient.Client BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() bootstrapSecret = getDefaultBootstrapSecret(namespace) Expect(testEnv.Create(ctx, bootstrapSecret)).To(Succeed()) }) @@ -773,8 +782,10 @@ var _ = Describe("Hetzner ClusterReconciler", func() { Context("Placement groups", func() { var bootstrapSecret *corev1.Secret + var hcloudClient hcloudclient.Client BeforeEach(func() { + hcloudClient = testEnv.ResetAndGetGlobalHCloudClient() // Create the bootstrap secret bootstrapSecret = getDefaultBootstrapSecret(namespace) Expect(testEnv.Create(ctx, bootstrapSecret)).To(Succeed()) diff --git a/pkg/services/hcloud/client/client.go b/pkg/services/hcloud/client/client.go index fa04d485b..10c1fb127 100644 --- a/pkg/services/hcloud/client/client.go +++ b/pkg/services/hcloud/client/client.go @@ -40,7 +40,8 @@ var ErrUnauthorized = fmt.Errorf("unauthorized") // Client collects all methods used by the controller in the hcloud cloud API. type Client interface { - Close() + // Reset resets the local cache. Only implemented in the fake client. + Reset() Client CreateLoadBalancer(context.Context, hcloud.LoadBalancerCreateOpts) (*hcloud.LoadBalancer, error) DeleteLoadBalancer(context.Context, int64) error @@ -78,6 +79,7 @@ type Client interface { // Factory is the interface for creating new Client objects. type Factory interface { + // NewClient returns a new Client in the real implementation, and the shared global Client in the fake implementation. NewClient(hcloudToken string) Client } @@ -141,8 +143,10 @@ type realClient struct { client *hcloud.Client } -// Close implements the Close method of the HCloudClient interface. -func (c *realClient) Close() {} +// Reset implements the Reset method of the HCloudClient interface. +func (c *realClient) Reset() Client { + return c +} func (c *realClient) CreateLoadBalancer(ctx context.Context, opts hcloud.LoadBalancerCreateOpts) (*hcloud.LoadBalancer, error) { res, _, err := c.client.LoadBalancer.Create(ctx, opts) diff --git a/pkg/services/hcloud/client/fake/hcloud_client.go b/pkg/services/hcloud/client/fake/hcloud_client.go index 54229b171..62eddf8af 100644 --- a/pkg/services/hcloud/client/fake/hcloud_client.go +++ b/pkg/services/hcloud/client/fake/hcloud_client.go @@ -55,8 +55,8 @@ func (f *cacheHCloudClientFactory) NewClient(string) hcloudclient.Client { return cacheHCloudClientInstance } -// Close implements Close method of hcloud client interface. -func (c *cacheHCloudClient) Close() { +// Reset implements Reset method of hcloud client interface. +func (c *cacheHCloudClient) Reset() hcloudclient.Client { c.counterMutex.Lock() defer c.counterMutex.Unlock() @@ -86,6 +86,7 @@ func (c *cacheHCloudClient) Close() { cacheHCloudClientInstance.placementGroupIDCounter = 0 cacheHCloudClientInstance.loadBalancerIDCounter = 0 cacheHCloudClientInstance.networkIDCounter = 0 + return c } type cacheHCloudClientFactory struct{} diff --git a/pkg/services/hcloud/client/fake/hcloud_client_test.go b/pkg/services/hcloud/client/fake/hcloud_client_test.go index f9e13c157..550585992 100644 --- a/pkg/services/hcloud/client/fake/hcloud_client_test.go +++ b/pkg/services/hcloud/client/fake/hcloud_client_test.go @@ -25,6 +25,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + hcloudclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client" "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client/fake" ) @@ -75,12 +76,13 @@ var _ = Describe("Load balancer", func() { Subnets: []hcloud.NetworkSubnet{{IPRange: &net.IPNet{IP: net.IP("1.2.3.6")}}}, } - client := factory.NewClient("") + var client hcloudclient.Client var lb *hcloud.LoadBalancer var network *hcloud.Network BeforeEach(func() { + client = factory.NewClient("").Reset() _, err := client.CreateLoadBalancer(ctx, opts) Expect(err).To(Succeed()) @@ -92,9 +94,6 @@ var _ = Describe("Load balancer", func() { network, err = client.CreateNetwork(ctx, networkOpts) Expect(err).To(Succeed()) }) - AfterEach(func() { - client.Close() - }) It("creates a load balancer and returns it with an ID", func() { Expect(lb.ID).ToNot(Equal(0)) @@ -421,18 +420,21 @@ var _ = Describe("Load balancer", func() { err := client.DeleteServiceFromLoadBalancer(ctx, lb, 999) Expect(err).ToNot(Succeed()) }) +}) - _ = Describe("Images", func() { - var listOpts hcloud.ImageListOpts - client := factory.NewClient("") - BeforeEach(func() { - listOpts.LabelSelector = "caph-image-name==fedora-control-plane" - }) - It("lists at least one image", func() { - resp, err := client.ListImages(ctx, listOpts) - Expect(err).To(Succeed()) - Expect(len(resp)).To(BeNumerically(">", 0)) - }) +var _ = Describe("Images", func() { + var listOpts hcloud.ImageListOpts + var client hcloudclient.Client + + BeforeEach(func() { + client = factory.NewClient("").Reset() + + listOpts.LabelSelector = "caph-image-name==fedora-control-plane" + }) + It("lists at least one image", func() { + resp, err := client.ListImages(ctx, listOpts) + Expect(err).To(Succeed()) + Expect(len(resp)).To(BeNumerically(">", 0)) }) }) @@ -440,7 +442,7 @@ var _ = Describe("Server", func() { var listOpts hcloud.ServerListOpts listOpts.LabelSelector = labelSelector - client := factory.NewClient("") + var client hcloudclient.Client opts := hcloud.ServerCreateOpts{ Name: "test-server", @@ -464,6 +466,8 @@ var _ = Describe("Server", func() { BeforeEach(func() { var err error + client = factory.NewClient("").Reset() + server, err = client.CreateServer(ctx, opts) Expect(err).To(Succeed()) @@ -478,9 +482,6 @@ var _ = Describe("Server", func() { }) Expect(err).To(Succeed()) }) - AfterEach(func() { - client.Close() - }) It("creates a server with an ID", func() { Expect(server.ID).ToNot(Equal(0)) @@ -609,7 +610,7 @@ var _ = Describe("Network", func() { var listOpts hcloud.NetworkListOpts listOpts.LabelSelector = labelSelector - client := factory.NewClient("") + var client hcloudclient.Client opts := hcloud.NetworkCreateOpts{ Name: "test-network", @@ -625,12 +626,10 @@ var _ = Describe("Network", func() { BeforeEach(func() { var err error + client = factory.NewClient("").Reset() network, err = client.CreateNetwork(ctx, opts) Expect(err).To(Succeed()) }) - AfterEach(func() { - client.Close() - }) It("creates a network with an ID", func() { Expect(network.ID).ToNot(Equal(0)) @@ -675,13 +674,13 @@ var _ = Describe("Placement groups", func() { Type: "stream", } - client := factory.NewClient("") - + var client hcloudclient.Client var server *hcloud.Server var placementGroup *hcloud.PlacementGroup BeforeEach(func() { var err error + client = factory.NewClient("").Reset() server, err = client.CreateServer(ctx, hcloud.ServerCreateOpts{ Name: "test-server", Labels: map[string]string{ @@ -703,9 +702,6 @@ var _ = Describe("Placement groups", func() { placementGroup, err = client.CreatePlacementGroup(ctx, opts) Expect(err).To(Succeed()) }) - AfterEach(func() { - client.Close() - }) It("creates a placementGroup with an ID", func() { Expect(placementGroup.ID).ToNot(Equal(0)) diff --git a/pkg/services/hcloud/client/mocks/Client.go b/pkg/services/hcloud/client/mocks/Client.go index fd5614345..5b321eb45 100644 --- a/pkg/services/hcloud/client/mocks/Client.go +++ b/pkg/services/hcloud/client/mocks/Client.go @@ -6,6 +6,7 @@ import ( context "context" hcloud "github.com/hetznercloud/hcloud-go/v2/hcloud" + hcloudclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client" mock "github.com/stretchr/testify/mock" @@ -161,11 +162,6 @@ func (_m *Client) ChangeLoadBalancerType(_a0 context.Context, _a1 *hcloud.LoadBa return r0 } -// Close provides a mock function with given fields: -func (_m *Client) Close() { - _m.Called() -} - // CreateLoadBalancer provides a mock function with given fields: _a0, _a1 func (_m *Client) CreateLoadBalancer(_a0 context.Context, _a1 hcloud.LoadBalancerCreateOpts) (*hcloud.LoadBalancer, error) { ret := _m.Called(_a0, _a1) @@ -718,6 +714,26 @@ func (_m *Client) RebootServer(_a0 context.Context, _a1 *hcloud.Server) error { return r0 } +// Reset provides a mock function with given fields: +func (_m *Client) Reset() hcloudclient.Client { + ret := _m.Called() + + if len(ret) == 0 { + panic("no return value specified for Reset") + } + + var r0 hcloudclient.Client + if rf, ok := ret.Get(0).(func() hcloudclient.Client); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(hcloudclient.Client) + } + } + + return r0 +} + // ShutdownServer provides a mock function with given fields: _a0, _a1 func (_m *Client) ShutdownServer(_a0 context.Context, _a1 *hcloud.Server) error { ret := _m.Called(_a0, _a1) diff --git a/test/helpers/envtest.go b/test/helpers/envtest.go index d6c22587c..804da7cd6 100644 --- a/test/helpers/envtest.go +++ b/test/helpers/envtest.go @@ -257,6 +257,11 @@ func (t *TestEnvironment) CreateKubeconfigSecret(ctx context.Context, cluster *c return t.Create(ctx, kubeconfig.GenerateSecret(cluster, kubeconfig.FromEnvTestConfig(t.Config, cluster))) } +// ResetAndGetGlobalHCloudClient resets the cache of the fake Client and returns it. +func (t *TestEnvironment) ResetAndGetGlobalHCloudClient() hcloudclient.Client { + return t.HCloudClientFactory.NewClient("").Reset() +} + func getFilePathToCAPICRDs(root string) string { mod, err := newMod(filepath.Join(root, "go.mod")) if err != nil {