Skip to content

Commit 3694e40

Browse files
fernando-villalbafernando-villalba
committed
refactor(controller): adhere to go idioms and fix regression in tests
1 parent 8a14d5f commit 3694e40

File tree

1 file changed

+84
-45
lines changed

1 file changed

+84
-45
lines changed

‎pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go‎

Lines changed: 84 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,15 @@ import (
2626
)
2727

2828
// setupFixtures helper returns a fresh set of test objects to ensure isolation between test functions.
29-
func setupFixtures() (
29+
func setupFixtures(tb testing.TB) (
3030
*multigresv1alpha1.CoreTemplate,
3131
*multigresv1alpha1.CellTemplate,
3232
*multigresv1alpha1.ShardTemplate,
3333
*multigresv1alpha1.MultigresCluster,
3434
string, string, string,
3535
) {
36+
tb.Helper()
37+
3638
clusterName := "test-cluster"
3739
namespace := "default"
3840
finalizerName := "multigres.com/finalizer"
@@ -131,7 +133,7 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
131133
_ = appsv1.AddToScheme(scheme)
132134
_ = corev1.AddToScheme(scheme)
133135

134-
coreTpl, cellTpl, shardTpl, baseCluster, clusterName, namespace, finalizerName := setupFixtures()
136+
coreTpl, cellTpl, shardTpl, baseCluster, clusterName, namespace, finalizerName := setupFixtures(t)
135137

136138
tests := map[string]struct {
137139
multigrescluster *multigresv1alpha1.MultigresCluster
@@ -157,7 +159,6 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
157159
},
158160
},
159161
"Create: Full Cluster Creation with Templates": {
160-
// Using defaults
161162
validate: func(t testing.TB, c client.Client) {
162163
ctx := t.Context()
163164
updatedCluster := &multigresv1alpha1.MultigresCluster{}
@@ -244,7 +245,6 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
244245
c.Spec.MultiAdmin = multigresv1alpha1.MultiAdminConfig{TemplateRef: "default-core"}
245246
c.Spec.TemplateDefaults.CoreTemplate = ""
246247
},
247-
// Using defaults
248248
validate: func(t testing.TB, c client.Client) {
249249
ctx := t.Context()
250250
deploy := &appsv1.Deployment{}
@@ -308,7 +308,6 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
308308
preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
309309
c.Spec.Images.ImagePullSecrets = []corev1.LocalObjectReference{{Name: "my-secret"}}
310310
},
311-
// Using defaults
312311
validate: func(t testing.TB, c client.Client) {
313312
ctx := t.Context()
314313
deploy := &appsv1.Deployment{}
@@ -329,7 +328,6 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
329328
}
330329
c.Spec.TemplateDefaults.CoreTemplate = ""
331330
},
332-
// Using defaults
333331
validate: func(t testing.TB, c client.Client) {
334332
ctx := t.Context()
335333
ts := &multigresv1alpha1.TopoServer{}
@@ -506,6 +504,14 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
506504
objects = []client.Object{coreTpl, cellTpl, shardTpl}
507505
}
508506

507+
clientBuilder := fake.NewClientBuilder().
508+
WithScheme(scheme).
509+
WithObjects(objects...).
510+
WithStatusSubresource(&multigresv1alpha1.MultigresCluster{}, &multigresv1alpha1.Cell{}, &multigresv1alpha1.TableGroup{})
511+
baseClient := clientBuilder.Build()
512+
513+
finalClient := baseClient
514+
509515
// Apply defaults if no specific cluster is provided
510516
cluster := tc.multigrescluster
511517
if cluster == nil {
@@ -517,21 +523,39 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
517523
tc.preReconcileUpdate(t, cluster)
518524
}
519525

520-
// Inject cluster into existingObjects if creation is not skipped
521-
// This handles initializing the fake client with the cluster state,
522-
// including DeletionTimestamp if set by preReconcileUpdate.
523-
if !tc.skipClusterCreation {
524-
objects = append(objects, cluster)
526+
// Capture if deletion is intended BEFORE calling Create,
527+
// because Create strips DeletionTimestamp from the local struct in some client implementations (or API logic mimics it).
528+
shouldDelete := false
529+
if cluster.GetDeletionTimestamp() != nil && !cluster.GetDeletionTimestamp().IsZero() {
530+
shouldDelete = true
525531
}
526532

527-
clientBuilder := fake.NewClientBuilder().
528-
WithScheme(scheme).
529-
WithObjects(objects...).
530-
WithStatusSubresource(&multigresv1alpha1.MultigresCluster{}, &multigresv1alpha1.Cell{}, &multigresv1alpha1.TableGroup{})
531-
baseClient := clientBuilder.Build()
533+
if !strings.Contains(name, "Object Not Found") {
534+
check := &multigresv1alpha1.MultigresCluster{}
535+
err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, check)
536+
if apierrors.IsNotFound(err) {
537+
if err := baseClient.Create(t.Context(), cluster); err != nil {
538+
t.Fatalf("failed to create initial cluster: %v", err)
539+
}
532540

533-
var finalClient client.Client
534-
finalClient = baseClient
541+
// Ensure DeletionTimestamp is set in the API if the test requires it.
542+
// client.Create strips this field, so we must invoke Delete() to re-apply it.
543+
if shouldDelete {
544+
// 1. Refresh object to avoid ResourceVersion conflict
545+
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
546+
t.Fatalf("failed to refresh cluster before delete: %v", err)
547+
}
548+
// 2. Delete it
549+
if err := baseClient.Delete(t.Context(), cluster); err != nil {
550+
t.Fatalf("failed to set deletion timestamp: %v", err)
551+
}
552+
// 3. Refresh again to ensure the controller sees the deletion timestamp
553+
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
554+
t.Fatalf("failed to refresh cluster after deletion: %v", err)
555+
}
556+
}
557+
}
558+
}
535559

536560
reconciler := &MultigresClusterReconciler{
537561
Client: finalClient,
@@ -565,7 +589,7 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
565589
_ = appsv1.AddToScheme(scheme)
566590
_ = corev1.AddToScheme(scheme)
567591

568-
coreTpl, cellTpl, shardTpl, baseCluster, clusterName, namespace, finalizerName := setupFixtures()
592+
coreTpl, cellTpl, shardTpl, baseCluster, clusterName, namespace, finalizerName := setupFixtures(t)
569593
errBoom := errors.New("boom")
570594

571595
tests := map[string]struct {
@@ -714,19 +738,15 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
714738
failureConfig: &testutil.FailureConfig{OnGet: testutil.FailOnKeyName("admin-core-fail", errBoom)},
715739
},
716740
"Error: Create GlobalTopo Failed": {
717-
// Using defaults
718741
failureConfig: &testutil.FailureConfig{OnCreate: testutil.FailOnObjectName(clusterName+"-global-topo", errBoom)},
719742
},
720743
"Error: Create MultiAdmin Failed": {
721-
// Using defaults
722744
failureConfig: &testutil.FailureConfig{OnCreate: testutil.FailOnObjectName(clusterName+"-multiadmin", errBoom)},
723745
},
724746
"Error: Resolve CellTemplate Failed": {
725-
// Using defaults
726747
failureConfig: &testutil.FailureConfig{OnGet: testutil.FailOnKeyName("default-cell", errBoom)},
727748
},
728749
"Error: List Existing Cells Failed (Reconcile Loop)": {
729-
// Using defaults
730750
failureConfig: &testutil.FailureConfig{
731751
OnList: func(list client.ObjectList) error {
732752
if _, ok := list.(*multigresv1alpha1.CellList); ok {
@@ -737,7 +757,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
737757
},
738758
},
739759
"Error: Create Cell Failed": {
740-
// Using defaults
741760
failureConfig: &testutil.FailureConfig{OnCreate: testutil.FailOnObjectName(clusterName+"-zone-a", errBoom)},
742761
},
743762
"Error: Prune Cell Failed": {
@@ -748,7 +767,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
748767
failureConfig: &testutil.FailureConfig{OnDelete: testutil.FailOnObjectName(clusterName+"-zone-b", errBoom)},
749768
},
750769
"Error: List Existing TableGroups Failed": {
751-
// Using defaults
752770
failureConfig: &testutil.FailureConfig{
753771
OnList: func(list client.ObjectList) error {
754772
if _, ok := list.(*multigresv1alpha1.TableGroupList); ok {
@@ -759,11 +777,9 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
759777
},
760778
},
761779
"Error: Resolve ShardTemplate Failed": {
762-
// Using defaults
763780
failureConfig: &testutil.FailureConfig{OnGet: testutil.FailOnKeyName("default-shard", errBoom)},
764781
},
765782
"Error: Create TableGroup Failed": {
766-
// Using defaults
767783
failureConfig: &testutil.FailureConfig{OnCreate: testutil.FailOnObjectName(clusterName+"-db1-tg1", errBoom)},
768784
},
769785
"Error: Prune TableGroup Failed": {
@@ -774,7 +790,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
774790
failureConfig: &testutil.FailureConfig{OnDelete: testutil.FailOnObjectName(clusterName+"-orphan-tg", errBoom)},
775791
},
776792
"Error: UpdateStatus (List Cells Failed)": {
777-
// Using defaults
778793
failureConfig: &testutil.FailureConfig{
779794
OnList: func() func(client.ObjectList) error {
780795
count := 0
@@ -791,7 +806,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
791806
},
792807
},
793808
"Error: UpdateStatus (List TableGroups Failed)": {
794-
// Using defaults
795809
failureConfig: &testutil.FailureConfig{
796810
OnList: func() func(client.ObjectList) error {
797811
count := 0
@@ -808,7 +822,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
808822
},
809823
},
810824
"Error: Update Status Failed (API Error)": {
811-
// Using defaults
812825
failureConfig: &testutil.FailureConfig{OnStatusUpdate: testutil.FailOnObjectName(clusterName, errBoom)},
813826
},
814827
"Error: Global Topo Resolution Failed (During Cell Reconcile)": {
@@ -880,7 +893,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
880893
c.Spec.Databases[0].Name = longName
881894
c.Spec.Databases[0].TableGroups[0].Name = longName
882895
},
883-
// Using defaults
884896
},
885897
}
886898

@@ -894,6 +906,18 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
894906
objects = []client.Object{coreTpl, cellTpl, shardTpl}
895907
}
896908

909+
clientBuilder := fake.NewClientBuilder().
910+
WithScheme(scheme).
911+
WithObjects(objects...).
912+
WithStatusSubresource(&multigresv1alpha1.MultigresCluster{}, &multigresv1alpha1.Cell{}, &multigresv1alpha1.TableGroup{})
913+
baseClient := clientBuilder.Build()
914+
915+
var finalClient client.Client
916+
finalClient = client.Client(baseClient)
917+
if tc.failureConfig != nil {
918+
finalClient = testutil.NewFakeClientWithFailures(baseClient, tc.failureConfig)
919+
}
920+
897921
// Apply defaults if no specific cluster is provided
898922
cluster := tc.multigrescluster
899923
if cluster == nil {
@@ -905,23 +929,38 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
905929
tc.preReconcileUpdate(t, cluster)
906930
}
907931

908-
// Inject cluster into existingObjects if creation is not skipped
909-
// This handles initializing the fake client with the cluster state,
910-
// including DeletionTimestamp if set by preReconcileUpdate.
911-
if !tc.skipClusterCreation {
912-
objects = append(objects, cluster)
932+
// Capture if deletion is intended BEFORE calling Create,
933+
// because Create strips DeletionTimestamp from the local struct in some client implementations (or API logic mimics it).
934+
shouldDelete := false
935+
if cluster.GetDeletionTimestamp() != nil && !cluster.GetDeletionTimestamp().IsZero() {
936+
shouldDelete = true
913937
}
914938

915-
clientBuilder := fake.NewClientBuilder().
916-
WithScheme(scheme).
917-
WithObjects(objects...).
918-
WithStatusSubresource(&multigresv1alpha1.MultigresCluster{}, &multigresv1alpha1.Cell{}, &multigresv1alpha1.TableGroup{})
919-
baseClient := clientBuilder.Build()
939+
if !strings.Contains(name, "Object Not Found") {
940+
check := &multigresv1alpha1.MultigresCluster{}
941+
err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, check)
942+
if apierrors.IsNotFound(err) {
943+
if err := baseClient.Create(t.Context(), cluster); err != nil {
944+
t.Fatalf("failed to create initial cluster: %v", err)
945+
}
920946

921-
var finalClient client.Client
922-
finalClient = baseClient
923-
if tc.failureConfig != nil {
924-
finalClient = testutil.NewFakeClientWithFailures(baseClient, tc.failureConfig)
947+
// Ensure DeletionTimestamp is set in the API if the test requires it.
948+
// client.Create strips this field, so we must invoke Delete() to re-apply it.
949+
if shouldDelete {
950+
// 1. Refresh object to avoid ResourceVersion conflict
951+
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
952+
t.Fatalf("failed to refresh cluster before delete: %v", err)
953+
}
954+
// 2. Delete it
955+
if err := baseClient.Delete(t.Context(), cluster); err != nil {
956+
t.Fatalf("failed to set deletion timestamp: %v", err)
957+
}
958+
// 3. Refresh again to ensure the controller sees the deletion timestamp
959+
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
960+
t.Fatalf("failed to refresh cluster after deletion: %v", err)
961+
}
962+
}
963+
}
925964
}
926965

927966
reconciler := &MultigresClusterReconciler{

0 commit comments

Comments
 (0)