add unit tests for api-service condition methods and also refactor out getAPIServiceConditionByType

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/helpers.go

@@ -98,16 +98,20 @@ func NewLocalAvailableAPIServiceCondition() APIServiceCondition {
 	}
 }
 
+// GetAPIServiceConditionByType gets an *APIServiceCondition by APIServiceConditionType if present
+func GetAPIServiceConditionByType(apiService *APIService, conditionType APIServiceConditionType) *APIServiceCondition {
+	for i := range apiService.Status.Conditions {
+		if apiService.Status.Conditions[i].Type == conditionType {
+			return &apiService.Status.Conditions[i]
+		}
+	}
+	return nil
+}
+
 // SetAPIServiceCondition sets the status condition.  It either overwrites the existing one or
 // creates a new one
 func SetAPIServiceCondition(apiService *APIService, newCondition APIServiceCondition) {
-	var existingCondition *APIServiceCondition
+	existingCondition := GetAPIServiceConditionByType(apiService, newCondition.Type)
-	for i := range apiService.Status.Conditions {
-		if apiService.Status.Conditions[i].Type == newCondition.Type {
-			existingCondition = &apiService.Status.Conditions[i]
-			break
-		}
-	}
 	if existingCondition == nil {
 		apiService.Status.Conditions = append(apiService.Status.Conditions, newCondition)
 		return
@@ -124,10 +128,6 @@ func SetAPIServiceCondition(apiService *APIService, newCondition APIServiceCondi
 
 // IsAPIServiceConditionTrue indicates if the condition is present and strictly true
 func IsAPIServiceConditionTrue(apiService *APIService, conditionType APIServiceConditionType) bool {
-	for _, condition := range apiService.Status.Conditions {
+	condition := GetAPIServiceConditionByType(apiService, conditionType)
-		if condition.Type == conditionType && condition.Status == ConditionTrue {
+	return condition != nil && condition.Status == ConditionTrue
-			return true
-		}
-	}
-	return false
 }

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/helpers_test.go

@@ -21,6 +21,124 @@ import (
 	"testing"
 )
 
+var (
+	a APIServiceConditionType = "A"
+	b APIServiceConditionType = "B"
+	c APIServiceConditionType = "C"
+)
+
+func TestGetAPIServiceConditionByType(t *testing.T) {
+	conditionA := makeNewAPIServiceCondition(a, "a reason", "a message", ConditionTrue)
+	conditionB := makeNewAPIServiceCondition(b, "b reason", "b message", ConditionTrue)
+	tests := []*struct {
+		name              string
+		apiService        *APIService
+		conditionType     APIServiceConditionType
+		expectedCondition *APIServiceCondition
+	}{
+		{
+			name:              "Should find a matching condition from apiService",
+			apiService:        makeNewApiService("v1", 100, conditionA, conditionB),
+			conditionType:     a,
+			expectedCondition: &conditionA,
+		},
+		{
+			name:              "Should not find a matching condition",
+			apiService:        makeNewApiService("v1", 100, conditionA),
+			conditionType:     b,
+			expectedCondition: nil,
+		},
+	}
+
+	for _, tc := range tests {
+		actual := GetAPIServiceConditionByType(tc.apiService, tc.conditionType)
+		if !reflect.DeepEqual(tc.expectedCondition, actual) {
+			t.Errorf("expected %s, actual %s", tc.expectedCondition, actual)
+		}
+	}
+}
+
+func TestIsAPIServiceConditionTrue(t *testing.T) {
+	conditionATrue := makeNewAPIServiceCondition(a, "a reason", "a message", ConditionTrue)
+	conditionAFalse := makeNewAPIServiceCondition(a, "a reason", "a message", ConditionFalse)
+	tests := []*struct {
+		name          string
+		apiService    *APIService
+		conditionType APIServiceConditionType
+		expected      bool
+	}{
+		{
+			name:          "Should return false when condition of type is not present",
+			apiService:    makeNewApiService("v1", 100),
+			conditionType: a,
+			expected:      false,
+		},
+		{
+			name:          "Should return false when condition of type is present but status is not ConditionTrue",
+			apiService:    makeNewApiService("v1", 100, conditionAFalse),
+			conditionType: a,
+			expected:      false,
+		},
+		{
+			name:          "Should return false when condition of type is present but status is not ConditionTrue",
+			apiService:    makeNewApiService("v1", 100, conditionATrue),
+			conditionType: a,
+			expected:      true,
+		},
+	}
+
+	for _, tc := range tests {
+		if isConditionTrue := IsAPIServiceConditionTrue(tc.apiService, tc.conditionType); isConditionTrue != tc.expected {
+			t.Errorf("expected condition of type %v to be %v, actually was %v",
+				tc.conditionType, isConditionTrue, tc.expected)
+
+		}
+	}
+}
+
+func TestSetAPIServiceCondition(t *testing.T) {
+	conditionA1 := makeNewAPIServiceCondition(a, "a1 reason", "a1 message", ConditionTrue)
+	conditionA2 := makeNewAPIServiceCondition(a, "a2 reason", "a2 message", ConditionTrue)
+	tests := []*struct {
+		name              string
+		apiService        *APIService
+		conditionType     APIServiceConditionType
+		initialCondition  *APIServiceCondition
+		setCondition      APIServiceCondition
+		expectedCondition *APIServiceCondition
+	}{
+		{
+			name:              "Should set a new condition with type where previously there was no condition of that type",
+			apiService:        makeNewApiService("v1", 100),
+			conditionType:     a,
+			initialCondition:  nil,
+			setCondition:      conditionA1,
+			expectedCondition: &conditionA1,
+		},
+		{
+			name:              "Should override a condition of type, when a condition of that type existed previously",
+			apiService:        makeNewApiService("v1", 100, conditionA1),
+			conditionType:     a,
+			initialCondition:  &conditionA1,
+			setCondition:      conditionA2,
+			expectedCondition: &conditionA2,
+		},
+	}
+
+	for _, tc := range tests {
+		startingCondition := GetAPIServiceConditionByType(tc.apiService, tc.conditionType)
+		if !reflect.DeepEqual(startingCondition, tc.initialCondition) {
+			t.Errorf("expected to find condition %s initially, actual was %s", tc.initialCondition, startingCondition)
+
+		}
+		SetAPIServiceCondition(tc.apiService, tc.setCondition)
+		actual := GetAPIServiceConditionByType(tc.apiService, tc.setCondition.Type)
+		if !reflect.DeepEqual(actual, tc.expectedCondition) {
+			t.Errorf("expected %s, actual %s", tc.expectedCondition, actual)
+		}
+	}
+}
+
 func TestSortedAPIServicesByVersion(t *testing.T) {
 	tests := []*struct {
 		name     string
@@ -55,12 +173,12 @@ func TestSortedAPIServicesByVersion(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		apiServices := []*APIService{}
+		apiServices := make([]*APIService, 0)
 		for _, v := range tc.versions {
-			apiServices = append(apiServices, &APIService{Spec: APIServiceSpec{Version: v, VersionPriority: 100}})
+			apiServices = append(apiServices, makeNewApiService(v, 100))
 		}
 		sortedServices := SortedByGroupAndVersion(apiServices)
-		actual := []string{}
+		actual := make([]string, 0)
 		for _, s := range sortedServices[0] {
 			actual = append(actual, s.Spec.Version)
 		}
@@ -69,3 +187,12 @@ func TestSortedAPIServicesByVersion(t *testing.T) {
 		}
 	}
 }
+
+func makeNewApiService(version string, priority int32, conditions ...APIServiceCondition) *APIService {
+	status := APIServiceStatus{Conditions: conditions}
+	return &APIService{Spec: APIServiceSpec{Version: version, VersionPriority: priority}, Status: status}
+}
+
+func makeNewAPIServiceCondition(conditionType APIServiceConditionType, reason string, message string, status ConditionStatus) APIServiceCondition {
+	return APIServiceCondition{Type: conditionType, Reason: reason, Message: message, Status: status}
+}