From 1f497d324fa2bc2c530b9b948465b3cf1dabb483 Mon Sep 17 00:00:00 2001 From: xushiwei 00425595 Date: Mon, 3 Sep 2018 20:29:25 +0800 Subject: [PATCH] address comments --- pkg/connection/connection_test.go | 93 +++++++++++++++++++------- pkg/controller/framework_test.go | 24 +++---- pkg/controller/snapshot_create_test.go | 4 +- 3 files changed, 81 insertions(+), 40 deletions(-) diff --git a/pkg/connection/connection_test.go b/pkg/connection/connection_test.go index a6aafd6c..2f88f715 100644 --- a/pkg/connection/connection_test.go +++ b/pkg/connection/connection_test.go @@ -127,10 +127,11 @@ func TestGetPluginInfo(t *testing.T) { func TestSupportsControllerCreateSnapshot(t *testing.T) { tests := []struct { - name string - output *csi.ControllerGetCapabilitiesResponse - injectError bool - expectError bool + name string + output *csi.ControllerGetCapabilitiesResponse + injectError bool + expectError bool + expectResult bool }{ { name: "success", @@ -152,13 +153,15 @@ func TestSupportsControllerCreateSnapshot(t *testing.T) { }, }, }, - expectError: false, + expectError: false, + expectResult: true, }, { - name: "gRPC error", - output: nil, - injectError: true, - expectError: true, + name: "gRPC error", + output: nil, + injectError: true, + expectError: true, + expectResult: false, }, { name: "no create snapshot", @@ -173,7 +176,8 @@ func TestSupportsControllerCreateSnapshot(t *testing.T) { }, }, }, - expectError: false, + expectError: false, + expectResult: false, }, { name: "empty capability", @@ -184,14 +188,16 @@ func TestSupportsControllerCreateSnapshot(t *testing.T) { }, }, }, - expectError: false, + expectError: false, + expectResult: false, }, { name: "no capabilities", output: &csi.ControllerGetCapabilitiesResponse{ Capabilities: []*csi.ControllerServiceCapability{}, }, - expectError: false, + expectError: false, + expectResult: false, }, } @@ -216,22 +222,26 @@ func TestSupportsControllerCreateSnapshot(t *testing.T) { // Setup expectation controllerServer.EXPECT().ControllerGetCapabilities(gomock.Any(), in).Return(out, injectedErr).Times(1) - _, err = csiConn.SupportsControllerCreateSnapshot(context.Background()) + ok, err := csiConn.SupportsControllerCreateSnapshot(context.Background()) if test.expectError && err == nil { t.Errorf("test %q: Expected error, got none", test.name) } if !test.expectError && err != nil { t.Errorf("test %q: got error: %v", test.name, err) } + if err == nil && test.expectResult != ok { + t.Errorf("test fail expected result %t but got %t\n", test.expectResult, ok) + } } } func TestSupportsControllerListSnapshots(t *testing.T) { tests := []struct { - name string - output *csi.ControllerGetCapabilitiesResponse - injectError bool - expectError bool + name string + output *csi.ControllerGetCapabilitiesResponse + injectError bool + expectError bool + expectResult bool }{ { name: "success", @@ -260,13 +270,38 @@ func TestSupportsControllerListSnapshots(t *testing.T) { }, }, }, - expectError: false, + expectError: false, + expectResult: true, }, { - name: "gRPC error", - output: nil, - injectError: true, - expectError: true, + name: "have create_delete_snapshot but no list snapshot ", + output: &csi.ControllerGetCapabilitiesResponse{ + Capabilities: []*csi.ControllerServiceCapability{ + { + Type: &csi.ControllerServiceCapability_Rpc{ + Rpc: &csi.ControllerServiceCapability_RPC{ + Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, + }, + }, + }, + { + Type: &csi.ControllerServiceCapability_Rpc{ + Rpc: &csi.ControllerServiceCapability_RPC{ + Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT, + }, + }, + }, + }, + }, + expectError: false, + expectResult: false, + }, + { + name: "gRPC error", + output: nil, + injectError: true, + expectError: true, + expectResult: false, }, { name: "no list snapshot", @@ -281,7 +316,8 @@ func TestSupportsControllerListSnapshots(t *testing.T) { }, }, }, - expectError: false, + expectError: false, + expectResult: false, }, { name: "empty capability", @@ -292,14 +328,16 @@ func TestSupportsControllerListSnapshots(t *testing.T) { }, }, }, - expectError: false, + expectError: false, + expectResult: false, }, { name: "no capabilities", output: &csi.ControllerGetCapabilitiesResponse{ Capabilities: []*csi.ControllerServiceCapability{}, }, - expectError: false, + expectError: false, + expectResult: false, }, } @@ -324,13 +362,16 @@ func TestSupportsControllerListSnapshots(t *testing.T) { // Setup expectation controllerServer.EXPECT().ControllerGetCapabilities(gomock.Any(), in).Return(out, injectedErr).Times(1) - _, err = csiConn.SupportsControllerListSnapshots(context.Background()) + ok, err := csiConn.SupportsControllerListSnapshots(context.Background()) if test.expectError && err == nil { t.Errorf("test %q: Expected error, got none", test.name) } if !test.expectError && err != nil { t.Errorf("test %q: got error: %v", test.name, err) } + if err == nil && test.expectResult != ok { + t.Errorf("test fail expected result %t but got %t\n", test.expectResult, ok) + } } } diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go index 8e3c8074..5d9311d6 100644 --- a/pkg/controller/framework_test.go +++ b/pkg/controller/framework_test.go @@ -195,14 +195,14 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O // check the content does not exist _, found := r.contents[content.Name] if found { - return true, nil, fmt.Errorf("Cannot create content %s: content already exists", content.Name) + return true, nil, fmt.Errorf("cannot create content %s: content already exists", content.Name) } // Store the updated object to appropriate places. r.contents[content.Name] = content r.changedObjects = append(r.changedObjects, content) r.changedSinceLastSync++ - glog.V(4).Infof("created content %s", content.Name) + glog.V(5).Infof("created content %s", content.Name) return true, content, nil case action.Matches("update", "volumesnapshotcontents"): @@ -221,7 +221,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O content = content.DeepCopy() content.ResourceVersion = strconv.Itoa(storedVer + 1) } else { - return true, nil, fmt.Errorf("Cannot update content %s: content not found", content.Name) + return true, nil, fmt.Errorf("cannot update content %s: content not found", content.Name) } // Store the updated object to appropriate places. @@ -247,7 +247,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O snapshot = snapshot.DeepCopy() snapshot.ResourceVersion = strconv.Itoa(storedVer + 1) } else { - return true, nil, fmt.Errorf("Cannot update snapshot %s: snapshot not found", snapshot.Name) + return true, nil, fmt.Errorf("cannot update snapshot %s: snapshot not found", snapshot.Name) } // Store the updated object to appropriate places. @@ -265,7 +265,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O return true, content, nil } else { glog.V(4).Infof("GetVolume: content %s not found", name) - return true, nil, fmt.Errorf("Cannot find content %s", name) + return true, nil, fmt.Errorf("cannot find content %s", name) } case action.Matches("get", "volumesnapshots"): @@ -276,7 +276,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O return true, snapshot, nil } else { glog.V(4).Infof("GetSnapshot: content %s not found", name) - return true, nil, fmt.Errorf("Cannot find snapshot %s", name) + return true, nil, fmt.Errorf("cannot find snapshot %s", name) } case action.Matches("delete", "volumesnapshotcontents"): @@ -288,7 +288,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O r.changedSinceLastSync++ return true, nil, nil } else { - return true, nil, fmt.Errorf("Cannot delete content %s: not found", name) + return true, nil, fmt.Errorf("cannot delete content %s: not found", name) } case action.Matches("delete", "volumesnapshots"): @@ -300,7 +300,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O r.changedSinceLastSync++ return true, nil, nil } else { - return true, nil, fmt.Errorf("Cannot delete snapshot %s: not found", name) + return true, nil, fmt.Errorf("cannot delete snapshot %s: not found", name) } case action.Matches("get", "persistentvolumes"): @@ -311,7 +311,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O return true, volume, nil } else { glog.V(4).Infof("GetVolume: volume %s not found", name) - return true, nil, fmt.Errorf("Cannot find volume %s", name) + return true, nil, fmt.Errorf("cannot find volume %s", name) } case action.Matches("get", "persistentvolumeclaims"): @@ -322,7 +322,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O return true, claim, nil } else { glog.V(4).Infof("GetClaim: claim %s not found", name) - return true, nil, fmt.Errorf("Cannot find claim %s", name) + return true, nil, fmt.Errorf("cannot find claim %s", name) } case action.Matches("get", "storageclasses"): @@ -333,7 +333,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O return true, storageClass, nil } else { glog.V(4).Infof("GetStorageClass: storageClass %s not found", name) - return true, nil, fmt.Errorf("Cannot find storageClass %s", name) + return true, nil, fmt.Errorf("cannot find storageClass %s", name) } case action.Matches("get", "secrets"): @@ -344,7 +344,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O return true, secret, nil } else { glog.V(4).Infof("GetSecret: secret %s not found", name) - return true, nil, fmt.Errorf("Cannot find secret %s", name) + return true, nil, fmt.Errorf("cannot find secret %s", name) } } diff --git a/pkg/controller/snapshot_create_test.go b/pkg/controller/snapshot_create_test.go index cad13957..57b8be80 100644 --- a/pkg/controller/snapshot_create_test.go +++ b/pkg/controller/snapshot_create_test.go @@ -271,7 +271,7 @@ func TestCreateSnapshotSync(t *testing.T) { initialContents: nocontents, expectedContents: nocontents, initialSnapshots: newSnapshotArray("snap7-4", classGold, "", "snapuid7-4", "claim7-4", false, nil, nil, nil), - expectedSnapshots: newSnapshotArray("snap7-4", classGold, "", "snapuid7-4", "claim7-4", false, newVolumeError("Failed to create snapshot: failed to retrieve PVC claim7-4 from the API server: \"Cannot find claim claim7-4\""), nil, nil), + expectedSnapshots: newSnapshotArray("snap7-4", classGold, "", "snapuid7-4", "claim7-4", false, newVolumeError("Failed to create snapshot: failed to retrieve PVC claim7-4 from the API server: \"cannot find claim claim7-4\""), nil, nil), initialVolumes: newVolumeArray("volume7-4", "pv-uid7-4", "pv-handle7-4", "1Gi", "pvc-uid7-4", "claim7-4", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), expectedEvents: []string{"Warning SnapshotCreationFailed"}, errors: noerrors, @@ -282,7 +282,7 @@ func TestCreateSnapshotSync(t *testing.T) { initialContents: nocontents, expectedContents: nocontents, initialSnapshots: newSnapshotArray("snap7-5", classGold, "", "snapuid7-5", "claim7-5", false, nil, nil, nil), - expectedSnapshots: newSnapshotArray("snap7-5", classGold, "", "snapuid7-5", "claim7-5", false, newVolumeError("Failed to create snapshot: failed to retrieve PV volume7-5 from the API server: \"Cannot find volume volume7-5\""), nil, nil), + expectedSnapshots: newSnapshotArray("snap7-5", classGold, "", "snapuid7-5", "claim7-5", false, newVolumeError("Failed to create snapshot: failed to retrieve PV volume7-5 from the API server: \"cannot find volume volume7-5\""), nil, nil), initialClaims: newClaimArray("claim7-5", "pvc-uid7-5", "1Gi", "volume7-5", v1.ClaimBound, &classEmpty), expectedEvents: []string{"Warning SnapshotCreationFailed"}, errors: noerrors,