Make deduplicateErrorStrings more efficient, and improve unit test or…

…ganization

pkg/operator/controller/canary/controller.go

@@ -44,6 +44,9 @@ const (
 	// canaryCheckFailureCount is how many successive failing canary checks should
 	// be observed before the default ingress controller goes degraded.
 	canaryCheckFailureCount = 5
+	// canaryFailingNumErrors is how many error messages to include in the
+	// CanaryChecksSucceeding status condition when checks are failing
+	canaryFailingNumErrors = 3
 
 	// CanaryRouteRotationAnnotation is an annotation on the default ingress controller
 	// that specifies whether or not the canary check loop should periodically rotate
@@ -312,9 +315,9 @@ func (r *reconciler) startCanaryRoutePolling(stop <-chan struct{}) error {
 }
 
 func (r *reconciler) setCanaryFailingStatusCondition(errors []error) error {
-	errorStrings := DeduplicateErrorStrings(errors)
-	if len(errorStrings) > 3 {
-		errorStrings = errorStrings[len(errorStrings)-3:]
+	errorStrings := deduplicateErrorStrings(errors)
+	if len(errorStrings) > canaryFailingNumErrors {
+		errorStrings = errorStrings[len(errorStrings)-canaryFailingNumErrors:]
 	}
 	cond := operatorv1.OperatorCondition{
 		Type:    ingresscontroller.IngressControllerCanaryCheckSuccessConditionType,
@@ -326,30 +329,36 @@ func (r *reconciler) setCanaryFailingStatusCondition(errors []error) error {
 	return r.setCanaryStatusCondition(cond)
 }
 
-// DeduplicateErrorStrings takes a slice of errors in chronological order, prunes the list for unique error messages
+// deduplicateErrorStrings takes a slice of errors in chronological order, prunes the list for unique error messages
 // It returns a slice of the error message as strings, with any string seen more than once incuding how many times it
 // was seen
 // The chronological order is preserved, based on the last time each error message was seen
-func DeduplicateErrorStrings(errors []error) []string {
+func deduplicateErrorStrings(errors []error) []string {
 	encountered := map[string]int{}
 	ret := []string{}
 	// Iterate over the list of errors from newest to oldest, keeping track of how many times each error message was
-	// seen
+	// seen. This iteration order makes sure we preserve order based on the newest time each message was seen
 	for i := len(errors) - 1; i >= 0; i-- {
 		if encountered[errors[i].Error()] >= 1 {
 			encountered[errors[i].Error()] += 1
 			continue
 		}
+		// if the error message hasn't been encountered yet, append it to the uniques list. This creates a list in the
+		// reverse order of the initial list, so it will need to be reversed later
 		encountered[errors[i].Error()] = 1
-		ret = append([]string{errors[i].Error()}, ret...)
+		ret = append(ret, errors[i].Error())
 	}
-	// Now that all error messages have been tallied, add a comment to each message that was seen more than once, so the
-	// reader can tell how often each error has occurred
+	// Now that all error messages have been tallied, prepend a comment to each message that was seen more than once, so
+	// the reader can tell how often each error has occurred
 	for i := range ret {
 		if encountered[ret[i]] > 1 {
 			ret[i] = fmt.Sprintf("[Seen %d times] %s", encountered[ret[i]], ret[i])
 		}
 	}
+	// Reverse the list so that it can be returned in oldest to newest order
+	for i, j := 0, len(ret)-1; i < j; i, j = i+1, j-1 {
+		ret[i], ret[j] = ret[j], ret[i]
+	}
 	return ret
 }
 

pkg/operator/controller/canary/controller_test.go

@@ -148,7 +148,7 @@ func TestCycleServicePort(t *testing.T) {
 	}
 }
 
-func TestDeduplicateErrorStrings(t *testing.T) {
+func Test_deduplicateErrorStrings(t *testing.T) {
 	testCases := []struct {
 		Name           string
 		ErrorMessages  []error
@@ -212,9 +212,11 @@ func TestDeduplicateErrorStrings(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		result := DeduplicateErrorStrings(tc.ErrorMessages)
-		if !cmp.Equal(tc.ExpectedResult, result) {
-			t.Errorf("test case %q failed: expected result:\n%s\nbut got:\n%s", tc.Name, strings.Join(tc.ExpectedResult, "\n"), strings.Join(result, "\n"))
-		}
+		t.Run(tc.Name, func(t *testing.T) {
+			result := deduplicateErrorStrings(tc.ErrorMessages)
+			if !cmp.Equal(tc.ExpectedResult, result) {
+				t.Errorf("Expected result:\n%s\nbut got:\n%s", strings.Join(tc.ExpectedResult, "\n"), strings.Join(result, "\n"))
+			}
+		})
 	}
 }