Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/operator/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ func TestGetProviderFromInfrastructure(t *testing.T) {
},
},
expected: kubemarkPlatform,
}, {
infra: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
Platform: configv1.VSpherePlatformType,
},
},
expected: configv1.VSpherePlatformType,
}, {
infra: &configv1.Infrastructure{
Status: configv1.InfrastructureStatus{
Expand Down Expand Up @@ -132,6 +139,10 @@ func TestGetProviderControllerFromImages(t *testing.T) {
provider: kubemarkPlatform,
expectedImage: clusterAPIControllerKubemark,
},
{
provider: configv1.VSpherePlatformType,
expectedImage: clusterAPIControllerNoOp,
},
{
provider: configv1.NonePlatformType,
expectedImage: clusterAPIControllerNoOp,
Expand Down
127 changes: 127 additions & 0 deletions pkg/operator/operator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
package operator

import (
"encoding/json"
"io/ioutil"
"os"
"path/filepath"
"testing"

configv1 "github.com/openshift/api/config/v1"
fakeconfigclientset "github.com/openshift/client-go/config/clientset/versioned/fake"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/record"
)

// TestOperatorSync_NoOp tests syncing to ensure that the mao reports available
// for platforms that are no-ops.
func TestOperatorSync_NoOp(t *testing.T) {
cases := []struct {
platform configv1.PlatformType
expectedNoop bool
}{
{
platform: configv1.AWSPlatformType,
expectedNoop: false,
},
{
platform: configv1.LibvirtPlatformType,
expectedNoop: false,
},
{
platform: configv1.OpenStackPlatformType,
expectedNoop: false,
},
{
platform: configv1.AzurePlatformType,
expectedNoop: false,
},
{
platform: bareMetalPlatform,
expectedNoop: false,
},
{
platform: kubemarkPlatform,
expectedNoop: false,
},
{
platform: configv1.VSpherePlatformType,
expectedNoop: true,
},
{
platform: configv1.NonePlatformType,
expectedNoop: true,
},
{
platform: "bad-platform",
expectedNoop: true,
},
}

tempDir, err := ioutil.TempDir("", "TestOperatorSync")
if err != nil {
t.Fatalf("could not create the temp dir: %v", err)
}
defer os.RemoveAll(tempDir)

images := Images{
MachineAPIOperator: "test-mao",
}
imagesAsJSON, err := json.Marshal(images)
if err != nil {
t.Fatalf("failed to marshal images: %v", err)
}
imagesFilePath := filepath.Join(tempDir, "test-images.json")
if err := ioutil.WriteFile(imagesFilePath, imagesAsJSON, 0666); err != nil {
t.Fatalf("could not write the images file: %v", err)
}

for _, tc := range cases {
t.Run(string(tc.platform), func(t *testing.T) {
infra := &configv1.Infrastructure{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster",
},
Status: configv1.InfrastructureStatus{
Platform: tc.platform,
},
}

optr := Operator{
eventRecorder: record.NewFakeRecorder(5),
osClient: fakeconfigclientset.NewSimpleClientset(infra),
imagesFile: imagesFilePath,
}

err = optr.sync("test-key")
if !tc.expectedNoop {
if !assert.Error(t, err, "unexpected sync success") {
t.Fatal()
Copy link
Member

@enxebre enxebre Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why Fatal? from the docs: "it will call failNow and stops function execution", why not just asset.Error so the loop keep going and we output for each case. Same for all below this line
cc @frobware

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With sub-tests, the call to Fatal will only fail the one sub-test. The rest of the sub-tests will still execute.

Copy link
Member

@enxebre enxebre Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so what's the difference over just if !tc.expectedNoop { assert.Error(t, err, "unexpected sync success")}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that with your code, the rest of the sub-test will be executed. So there will be more test errors later on in the sub-test when it comes to testing the conditions set on the clusteroperator. That is not terribly; it does make the test results harder to analyze due to more noise, however.

I am fine with removing the call to Fatal here, if you prefer. The call to Fatal at

o, err := optr.osClient.ConfigV1().ClusterOperators().Get(clusterOperatorName, metav1.GetOptions{})
if !assert.NoError(t, err, "failed to get clusteroperator") {
t.Fatal()
}
needs to stay to prevent a panic when the clusteroperator does not exist.

}
} else {
if !assert.NoError(t, err, "unexpected sync failure") {
t.Fatal()
}
}

o, err := optr.osClient.ConfigV1().ClusterOperators().Get(clusterOperatorName, metav1.GetOptions{})
if !assert.NoError(t, err, "failed to get clusteroperator") {
t.Fatal()
}
expectedConditions := map[configv1.ClusterStatusConditionType]configv1.ConditionStatus{
configv1.OperatorAvailable: configv1.ConditionTrue,
configv1.OperatorProgressing: configv1.ConditionFalse,
configv1.OperatorFailing: configv1.ConditionTrue,
}
if tc.expectedNoop {
expectedConditions[configv1.OperatorFailing] = configv1.ConditionFalse
}
actualConditions := map[configv1.ClusterStatusConditionType]configv1.ConditionStatus{}
for _, c := range o.Status.Conditions {
actualConditions[c.Type] = c.Status
}
assert.Equal(t, expectedConditions, actualConditions, "unexpected clusteroperator conditions")
})
}
}