Introduced error constants and replaced reflect with cmp

pkg/controller.v1beta1/experiment/manifest/generator.go

@@ -17,6 +17,7 @@ limitations under the License.
 package manifest
 
 import (
+	"errors"
 	"fmt"
 	"regexp"
 	"strings"
@@ -33,6 +34,15 @@ import (
 	"github.com/kubeflow/katib/pkg/util/v1beta1/katibconfig"
 )
 
+var (
+	ErrConfigMapNotFound                  = errors.New("ConfigMap not found")
+	ErrConvertStringToUnstructuredFailed  = errors.New("ConvertStringToUnstructured failed")
+	ErrConvertUnstructuredToStringFailed  = errors.New("ConvertUnstructuredToString failed")
+	ErrParamNotFoundInParameterAssignment = errors.New("unable to find non-meta paramater from TrialParameters in ParameterAssignment")
+	ErrParamNotFoundInTrialParameters     = errors.New("unable to find parameter from ParameterAssignment in TrialParameters")
+	ErrTemplatePathNotFound               = errors.New("TemplatePath not found in ConfigMap")
+)
+
 // Generator is the type for manifests Generator.
 type Generator interface {
 	InjectClient(c client.Client)
@@ -86,7 +96,7 @@ func (g *DefaultGenerator) GetRunSpecWithHyperParameters(experiment *experiments
 	// Convert Trial template to unstructured
 	runSpec, err := util.ConvertStringToUnstructured(replacedTemplate)
 	if err != nil {
-		return nil, fmt.Errorf("ConvertStringToUnstructured failed: %v", err)
+		return nil, fmt.Errorf("%w: %s", ErrConvertStringToUnstructuredFailed, err.Error())
 	}
 
 	// Set name and namespace for Run Spec
@@ -108,7 +118,7 @@ func (g *DefaultGenerator) applyParameters(experiment *experimentsv1beta1.Experi
 	if trialSpec == nil {
 		trialSpec, err = util.ConvertStringToUnstructured(trialTemplate)
 		if err != nil {
-			return "", fmt.Errorf("ConvertStringToUnstructured failed: %v", err)
+			return "", fmt.Errorf("%w: %s", ErrConvertStringToUnstructuredFailed, err.Error())
 		}
 	}
 
@@ -131,7 +141,7 @@ func (g *DefaultGenerator) applyParameters(experiment *experimentsv1beta1.Experi
 				nonMetaParamCount += 1
 				continue
 			} else {
-				return "", fmt.Errorf("Unable to find parameter: %v in parameter assignment %v", param.Reference, assignmentsMap)
+				return "", fmt.Errorf("%w: parameter: %v, parameter assignment: %v", ErrParamNotFoundInParameterAssignment, param.Reference, assignmentsMap)
 			}
 		}
 		metaRefKey = sub[1]
@@ -172,9 +182,10 @@ func (g *DefaultGenerator) applyParameters(experiment *experimentsv1beta1.Experi
 		}
 	}
 
-	// Number of parameters must be equal
+	// Number of assignment parameters must be equal to the number of non-meta trial parameters
+	// i.e. all parameters in ParameterAssignment must be in TrialParameters
 	if len(assignments) != nonMetaParamCount {
-		return "", fmt.Errorf("Number of TrialAssignment: %v != number of nonMetaTrialParameters in TrialSpec: %v", len(assignments), nonMetaParamCount)
+		return "", fmt.Errorf("%w: parameter assignments: %v, non-meta trial parameter count: %v", ErrParamNotFoundInTrialParameters, assignments, nonMetaParamCount)
 	}
 
 	// Replacing placeholders with parameter values
@@ -194,20 +205,20 @@ func (g *DefaultGenerator) GetTrialTemplate(instance *experimentsv1beta1.Experim
 	if trialSource.TrialSpec != nil {
 		trialTemplateString, err = util.ConvertUnstructuredToString(trialSource.TrialSpec)
 		if err != nil {
-			return "", fmt.Errorf("ConvertUnstructuredToString failed: %v", err)
+			return "", fmt.Errorf("%w: %s", ErrConvertUnstructuredToStringFailed, err.Error())
 		}
 	} else {
 		configMapNS := trialSource.ConfigMap.ConfigMapNamespace
 		configMapName := trialSource.ConfigMap.ConfigMapName
 		templatePath := trialSource.ConfigMap.TemplatePath
 		configMap, err := g.client.GetConfigMap(configMapName, configMapNS)
 		if err != nil {
-			return "", fmt.Errorf("GetConfigMap failed: %v", err)
+			return "", fmt.Errorf("%w: %v", ErrConfigMapNotFound, err)
 		}
 		var ok bool
 		trialTemplateString, ok = configMap[templatePath]
 		if !ok {
-			return "", fmt.Errorf("TemplatePath: %v not found in configMap: %v", templatePath, configMap)
+			return "", fmt.Errorf("%w: TemplatePath: %v, ConfigMap: %v", ErrTemplatePathNotFound, templatePath, configMap)
 		}
 	}
 

pkg/controller.v1beta1/experiment/manifest/generator_test.go

@@ -17,12 +17,12 @@ limitations under the License.
 package manifest
 
 import (
-	"errors"
 	"math"
-	"reflect"
 	"testing"
 
 	"github.com/golang/mock/gomock"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	batchv1 "k8s.io/api/batch/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -88,23 +88,18 @@ func TestGetRunSpecWithHP(t *testing.T) {
 		t.Errorf("ConvertObjectToUnstructured failed: %v", err)
 	}
 
-	tcs := []struct {
-		Instance             *experimentsv1beta1.Experiment
-		ParameterAssignments []commonapiv1beta1.ParameterAssignment
-		expectedRunSpec      *unstructured.Unstructured
-		Err                  bool
-		testDescription      string
+	cases := map[string]struct {
+		Instance                       *experimentsv1beta1.Experiment
+		ParameterAssignments           []commonapiv1beta1.ParameterAssignment
+		wantRunSpecWithHyperParameters *unstructured.Unstructured
+		wantError                      error
 	}{
-		// Valid run
-		{
-			Instance:             newFakeInstance(),
-			ParameterAssignments: newFakeParameterAssignment(),
-			expectedRunSpec:      expectedRunSpec,
-			Err:                  false,
-			testDescription:      "Run with valid parameters",
+		"Run with valid parameters": {
+			Instance:                       newFakeInstance(),
+			ParameterAssignments:           newFakeParameterAssignment(),
+			wantRunSpecWithHyperParameters: expectedRunSpec,
 		},
-		// Invalid JSON in unstructured
-		{
+		"Invalid JSON in Unstructured Trial template": {
 			Instance: func() *experimentsv1beta1.Experiment {
 				i := newFakeInstance()
 				trialSpec := i.Spec.TrialTemplate.TrialSource.TrialSpec
@@ -114,48 +109,45 @@ func TestGetRunSpecWithHP(t *testing.T) {
 				return i
 			}(),
 			ParameterAssignments: newFakeParameterAssignment(),
-			Err:                  true,
-			testDescription:      "Invalid JSON in Trial template",
+			wantError:            ErrConvertUnstructuredToStringFailed,
 		},
-		// len(parameterAssignment) != len(trialParameters)
-		{
+		"Non-meta parameter from TrialParameters not found in ParameterAssignment": {
 			Instance: newFakeInstance(),
 			ParameterAssignments: func() []commonapiv1beta1.ParameterAssignment {
 				pa := newFakeParameterAssignment()
-				pa = pa[1:]
+				pa[0] = commonapiv1beta1.ParameterAssignment{
+					Name:  "invalid-name",
+					Value: "invalid-value",
+				}
 				return pa
 			}(),
-			Err:             true,
-			testDescription: "Number of parameter assignments is not equal to number of Trial parameters",
+			wantError: ErrParamNotFoundInParameterAssignment,
 		},
-		// Parameter from assignments not found in Trial parameters
-		{
+		// case in which the lengths of trial parameters and parameter assignments are different
+		"Parameter from ParameterAssignment not found in TrialParameters": {
 			Instance: newFakeInstance(),
 			ParameterAssignments: func() []commonapiv1beta1.ParameterAssignment {
 				pa := newFakeParameterAssignment()
-				pa[0] = commonapiv1beta1.ParameterAssignment{
-					Name:  "invalid-name",
-					Value: "invalid-value",
-				}
+				pa = append(pa, commonapiv1beta1.ParameterAssignment{
+					Name:  "extra-name",
+					Value: "extra-value",
+				})
 				return pa
 			}(),
-			Err:             true,
-			testDescription: "Trial parameters don't have parameter from assignments",
+			wantError: ErrParamNotFoundInTrialParameters,
 		},
 	}
 
-	for _, tc := range tcs {
-		actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments)
-
-		if tc.Err && err == nil {
-			t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription)
-		} else if !tc.Err {
-			if err != nil {
-				t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err)
-			} else if !reflect.DeepEqual(tc.expectedRunSpec, actualRunSpec) {
-				t.Errorf("Case: %v failed. Expected %v\n got %v", tc.testDescription, tc.expectedRunSpec.Object, actualRunSpec.Object)
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments)
+			if diff := cmp.Diff(tc.wantError, err, cmpopts.EquateErrors()); len(diff) != 0 {
+				t.Errorf("Unexpected error from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
+			}
+			if diff := cmp.Diff(tc.wantRunSpecWithHyperParameters, got); len(diff) != 0 {
+				t.Errorf("Unexpected run spec from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
 			}
-		}
+		})
 	}
 }
 
@@ -208,7 +200,7 @@ spec:
 		map[string]string{templatePath: trialSpec}, nil)
 
 	invalidConfigMapName := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
-		nil, errors.New("Unable to get ConfigMap"))
+		nil, ErrConfigMapNotFound)
 
 	validGetConfigMap3 := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
 		map[string]string{templatePath: trialSpec}, nil)
@@ -245,18 +237,17 @@ spec:
 
 	expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr)
 	if err != nil {
-		t.Errorf("ConvertStringToUnstructured failed: %v", err)
+		t.Errorf("%v: %v", ErrConvertStringToUnstructuredFailed, err)
 	}
 
-	tcs := []struct {
-		Instance             *experimentsv1beta1.Experiment
-		ParameterAssignments []commonapiv1beta1.ParameterAssignment
-		Err                  bool
-		testDescription      string
+	cases := map[string]struct {
+		Instance                       *experimentsv1beta1.Experiment
+		ParameterAssignments           []commonapiv1beta1.ParameterAssignment
+		wantRunSpecWithHyperParameters *unstructured.Unstructured
+		wantError                      error
 	}{
-		// Valid run
 		// validGetConfigMap1 case
-		{
+		"Run with valid parameters": {
 			Instance: func() *experimentsv1beta1.Experiment {
 				i := newFakeInstance()
 				i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
@@ -268,13 +259,11 @@ spec:
 				}
 				return i
 			}(),
-			ParameterAssignments: newFakeParameterAssignment(),
-			Err:                  false,
-			testDescription:      "Run with valid parameters",
+			ParameterAssignments:           newFakeParameterAssignment(),
+			wantRunSpecWithHyperParameters: expectedRunSpec,
 		},
-		// Invalid ConfigMap name
 		// invalidConfigMapName case
-		{
+		"Invalid ConfigMap name": {
 			Instance: func() *experimentsv1beta1.Experiment {
 				i := newFakeInstance()
 				i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
@@ -285,12 +274,10 @@ spec:
 				return i
 			}(),
 			ParameterAssignments: newFakeParameterAssignment(),
-			Err:                  true,
-			testDescription:      "Invalid ConfigMap name",
+			wantError:            ErrConfigMapNotFound,
 		},
-		// Invalid template path in ConfigMap name
 		// validGetConfigMap3 case
-		{
+		"Invalid template path in ConfigMap name": {
 			Instance: func() *experimentsv1beta1.Experiment {
 				i := newFakeInstance()
 				i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
@@ -303,14 +290,12 @@ spec:
 				return i
 			}(),
 			ParameterAssignments: newFakeParameterAssignment(),
-			Err:                  true,
-			testDescription:      "Invalid template path in ConfigMap",
+			wantError:            ErrTemplatePathNotFound,
 		},
-		// Invalid Trial template spec in ConfigMap
+		// invalidTemplate case
 		// Trial template is a string in ConfigMap
 		// Because of that, user can specify not valid unstructured template
-		// invalidTemplate case
-		{
+		"Invalid trial spec in ConfigMap": {
 			Instance: func() *experimentsv1beta1.Experiment {
 				i := newFakeInstance()
 				i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
@@ -323,22 +308,20 @@ spec:
 				return i
 			}(),
 			ParameterAssignments: newFakeParameterAssignment(),
-			Err:                  true,
-			testDescription:      "Invalid Trial spec in ConfigMap",
+			wantError:            ErrConvertStringToUnstructuredFailed,
 		},
 	}
 
-	for _, tc := range tcs {
-		actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments)
-		if tc.Err && err == nil {
-			t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription)
-		} else if !tc.Err {
-			if err != nil {
-				t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err)
-			} else if !reflect.DeepEqual(expectedRunSpec, actualRunSpec) {
-				t.Errorf("Case: %v failed. Expected %v\n got %v", tc.testDescription, expectedRunSpec.Object, actualRunSpec.Object)
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments)
+			if diff := cmp.Diff(tc.wantError, err, cmpopts.EquateErrors()); len(diff) != 0 {
+				t.Errorf("Unexpected error from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
+			}
+			if diff := cmp.Diff(tc.wantRunSpecWithHyperParameters, got); len(diff) != 0 {
+				t.Errorf("Unexpected run spec from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
 			}
-		}
+		})
 	}
 }
 

pkg/controller.v1beta1/suggestion/composer/composer_test.go

@@ -22,12 +22,12 @@ import (
 	stdlog "log"
 	"os"
 	"path/filepath"
-	"reflect"
 	"strings"
 	"sync"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/onsi/gomega"
 	"github.com/spf13/viper"
 	appsv1 "k8s.io/api/apps/v1"
@@ -631,8 +631,8 @@ func TestDesiredRBAC(t *testing.T) {
 func metaEqual(expected, actual metav1.ObjectMeta) bool {
 	return expected.Name == actual.Name &&
 		expected.Namespace == actual.Namespace &&
-		reflect.DeepEqual(expected.Labels, actual.Labels) &&
-		reflect.DeepEqual(expected.Annotations, actual.Annotations) &&
+		cmp.Equal(expected.Labels, actual.Labels) &&
+		cmp.Equal(expected.Annotations, actual.Annotations) &&
 		(len(actual.OwnerReferences) > 0 &&
 			expected.OwnerReferences[0].APIVersion == actual.OwnerReferences[0].APIVersion &&
 			expected.OwnerReferences[0].Kind == actual.OwnerReferences[0].Kind &&

pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient_test.go

@@ -19,11 +19,11 @@ package suggestionclient
 import (
 	"errors"
 	"fmt"
-	"reflect"
 	"testing"
 	"time"
 
 	"github.com/golang/mock/gomock"
+	"github.com/google/go-cmp/cmp"
 	"github.com/onsi/gomega"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
@@ -578,8 +578,8 @@ func TestConvertTrialObservation(t *testing.T) {
 	}
 	for _, tc := range tcs {
 		actualObservation := convertTrialObservation(tc.strategies, tc.inObservation)
-		if !reflect.DeepEqual(actualObservation, tc.expectedObservation) {
-			t.Errorf("Case: %v failed.\nExpected observation: %v \ngot: %v", tc.testDescription, tc.expectedObservation, actualObservation)
+		if diff := cmp.Diff(actualObservation, tc.expectedObservation); diff != "" {
+			t.Errorf("Case: %v failed. Diff (-want,+got):\n%s", tc.testDescription, diff)
 		}
 	}
 }