fix: fix TestPickBindingsToRoll rollout unit test (#964)

fix naming conventions, correct UT accordingly, and add comments

Author: britaniar

pkg/controllers/rollout/controller_test.go

@@ -744,13 +744,14 @@ func TestPickBindingsToRoll(t *testing.T) {
 	crpWithApplyStrategy.Spec.Strategy.ApplyStrategy = &fleetv1beta1.ApplyStrategy{
 		Type: fleetv1beta1.ApplyStrategyTypeServerSideApply,
 	}
+	// crpWithUnavailablePeriod has a 60s unavailable period
 	crpWithUnavailablePeriod := clusterResourcePlacementForTest("test",
 		createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, 3))
 	crpWithUnavailablePeriod.Spec.Strategy.RollingUpdate.UnavailablePeriodSeconds = ptr.To(60)
 
-	readyBinding := generateClusterResourceBinding(fleetv1beta1.BindingStateBound, "snapshot-1", cluster1)
-	readyBinding.Generation = 15
-	readyBinding.Status.Conditions = []metav1.Condition{
+	canBeReadyBinding := generateClusterResourceBinding(fleetv1beta1.BindingStateBound, "snapshot-1", cluster1)
+	canBeReadyBinding.Generation = 15
+	canBeReadyBinding.Status.Conditions = []metav1.Condition{
 		{
 			Type:               string(fleetv1beta1.ResourceBindingApplied),
 			Status:             metav1.ConditionTrue,
@@ -760,6 +761,8 @@ func TestPickBindingsToRoll(t *testing.T) {
 			},
 		},
 	}
+
+	// create bindings below to test different wait times
 	notReadyUnscheduledBinding := generateClusterResourceBinding(fleetv1beta1.BindingStateUnscheduled, "snapshot-1", cluster2)
 	notReadyUnscheduledBinding.Generation = 15
 	notReadyUnscheduledBinding.Status.Conditions = []metav1.Condition{
@@ -775,7 +778,7 @@ func TestPickBindingsToRoll(t *testing.T) {
 			LastTransitionTime: metav1.Time{
 				Time: now.Add(-1 * time.Minute),
 			},
-			Reason: work.WorkNotTrackableReason,
+			Reason: work.WorkNotTrackableReason, // Make it not ready
 		},
 	}
 
@@ -794,13 +797,13 @@ func TestPickBindingsToRoll(t *testing.T) {
 			LastTransitionTime: metav1.Time{
 				Time: now.Add(-35 * time.Second),
 			},
-			Reason: work.WorkNotTrackableReason,
+			Reason: work.WorkNotTrackableReason, // Make it not ready
 		},
 	}
 
-	notReadyBoundBinding := generateClusterResourceBinding(fleetv1beta1.BindingStateBound, "snapshot-2", cluster2)
-	notReadyBoundBinding.Generation = 15
-	notReadyBoundBinding.Status.Conditions = []metav1.Condition{
+	readyBoundBinding := generateClusterResourceBinding(fleetv1beta1.BindingStateBound, "snapshot-2", cluster2)
+	readyBoundBinding.Generation = 15
+	readyBoundBinding.Status.Conditions = []metav1.Condition{
 		{
 			Type:               string(fleetv1beta1.ResourceBindingApplied),
 			Status:             metav1.ConditionTrue,
@@ -810,13 +813,13 @@ func TestPickBindingsToRoll(t *testing.T) {
 			Type:               string(fleetv1beta1.ResourceBindingAvailable),
 			Status:             metav1.ConditionTrue,
 			ObservedGeneration: 15,
-			Reason:             work.WorkAvailableReason,
+			Reason:             work.WorkAvailableReason, // Make it ready
 		},
 	}
 
-	notReadyBoundBinding2 := generateClusterResourceBinding(fleetv1beta1.BindingStateBound, "snapshot-1", cluster3)
-	notReadyUnscheduledBinding2.Generation = 15
-	notReadyUnscheduledBinding2.Status.Conditions = []metav1.Condition{
+	notReadyBoundBinding := generateClusterResourceBinding(fleetv1beta1.BindingStateBound, "snapshot-1", cluster3)
+	notReadyBoundBinding.Generation = 15
+	notReadyBoundBinding.Status.Conditions = []metav1.Condition{
 		{
 			Type:               string(fleetv1beta1.ResourceBindingApplied),
 			Status:             metav1.ConditionTrue,
@@ -829,7 +832,7 @@ func TestPickBindingsToRoll(t *testing.T) {
 			LastTransitionTime: metav1.Time{
 				Time: now.Add(-35 * time.Second),
 			},
-			Reason: work.WorkNotTrackableReason,
+			Reason: work.WorkNotTrackableReason, // Make it not ready
 		},
 	}
 	tests := map[string]struct {
@@ -1365,7 +1368,7 @@ func TestPickBindingsToRoll(t *testing.T) {
 		},
 		"test with scheduled bindings (always update the scheduled binding first)": {
 			allBindings: []*fleetv1beta1.ClusterResourceBinding{
-				readyBinding,
+				canBeReadyBinding,
 				generateClusterResourceBinding(fleetv1beta1.BindingStateScheduled, "snapshot-1", cluster2),
 			},
 			latestResourceSnapshotName:  "snapshot-2",
@@ -1457,55 +1460,43 @@ func TestPickBindingsToRoll(t *testing.T) {
 			wantErr: controller.ErrExpectedBehavior,
 		},
 		"test bound bindings with different waitTimes": {
+			// want the min wait time of bound bindings that are not ready
 			allBindings: []*fleetv1beta1.ClusterResourceBinding{
-				notReadyBoundBinding,
-				readyBinding,
-				notReadyBoundBinding2,
+				notReadyBoundBinding, // notReady, waitTime = t - 35s
+				canBeReadyBinding,    // notReady, no wait time because it does not have available condition yet
+				readyBoundBinding,    // Ready
 			},
 			latestResourceSnapshotName:  "snapshot-2",
-			crp:                         crpWithUnavailablePeriod,
-			wantStaleUnselectedBindings: []int{1, 2},
+			crp:                         crpWithUnavailablePeriod, // UnavailablePeriodSeconds is 60s -> readyTimeCutOff = t - 60s
+			wantStaleUnselectedBindings: []int{0, 1},
 			wantDesiredBindingsSpec: []fleetv1beta1.ResourceBindingSpec{
 				{
 					State:                fleetv1beta1.BindingStateBound,
-					TargetCluster:        cluster2,
+					TargetCluster:        cluster3,
 					ResourceSnapshotName: "snapshot-2",
 				},
 				{
 					State:                fleetv1beta1.BindingStateBound,
 					TargetCluster:        cluster1,
 					ResourceSnapshotName: "snapshot-2",
 				},
-				{
-					State:                fleetv1beta1.BindingStateBound,
-					TargetCluster:        cluster3,
-					ResourceSnapshotName: "snapshot-2",
-				},
+				{}, // bound binding is ready and up-to-date pointing to the latest resource snapshot.
 			},
 			wantNeedRoll: true,
-			wantWaitTime: 60 * time.Second,
+			wantWaitTime: 25 * time.Second, // minWaitTime = (t - 35 seconds) - (t - 60 seconds) = 25 seconds
 		},
 		"test unscheduled bindings with different waitTimes": {
+			// want the min wait time of unscheduled bindings that are not ready
 			allBindings: []*fleetv1beta1.ClusterResourceBinding{
-				notReadyUnscheduledBinding,
-				notReadyUnscheduledBinding2,
+				notReadyUnscheduledBinding,  // NotReady, waitTime = t - 60s
+				notReadyUnscheduledBinding2, // NotReady,  waitTime = t - 35s
 			},
-			latestResourceSnapshotName: "snapshot-2",
-			crp:                        crpWithUnavailablePeriod,
-			wantDesiredBindingsSpec: []fleetv1beta1.ResourceBindingSpec{
-				{
-					State:                fleetv1beta1.BindingStateBound,
-					TargetCluster:        cluster2,
-					ResourceSnapshotName: "snapshot-2",
-				},
-				{
-					State:                fleetv1beta1.BindingStateBound,
-					TargetCluster:        cluster3,
-					ResourceSnapshotName: "snapshot-2",
-				},
-			},
-			wantNeedRoll: true,
-			wantWaitTime: 25 * time.Second,
+			latestResourceSnapshotName:  "snapshot-2",
+			crp:                         crpWithUnavailablePeriod,             // UnavailablePeriodSeconds is 60s -> readyTimeCutOff = t - 60s
+			wantStaleUnselectedBindings: []int{},                              // empty list as unscheduled bindings will be removed and are not tracked in the CRP today.
+			wantDesiredBindingsSpec:     []fleetv1beta1.ResourceBindingSpec{}, // unscheduled binding does not have desired spec so that putting the empty here
+			wantNeedRoll:                true,
+			wantWaitTime:                25 * time.Second, // minWaitTime = (t - 35 seconds) - (t - 60 seconds) = 25 seconds
 		},
 	}
 	for name, tt := range tests {