add comments to validation testcases, and use const in util.go

pkg/api/persistentvolumeclaim/util.go

@@ -22,6 +22,11 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 )
 
+const (
+	pvc            string = "PersistentVolumeClaim"
+	volumeSnapshot string = "VolumeSnapshot"
+)
+
 // DropDisabledFields removes disabled fields from the pvc spec.
 // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pvc spec.
 func DropDisabledFields(pvcSpec, oldPVCSpec *core.PersistentVolumeClaimSpec) {
@@ -55,13 +60,13 @@ func dataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool {
 
 func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool {
 	if pvcSpec.DataSource != nil {
-		if pvcSpec.DataSource.Kind == "PersistentVolumeClaim" &&
+		if pvcSpec.DataSource.Kind == pvc &&
 			*pvcSpec.DataSource.APIGroup == "" &&
 			utilfeature.DefaultFeatureGate.Enabled(features.VolumePVCDataSource) {
 			return true
 
 		}
-		if pvcSpec.DataSource.Kind == "VolumeSnapshot" &&
+		if pvcSpec.DataSource.Kind == volumeSnapshot &&
 			*pvcSpec.DataSource.APIGroup == "snapshot.storage.k8s.io" &&
 			utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) {
 			return true

pkg/apis/core/validation/validation_test.go

@@ -13312,24 +13312,48 @@ func testDataSourceInSpec(name string, kind string, apiGroup string) *core.Persi
 }
 
 func TestAlphaVolumePVCDataSource(t *testing.T) {
-	successTestCases := []core.PersistentVolumeClaimSpec{
-		*testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "snapshot.storage.k8s.io"),
-		*testDataSourceInSpec("test_pvc", "PersistentVolumeClaim", ""),
-	}
-	failedTestCases := []core.PersistentVolumeClaimSpec{
-		*testDataSourceInSpec("", "VolumeSnapshot", "snapshot.storage.k8s.io"),
-		*testDataSourceInSpec("test_snapshot", "PersistentVolumeClaim", "snapshot.storage.k8s.io"),
-		*testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "storage.k8s.io"),
-	}
 
-	for _, tc := range successTestCases {
-		if errs := ValidatePersistentVolumeClaimSpec(&tc, field.NewPath("spec")); len(errs) != 0 {
-			t.Errorf("expected success: %v", errs)
-		}
+	testCases := []struct {
+		testName     string
+		claimSpec    core.PersistentVolumeClaimSpec
+		expectedFail bool
+	}{
+		{
+			testName:  "test create from valid snapshot source",
+			claimSpec: *testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "snapshot.storage.k8s.io"),
+		},
+		{
+			testName:  "test create from valid pvc source",
+			claimSpec: *testDataSourceInSpec("test_pvc", "PersistentVolumeClaim", ""),
+		},
+		{
+			testName:     "test missing name in snapshot datasource should fail",
+			claimSpec:    *testDataSourceInSpec("", "VolumeSnapshot", "snapshot.storage.k8s.io"),
+			expectedFail: true,
+		},
+		{
+			testName:     "test specifying pvc with snapshot api group should fail",
+			claimSpec:    *testDataSourceInSpec("test_snapshot", "PersistentVolumeClaim", "snapshot.storage.k8s.io"),
+			expectedFail: true,
+		},
+		{
+			testName:     "test invalid group name in snapshot datasource should fail",
+			claimSpec:    *testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "storage.k8s.io"),
+			expectedFail: true,
+		},
 	}
-	for _, tc := range failedTestCases {
-		if errs := ValidatePersistentVolumeClaimSpec(&tc, field.NewPath("spec")); len(errs) == 0 {
-			t.Errorf("expected failure: %v", errs)
+
+	for _, tc := range testCases {
+		if tc.expectedFail {
+			if errs := ValidatePersistentVolumeClaimSpec(&tc.claimSpec, field.NewPath("spec")); len(errs) == 0 {
+				t.Errorf("expected failure: %v", errs)
+			}
+
+		} else {
+			if errs := ValidatePersistentVolumeClaimSpec(&tc.claimSpec, field.NewPath("spec")); len(errs) != 0 {
+				t.Errorf("expected success: %v", errs)
+			}
+
 		}
 	}
 }