diff --git a/pkg/controller.v1beta1/experiment/manifest/generator_test.go b/pkg/controller.v1beta1/experiment/manifest/generator_test.go index a0fe320631c..df687c53eb4 100644 --- a/pkg/controller.v1beta1/experiment/manifest/generator_test.go +++ b/pkg/controller.v1beta1/experiment/manifest/generator_test.go @@ -1,6 +1,8 @@ package manifest import ( + "errors" + "math" "reflect" "testing" @@ -12,10 +14,10 @@ import ( batchv1 "k8s.io/api/batch/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestGetRunSpecWithHP(t *testing.T) { - tc := newFakeInstance() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() @@ -26,22 +28,6 @@ func TestGetRunSpecWithHP(t *testing.T) { client: c, } - // TODO: Add more test cases - actual, err := p.GetRunSpecWithHyperParameters(tc, "trial-name", "trial-namespace", []commonapiv1beta1.ParameterAssignment{ - { - Name: "lr", - Value: "0.05", - }, - { - Name: "num-layers", - Value: "5", - }, - }) - - if err != nil { - t.Errorf("Expected nil, got %v", err) - } - expectedJob := batchv1.Job{ TypeMeta: metav1.TypeMeta{ APIVersion: "batch/v1", @@ -71,13 +57,79 @@ func TestGetRunSpecWithHP(t *testing.T) { }, } - expected, err := util.ConvertObjectToUnstructured(expectedJob) + expectedRunSpec, err := util.ConvertObjectToUnstructured(expectedJob) if err != nil { t.Errorf("ConvertObjectToUnstructured failed: %v", err) } - if !reflect.DeepEqual(expected.Object, actual.Object) { - t.Errorf("Expected %v\n got %v", expected.Object, actual.Object) + tcs := []struct { + Instance *experimentsv1beta1.Experiment + ParameterAssignments []commonapiv1beta1.ParameterAssignment + expectedRunSpec *unstructured.Unstructured + Err bool + testDescription string + }{ + // Valid run + { + Instance: newFakeInstance(), + ParameterAssignments: newFakeParameterAssignment(), + expectedRunSpec: expectedRunSpec, + Err: false, + testDescription: "Run with valid parameters", + }, + // Invalid JSON in unstructured + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + trialSpec := i.Spec.TrialTemplate.TrialSource.TrialSpec + trialSpec.Object = map[string]interface{}{ + "invalidJSON": math.NaN(), + } + return i + }(), + ParameterAssignments: newFakeParameterAssignment(), + Err: true, + testDescription: "Invalid JSON in Trial template", + }, + // len(parameterAssignment) != len(trialParameters) + { + Instance: newFakeInstance(), + ParameterAssignments: func() []commonapiv1beta1.ParameterAssignment { + pa := newFakeParameterAssignment() + pa = pa[1:] + return pa + }(), + Err: true, + testDescription: "Number of parameter assignments is not equal to number of Trial parameters", + }, + // Parameter from assignments not found in Trial paramters + { + Instance: newFakeInstance(), + ParameterAssignments: func() []commonapiv1beta1.ParameterAssignment { + pa := newFakeParameterAssignment() + pa[0] = commonapiv1beta1.ParameterAssignment{ + Name: "invalid-name", + Value: "invalid-value", + } + return pa + }(), + Err: true, + testDescription: "Trial parameters don't have parameter from assignments", + }, + } + + 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) + } + } } } @@ -91,7 +143,7 @@ func TestGetRunSpecWithHPConfigMap(t *testing.T) { client: c, } - templatePath := "trial-template.yaml" + templatePath := "trial-template-path" trialSpec := `apiVersion: batch/v1 kind: Job @@ -107,30 +159,42 @@ spec: - "--lr=${trialParameters.learningRate}" - "--num-layers=${trialParameters.numberLayers}"` - c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(map[string]string{ - templatePath: trialSpec, - }, nil) + invalidTrialSpec := `apiVersion: batch/v1 +kind: Job +spec: + template: + spec: + containers: + - name: training-container + image: docker.io/kubeflowkatib/mxnet-mnist + command: + - python3 + - /opt/mxnet-mnist/mnist.py + - --lr=${trialParameters.learningRate} + - --num-layers=${trialParameters.numberLayers} + - --invalidParameter={'num_layers': 2, 'input_sizes': [32, 32, 3]}` - instance := newFakeInstance() - instance.Spec.TrialTemplate.TrialSource.ConfigMap = &experimentsv1beta1.ConfigMapSource{ - TemplatePath: templatePath, - } - instance.Spec.TrialTemplate.TrialSource.TrialSpec = nil - actual, err := p.GetRunSpecWithHyperParameters(instance, "trial-name", "trial-namespace", []commonapiv1beta1.ParameterAssignment{ - { - Name: "lr", - Value: "0.05", - }, - { - Name: "num-layers", - Value: "5", - }, - }) - if err != nil { - t.Errorf("Expected nil, got %v", err) - } - // We can't compare structures, because trialSpec is a string and creationTimestamp was not added - expectedJob := `apiVersion: batch/v1 + validGetConfigMap1 := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return( + map[string]string{templatePath: trialSpec}, nil) + + invalidConfigMapName := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return( + nil, errors.New("Unable to get ConfigMap")) + + validGetConfigMap3 := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return( + map[string]string{templatePath: trialSpec}, nil) + + invalidTemplate := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return( + map[string]string{templatePath: invalidTrialSpec}, nil) + + gomock.InOrder( + validGetConfigMap1, + invalidConfigMapName, + validGetConfigMap3, + invalidTemplate, + ) + + // We can't compare structures, because in ConfigMap trialSpec is a string and creationTimestamp was not added + expectedStr := `apiVersion: batch/v1 kind: Job metadata: name: trial-name @@ -147,12 +211,102 @@ spec: - "--lr=0.05" - "--num-layers=5"` - expected, err := util.ConvertStringToUnstructured(expectedJob) + expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr) if err != nil { t.Errorf("ConvertStringToUnstructured failed: %v", err) } - if !reflect.DeepEqual(expected, actual) { - t.Errorf("Expected %s\n got %s", expected.Object, actual.Object) + + tcs := []struct { + Instance *experimentsv1beta1.Experiment + ParameterAssignments []commonapiv1beta1.ParameterAssignment + Err bool + testDescription string + }{ + // Valid run + // validGetConfigMap1 case + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{ + ConfigMap: &experimentsv1beta1.ConfigMapSource{ + ConfigMapName: "config-map-name", + ConfigMapNamespace: "config-map-namespace", + TemplatePath: "trial-template-path", + }, + } + return i + }(), + ParameterAssignments: newFakeParameterAssignment(), + Err: false, + testDescription: "Run with valid parameters", + }, + // Invalid ConfigMap name + // invalidConfigMapName case + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{ + ConfigMap: &experimentsv1beta1.ConfigMapSource{ + ConfigMapName: "invalid-name", + }, + } + return i + }(), + ParameterAssignments: newFakeParameterAssignment(), + Err: true, + testDescription: "Invalid ConfigMap name", + }, + // Invalid template path in ConfigMap name + // validGetConfigMap3 case + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{ + ConfigMap: &experimentsv1beta1.ConfigMapSource{ + ConfigMapName: "config-map-name", + ConfigMapNamespace: "config-map-namespace", + TemplatePath: "invalid-path", + }, + } + return i + }(), + ParameterAssignments: newFakeParameterAssignment(), + Err: true, + testDescription: "Invalid template path in ConfigMap", + }, + // Invalid Trial template spec in ConfigMap + // Trial template is a string in ConfigMap + // Because of that, user can specify not valid unstructured template + // invalidTemplate case + { + Instance: func() *experimentsv1beta1.Experiment { + i := newFakeInstance() + i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{ + ConfigMap: &experimentsv1beta1.ConfigMapSource{ + ConfigMapName: "config-map-name", + ConfigMapNamespace: "config-map-namespace", + TemplatePath: "trial-template-path", + }, + } + return i + }(), + ParameterAssignments: newFakeParameterAssignment(), + Err: true, + testDescription: "Invalid Trial spec in ConfigMap", + }, + } + + 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) + } + } } } @@ -206,3 +360,16 @@ func newFakeInstance() *experimentsv1beta1.Experiment { }, } } + +func newFakeParameterAssignment() []commonapiv1beta1.ParameterAssignment { + return []commonapiv1beta1.ParameterAssignment{ + { + Name: "lr", + Value: "0.05", + }, + { + Name: "num-layers", + Value: "5", + }, + } +}