Skip to content

Commit

Permalink
Add more test cases for Generator (#1216)
Browse files Browse the repository at this point in the history 
* Init commit

* Add test to generator for unstructured

* Add test to generator for configmap

* Fix log

* Add comment
  • Loading branch information
@andreyvelich
andreyvelich authored Jun 16, 2020
1 parent 94ae58f commit 0be190a
Showing 1 changed file with 214 additions and 47 deletions.
261 changes: 214 additions & 47 deletions pkg/controller.v1beta1/experiment/manifest/generator_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package manifest

import (
"errors"
"math"
"reflect"
"testing"

Expand All @@ -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()
Expand All @@ -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",
Expand Down Expand Up @@ -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)
}
}
}
}

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
}
}
}
}

Expand Down Expand Up @@ -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",
},
}
}

0 comments on commit 0be190a

Please sign in to comment.