Skip to content

Commit 2562d8c

Browse files
fernando-villalbafernando-villalba
committed
refactor(controller): use explicit flag for skipping cluster creation in tests
- pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go: - Introduce `skipClusterCreation` boolean field to the test table struct to explicitly control whether the initial `MultigresCluster` object should be created. - Update the test runner to inject the cluster object into `existingObjects` before building the fake client, rather than manually calling `client.Create` inside the loop. This simplifies the runner and ensures correct state initialization (including `DeletionTimestamp` handling) via the client builder. - Remove brittle logic that relied on matching the test case name string ("Object Not Found") to skip creation. - Update "Object Not Found (Clean Exit)" test case to use the new flag.
1 parent bb9d2b8 commit 2562d8c

File tree

1 file changed

+53
-93
lines changed

1 file changed

+53
-93
lines changed

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

Lines changed: 53 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,11 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
134134
coreTpl, cellTpl, shardTpl, baseCluster, clusterName, namespace, finalizerName := setupFixtures()
135135

136136
tests := map[string]struct {
137-
multigrescluster *multigresv1alpha1.MultigresCluster
138-
existingObjects []client.Object
139-
preReconcileUpdate func(testing.TB, *multigresv1alpha1.MultigresCluster)
140-
validate func(testing.TB, client.Client)
137+
multigrescluster *multigresv1alpha1.MultigresCluster
138+
existingObjects []client.Object
139+
preReconcileUpdate func(testing.TB, *multigresv1alpha1.MultigresCluster)
140+
skipClusterCreation bool
141+
validate func(testing.TB, client.Client)
141142
}{
142143
"Create: Adds Finalizer": {
143144
preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
@@ -412,7 +413,7 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
412413
},
413414
}
414415
},
415-
// Using defaults
416+
existingObjects: []client.Object{coreTpl, cellTpl, shardTpl},
416417
validate: func(t testing.TB, c client.Client) {
417418
ctx := t.Context()
418419
cell := &multigresv1alpha1.Cell{}
@@ -446,6 +447,22 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
446447
}
447448
},
448449
},
450+
"Create: No Global Topo Config": {
451+
preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
452+
c.Spec.GlobalTopoServer = multigresv1alpha1.GlobalTopoServerSpec{} // Empty
453+
c.Spec.TemplateDefaults = multigresv1alpha1.TemplateDefaults{} // Empty
454+
c.Spec.MultiAdmin = multigresv1alpha1.MultiAdminConfig{} // Empty
455+
},
456+
// Using defaults (coreTpl presence doesn't hurt)
457+
validate: func(t testing.TB, c client.Client) {
458+
// Verify Cell got empty topo address
459+
cell := &multigresv1alpha1.Cell{}
460+
_ = c.Get(t.Context(), types.NamespacedName{Name: clusterName + "-zone-a", Namespace: namespace}, cell)
461+
if got, want := cell.Spec.GlobalTopoServer.Address, ""; got != want {
462+
t.Errorf("Expected empty topo address mismatch got %q, want %q", got, want)
463+
}
464+
},
465+
},
449466
"Status: Aggregation Logic": {
450467
preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
451468
c.Spec.Databases = append(c.Spec.Databases, multigresv1alpha1.DatabaseConfig{Name: "db2", TableGroups: []multigresv1alpha1.TableGroupConfig{}})
@@ -490,7 +507,8 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
490507
},
491508
},
492509
"Object Not Found (Clean Exit)": {
493-
existingObjects: []client.Object{},
510+
skipClusterCreation: true,
511+
existingObjects: []client.Object{},
494512
},
495513
}
496514

@@ -504,14 +522,6 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
504522
objects = []client.Object{coreTpl, cellTpl, shardTpl}
505523
}
506524

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-
515525
// Apply defaults if no specific cluster is provided
516526
cluster := tc.multigrescluster
517527
if cluster == nil {
@@ -523,39 +533,18 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
523533
tc.preReconcileUpdate(t, cluster)
524534
}
525535

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
536+
// Inject cluster into existingObjects if creation is not skipped
537+
if !tc.skipClusterCreation {
538+
objects = append(objects, cluster)
531539
}
532540

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-
}
541+
clientBuilder := fake.NewClientBuilder().
542+
WithScheme(scheme).
543+
WithObjects(objects...).
544+
WithStatusSubresource(&multigresv1alpha1.MultigresCluster{}, &multigresv1alpha1.Cell{}, &multigresv1alpha1.TableGroup{})
545+
baseClient := clientBuilder.Build()
540546

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-
}
547+
finalClient := baseClient
559548

560549
reconciler := &MultigresClusterReconciler{
561550
Client: finalClient,
@@ -594,11 +583,12 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
594583
errBoom := errors.New("boom")
595584

596585
tests := map[string]struct {
597-
multigrescluster *multigresv1alpha1.MultigresCluster
598-
existingObjects []client.Object
599-
failureConfig *testutil.FailureConfig
600-
preReconcileUpdate func(testing.TB, *multigresv1alpha1.MultigresCluster)
601-
validate func(testing.TB, client.Client)
586+
multigrescluster *multigresv1alpha1.MultigresCluster
587+
existingObjects []client.Object
588+
failureConfig *testutil.FailureConfig
589+
preReconcileUpdate func(testing.TB, *multigresv1alpha1.MultigresCluster)
590+
skipClusterCreation bool
591+
validate func(testing.TB, client.Client)
602592
}{
603593
"Delete: Block Finalization if Cells Exist": {
604594
preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
@@ -918,21 +908,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
918908
objects = []client.Object{coreTpl, cellTpl, shardTpl}
919909
}
920910

921-
clientBuilder := fake.NewClientBuilder().
922-
WithScheme(scheme).
923-
WithObjects(objects...).
924-
WithStatusSubresource(&multigresv1alpha1.MultigresCluster{}, &multigresv1alpha1.Cell{}, &multigresv1alpha1.TableGroup{})
925-
baseClient := clientBuilder.Build()
926-
927-
var finalClient client.Client
928-
finalClient = baseClient
929-
if tc.failureConfig != nil {
930-
finalClient = testutil.NewFakeClientWithFailures(baseClient, tc.failureConfig)
931-
}
932-
// In Failure tests, finalClient is assigned a new value (the wrapper), so we explicitly cast baseClient
933-
// to client.Client interface if needed, or rely on type compatibility.
934-
// Since failureConfig wrapper implements client.Client, we assign it.
935-
936911
// Apply defaults if no specific cluster is provided
937912
cluster := tc.multigrescluster
938913
if cluster == nil {
@@ -944,38 +919,23 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
944919
tc.preReconcileUpdate(t, cluster)
945920
}
946921

947-
// Capture if deletion is intended BEFORE calling Create,
948-
// because Create strips DeletionTimestamp from the local struct in some client implementations (or API logic mimics it).
949-
shouldDelete := false
950-
if cluster.GetDeletionTimestamp() != nil && !cluster.GetDeletionTimestamp().IsZero() {
951-
shouldDelete = true
922+
// Inject cluster into existingObjects if creation is not skipped
923+
// This handles initializing the fake client with the cluster state,
924+
// including DeletionTimestamp if set by preReconcileUpdate.
925+
if !tc.skipClusterCreation {
926+
objects = append(objects, cluster)
952927
}
953928

954-
if !strings.Contains(name, "Object Not Found") {
955-
check := &multigresv1alpha1.MultigresCluster{}
956-
err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, check)
957-
if apierrors.IsNotFound(err) {
958-
if err := baseClient.Create(t.Context(), cluster); err != nil {
959-
t.Fatalf("failed to create initial cluster: %v", err)
960-
}
929+
clientBuilder := fake.NewClientBuilder().
930+
WithScheme(scheme).
931+
WithObjects(objects...).
932+
WithStatusSubresource(&multigresv1alpha1.MultigresCluster{}, &multigresv1alpha1.Cell{}, &multigresv1alpha1.TableGroup{})
933+
baseClient := clientBuilder.Build()
961934

962-
// Ensure DeletionTimestamp is set in the API if the test requires it.
963-
// client.Create strips this field, so we must invoke Delete() to re-apply it.
964-
if shouldDelete {
965-
// 1. Refresh object to avoid ResourceVersion conflict
966-
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
967-
t.Fatalf("failed to refresh cluster before delete: %v", err)
968-
}
969-
// 2. Delete it
970-
if err := baseClient.Delete(t.Context(), cluster); err != nil {
971-
t.Fatalf("failed to set deletion timestamp: %v", err)
972-
}
973-
// 3. Refresh again to ensure the controller sees the deletion timestamp
974-
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
975-
t.Fatalf("failed to refresh cluster after deletion: %v", err)
976-
}
977-
}
978-
}
935+
var finalClient client.Client
936+
finalClient = client.Client(baseClient)
937+
if tc.failureConfig != nil {
938+
finalClient = testutil.NewFakeClientWithFailures(baseClient, tc.failureConfig)
979939
}
980940

981941
reconciler := &MultigresClusterReconciler{

0 commit comments

Comments
 (0)