🌱 fix flaky unit-tests (#1459)

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)
 

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,

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())
 

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())
 

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())

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)

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{}

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,26 +420,29 @@ 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))
 	})
 })
 
 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))