Added WaitForRunningPodCount back in to helpers and made it the main point of assertion for testing the accurate strategy

I hastily assumed that a combination of WaitForScaledJobCount and WaitForAllPodRunningInNamespace would satisfy what is needed for the test. However, that approach comes with a major flaw. That is, unless you delete the jobs, those pods count towards the condition in WaitForAllPodRunningInNamespace. Thus, a much simpler way to test the strategy is to wait for the correct amount of pods to be running. A running pod in the given namespace implies a corresponding Job exists. So, we test the 2 strategy cases with pod count. This also passes local tests

Issue #3661

Signed-off-by: jansworld <navon.josh@gmail.com>

Author: jansworld

tests/helper/helper.go

@@ -490,6 +490,35 @@ func WaitForAllPodRunningInNamespace(t *testing.T, kc *kubernetes.Clientset, nam
 	return false
 }
 
+func WaitForRunningPodCount(t *testing.T, kc *kubernetes.Clientset, scaledJobName, namespace string, target, iterations, interval int) bool {
+	for i := 0; i < iterations; i++ {
+		pods, err := kc.CoreV1().Pods(namespace).List(context.Background(), metav1.ListOptions{
+			LabelSelector: fmt.Sprintf("scaledjob.keda.sh/name=%s", scaledJobName),
+		})
+		if err != nil {
+			t.Logf("cannot list pods - %s", err)
+		}
+
+		runningPodCount := 0
+		for _, pod := range pods.Items {
+			if pod.Status.Phase == corev1.PodRunning {
+				runningPodCount++
+			}
+		}
+
+		t.Logf("Waiting for running pods. Namespace - %s, Current - %d, Target - %d",
+			namespace, runningPodCount, target)
+		if runningPodCount == target {
+			return true
+		} else if runningPodCount > target {
+			return false
+		}
+
+		time.Sleep(time.Duration(interval) * time.Second)
+	}
+	return false
+}
+
 // Waits until the Horizontal Pod Autoscaler for the scaledObject reports that it has metrics available
 // to calculate, or until the number of iterations are done, whichever happens first.
 func WaitForHPAMetricsToPopulate(t *testing.T, kc *kubernetes.Clientset, name, namespace string, iterations, intervalSeconds int) bool {

tests/internals/scaling_strategies/accurate_scaling_strategy/accurate_scaling_strategy_test.go

@@ -62,7 +62,7 @@ spec:
             image: docker.io/library/busybox
             command:
             - sleep
-            - "60"
+            - "30"
             imagePullPolicy: IfNotPresent
             envFrom:
             - secretRef:
@@ -130,41 +130,38 @@ func getTemplateData() (templateData, []Template) {
 }
 
 func testAccurateScaling(ctx context.Context, t *testing.T, kc *kubernetes.Clientset, client *azqueue.QueueClient) {
-	iterationCount := 30
+	iterationCount := 60
 
-	// Base case (number of scale = maxScale since pendingJobs = 0)
+	// Base case (number of scale = maxScale since pendingJobs = 0). Enqueue up 4 messages
 	enqueueMessages(ctx, t, client, 4)
-	assert.True(t, WaitForScaledJobCount(t, kc, scaledJobName, testNamespace, 4, iterationCount, 1),
-		"job count should be %d after %d iterations", 4, iterationCount)
-	assert.True(t, WaitForAllPodRunningInNamespace(t, kc, testNamespace, iterationCount, 1),
-		"all scaledjob pods should be running after %d iterations", iterationCount)
+	assert.True(t, WaitForRunningPodCount(t, kc, scaledJobName, testNamespace, 4, iterationCount, 1),
+		"running pod count should be %d after %d iterations", 4, iterationCount)
 
-	// Clear the queue to simulate message consumption and wait for job completion
+	// Clear the messages to simulate message consumption
 	_, err := client.ClearMessages(ctx, nil)
 	assert.NoErrorf(t, err, "cannot clear queue - %s", err)
+
+	// Wait for job completion
 	WaitForAllJobsSuccess(t, kc, testNamespace, 90, 1)
 
-	// Test the cap condition (maxScale + runningJobs > maxReplicaCount)
+	// Test the cap condition (maxScale + runningJobs > maxReplicaCount). Enqueue 4 messages
 	enqueueMessages(ctx, t, client, 4)
-	assert.True(t, WaitForScaledJobCount(t, kc, scaledJobName, testNamespace, 4, iterationCount, 1),
-		"running job count should be %d after %d iterations", 4, iterationCount)
-	assert.True(t, WaitForAllPodRunningInNamespace(t, kc, testNamespace, iterationCount, 1),
-		"all scaledjob pods should be running after %d iterations", iterationCount)
+	assert.True(t, WaitForRunningPodCount(t, kc, scaledJobName, testNamespace, 4, iterationCount, 1),
+		"running pod count should be %d after %d iterations", 4, iterationCount)
 
 	// Clear the messages to simulate message consumption
 	_, err = client.ClearMessages(ctx, nil)
 	assert.NoErrorf(t, err, "cannot clear queue - %s", err)
 
-	// Queue up 8 more messages to trigger the cap condition
+	// Enqueue 8 more messages to trigger the cap condition
 	enqueueMessages(ctx, t, client, 8)
-	assert.True(t, WaitForScaledJobCount(t, kc, scaledJobName, testNamespace, 10, iterationCount, 1),
-		"running job count should be %d after %d iterations", 10, iterationCount)
-	assert.True(t, WaitForAllPodRunningInNamespace(t, kc, testNamespace, iterationCount, 1),
-		"all scaledjob pods should be running after %d iterations", iterationCount)
+	assert.True(t, WaitForRunningPodCount(t, kc, scaledJobName, testNamespace, 10, iterationCount, 1),
+		"running pod count should be %d after %d iterations", 10, iterationCount)
 
-	// Message cleanup and wait for jobs to complete
 	_, err = client.ClearMessages(ctx, nil)
 	assert.NoErrorf(t, err, "cannot clear queue - %s", err)
+
+	// Message cleanup and wait for jobs to complete
 	WaitForAllJobsSuccess(t, kc, testNamespace, 120, 1)
 }