Skip to content

Commit cd48659

Browse files
fernando-villalbafernando-villalba
committed
refactor(controller): simplify test setup and fix deletion handling in cluster tests
- pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go: - Remove `setupFunc` from the test table structure as it was unnecessary with the `fake` client. - Remove redundant IIFE patterns from `existingObjects` in failure tests, relying on the test runner to create the initial cluster instead. - Update the test runner to intelligently handle `DeletionTimestamp`. Since `client.Create` strips metadata like timestamps, the runner now checks for a deletion timestamp on the input object and conditionally triggers a `client.Delete` to correctly simulate resources in a terminating state. - Ensure `baseCluster` defaults are applied consistently when `tc.multigrescluster` is nil.
1 parent fbd230a commit cd48659

File tree

1 file changed

+44
-67
lines changed

1 file changed

+44
-67
lines changed

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

Lines changed: 44 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -491,15 +491,7 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
491491
c.DeletionTimestamp = &now
492492
c.Finalizers = []string{finalizerName}
493493
},
494-
existingObjects: []client.Object{
495-
func() *multigresv1alpha1.MultigresCluster {
496-
c := baseCluster.DeepCopy()
497-
now := metav1.Now()
498-
c.DeletionTimestamp = &now
499-
c.Finalizers = []string{finalizerName}
500-
return c
501-
}(),
502-
},
494+
existingObjects: []client.Object{},
503495
validate: func(t testing.TB, c client.Client) {
504496
updated := &multigresv1alpha1.MultigresCluster{}
505497
err := c.Get(t.Context(), types.NamespacedName{Name: clusterName, Namespace: namespace}, updated)
@@ -539,13 +531,32 @@ func TestMultigresClusterReconciler_Reconcile_Success(t *testing.T) {
539531
tc.preReconcileUpdate(t, cluster)
540532
}
541533

534+
// Capture if deletion is intended BEFORE calling Create,
535+
// because Create strips DeletionTimestamp from the local struct in some client implementations (or API logic mimics it).
536+
shouldDelete := false
537+
if cluster.GetDeletionTimestamp() != nil && !cluster.GetDeletionTimestamp().IsZero() {
538+
shouldDelete = true
539+
}
540+
542541
if !strings.Contains(name, "Object Not Found") {
543542
check := &multigresv1alpha1.MultigresCluster{}
544543
err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, check)
545544
if apierrors.IsNotFound(err) {
546545
if err := baseClient.Create(t.Context(), cluster); err != nil {
547546
t.Fatalf("failed to create initial cluster: %v", err)
548547
}
548+
549+
// Ensure DeletionTimestamp is set in the API if the test requires it.
550+
// client.Create strips this field, so we must invoke Delete() to re-apply it.
551+
if shouldDelete {
552+
if err := baseClient.Delete(t.Context(), cluster); err != nil {
553+
t.Fatalf("failed to set deletion timestamp: %v", err)
554+
}
555+
// Refresh our local object to match the client state
556+
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
557+
t.Fatalf("failed to refresh cluster after deletion: %v", err)
558+
}
559+
}
549560
}
550561
}
551562

@@ -599,13 +610,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
599610
c.Finalizers = []string{finalizerName}
600611
},
601612
existingObjects: []client.Object{
602-
func() *multigresv1alpha1.MultigresCluster {
603-
c := baseCluster.DeepCopy()
604-
now := metav1.Now()
605-
c.DeletionTimestamp = &now
606-
c.Finalizers = []string{finalizerName}
607-
return c
608-
}(),
609613
&multigresv1alpha1.Cell{ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-zone-a", Namespace: namespace, Labels: map[string]string{"multigres.com/cluster": clusterName}}},
610614
},
611615
},
@@ -616,13 +620,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
616620
c.Finalizers = []string{finalizerName}
617621
},
618622
existingObjects: []client.Object{
619-
func() *multigresv1alpha1.MultigresCluster {
620-
c := baseCluster.DeepCopy()
621-
now := metav1.Now()
622-
c.DeletionTimestamp = &now
623-
c.Finalizers = []string{finalizerName}
624-
return c
625-
}(),
626623
&multigresv1alpha1.TableGroup{ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-db1-tg1", Namespace: namespace, Labels: map[string]string{"multigres.com/cluster": clusterName}}},
627624
},
628625
},
@@ -633,13 +630,6 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
633630
c.Finalizers = []string{finalizerName}
634631
},
635632
existingObjects: []client.Object{
636-
func() *multigresv1alpha1.MultigresCluster {
637-
c := baseCluster.DeepCopy()
638-
now := metav1.Now()
639-
c.DeletionTimestamp = &now
640-
c.Finalizers = []string{finalizerName}
641-
return c
642-
}(),
643633
&multigresv1alpha1.TopoServer{ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-global-topo", Namespace: namespace, Labels: map[string]string{"multigres.com/cluster": clusterName}}},
644634
},
645635
},
@@ -679,32 +669,16 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
679669
c.DeletionTimestamp = &now
680670
c.Finalizers = []string{finalizerName}
681671
},
682-
existingObjects: []client.Object{
683-
func() *multigresv1alpha1.MultigresCluster {
684-
c := baseCluster.DeepCopy()
685-
now := metav1.Now()
686-
c.DeletionTimestamp = &now
687-
c.Finalizers = []string{finalizerName}
688-
return c
689-
}(),
690-
},
691-
failureConfig: &testutil.FailureConfig{OnUpdate: testutil.FailOnObjectName(clusterName, errBoom)},
672+
existingObjects: []client.Object{},
673+
failureConfig: &testutil.FailureConfig{OnUpdate: testutil.FailOnObjectName(clusterName, errBoom)},
692674
},
693675
"Error: CheckChildrenDeleted (List Cells Failed)": {
694676
preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
695677
now := metav1.Now()
696678
c.DeletionTimestamp = &now
697679
c.Finalizers = []string{finalizerName}
698680
},
699-
existingObjects: []client.Object{
700-
func() *multigresv1alpha1.MultigresCluster {
701-
c := baseCluster.DeepCopy()
702-
now := metav1.Now()
703-
c.DeletionTimestamp = &now
704-
c.Finalizers = []string{finalizerName}
705-
return c
706-
}(),
707-
},
681+
existingObjects: []client.Object{},
708682
failureConfig: &testutil.FailureConfig{
709683
OnList: func(list client.ObjectList) error {
710684
if _, ok := list.(*multigresv1alpha1.CellList); ok {
@@ -720,15 +694,7 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
720694
c.DeletionTimestamp = &now
721695
c.Finalizers = []string{finalizerName}
722696
},
723-
existingObjects: []client.Object{
724-
func() *multigresv1alpha1.MultigresCluster {
725-
c := baseCluster.DeepCopy()
726-
now := metav1.Now()
727-
c.DeletionTimestamp = &now
728-
c.Finalizers = []string{finalizerName}
729-
return c
730-
}(),
731-
},
697+
existingObjects: []client.Object{},
732698
failureConfig: &testutil.FailureConfig{
733699
OnList: func(list client.ObjectList) error {
734700
if _, ok := list.(*multigresv1alpha1.TableGroupList); ok {
@@ -744,15 +710,7 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
744710
c.DeletionTimestamp = &now
745711
c.Finalizers = []string{finalizerName}
746712
},
747-
existingObjects: []client.Object{
748-
func() *multigresv1alpha1.MultigresCluster {
749-
c := baseCluster.DeepCopy()
750-
now := metav1.Now()
751-
c.DeletionTimestamp = &now
752-
c.Finalizers = []string{finalizerName}
753-
return c
754-
}(),
755-
},
713+
existingObjects: []client.Object{},
756714
failureConfig: &testutil.FailureConfig{
757715
OnList: func(list client.ObjectList) error {
758716
if _, ok := list.(*multigresv1alpha1.TopoServerList); ok {
@@ -980,13 +938,32 @@ func TestMultigresClusterReconciler_Reconcile_Failure(t *testing.T) {
980938
tc.preReconcileUpdate(t, cluster)
981939
}
982940

941+
// Capture if deletion is intended BEFORE calling Create,
942+
// because Create strips DeletionTimestamp from the local struct in some client implementations (or API logic mimics it).
943+
shouldDelete := false
944+
if cluster.GetDeletionTimestamp() != nil && !cluster.GetDeletionTimestamp().IsZero() {
945+
shouldDelete = true
946+
}
947+
983948
if !strings.Contains(name, "Object Not Found") {
984949
check := &multigresv1alpha1.MultigresCluster{}
985950
err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, check)
986951
if apierrors.IsNotFound(err) {
987952
if err := baseClient.Create(t.Context(), cluster); err != nil {
988953
t.Fatalf("failed to create initial cluster: %v", err)
989954
}
955+
956+
// Ensure DeletionTimestamp is set in the API if the test requires it.
957+
// client.Create strips this field, so we must invoke Delete() to re-apply it.
958+
if shouldDelete {
959+
if err := baseClient.Delete(t.Context(), cluster); err != nil {
960+
t.Fatalf("failed to set deletion timestamp: %v", err)
961+
}
962+
// Refresh our local object to match the client state
963+
if err := baseClient.Get(t.Context(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, cluster); err != nil {
964+
t.Fatalf("failed to refresh cluster after deletion: %v", err)
965+
}
966+
}
990967
}
991968
}
992969

0 commit comments

Comments
 (0)