volume: improve test coverage and minor refactoring

The volume package does not test enough use-cases.

Improve test coverage by adding additional tests and refactoring
current tests to use table testing.

This change introduces a new error var to make testing unsupported
volume type errors easier.

This change does not introduce any changes in behavior.

pkg/volume/volume.go

@@ -24,16 +24,18 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
-// All volume types are expected to implement this interface
+var ErrUnsupportedVolumeType = errors.New("unsupported volume type")
+
+// Interface is a directory used by pods or hosts. Interface implementations
+// must be idempotent.
 type Interface interface {
-	// Prepares and mounts/unpacks the volume to a directory path.
+	// SetUp prepares and mounts/unpacks the volume to a directory path.
-	// This procedure must be idempotent.
-	// TODO(jonesdl) SetUp should return an error if it fails.
 	SetUp() error
-	// Returns the directory path the volume is mounted to.
+
+	// GetPath returns the directory path the volume is mounted to.
 	GetPath() string
-	// Unmounts the volume and removes traces of the SetUp procedure.
+
-	// This procedure must be idempotent.
+	// TearDown unmounts the volume and removes traces of the SetUp procedure.
 	TearDown() error
 }
 
@@ -112,7 +114,7 @@ func CreateVolume(volume *api.Volume, podID string, rootDir string) (Interface,
 	} else if source.EmptyDirectory != nil {
 		vol = createEmptyDirectory(volume, podID, rootDir)
 	} else {
-		return nil, errors.New("Unsupported volume type.")
+		return nil, ErrUnsupportedVolumeType
 	}
 	return vol, nil
 }

pkg/volume/volume_test.go

@@ -17,35 +17,84 @@ limitations under the License.
 package volume
 
 import (
+	"io/ioutil"
+	"os"
+	"path"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
 func TestCreateVolumes(t *testing.T) {
-	volumes := []api.Volume{
+	tempDir, err := ioutil.TempDir("", "CreateVolumes")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	defer os.RemoveAll(tempDir)
+	createVolumesTests := []struct {
+		volume api.Volume
+		path   string
+		podID  string
+	}{
 		{
-			Name: "host-dir",
+			api.Volume{
-			Source: &api.VolumeSource{
+				Name: "host-dir",
-				HostDirectory: &api.HostDirectory{"/dir/path"},
+				Source: &api.VolumeSource{
+					HostDirectory: &api.HostDirectory{"/dir/path"},
+				},
 			},
+			"/dir/path",
+			"my-id",
 		},
 		{
-			Name: "empty-dir",
+			api.Volume{
-			Source: &api.VolumeSource{
+				Name: "empty-dir",
-				EmptyDirectory: &api.EmptyDirectory{},
+				Source: &api.VolumeSource{
+					EmptyDirectory: &api.EmptyDirectory{},
+				},
 			},
+			path.Join(tempDir, "/my-id/volumes/empty/empty-dir"),
+			"my-id",
+		},
+		{api.Volume{}, "", ""},
+		{
+			api.Volume{
+				Name:   "empty-dir",
+				Source: &api.VolumeSource{},
+			},
+			"",
+			"",
 		},
 	}
-	fakePodID := "my-id"
+	for _, createVolumesTest := range createVolumesTests {
-	rootDir := "/var/lib/kubelet"
+		tt := createVolumesTest
-	expectedPaths := []string{"/dir/path", "/var/lib/kubelet/my-id/volumes/empty/empty-dir"}
+		v, err := CreateVolume(&tt.volume, tt.podID, tempDir)
-	for i, volume := range volumes {
+		if tt.volume.Source == nil {
-		extVolume, _ := CreateVolume(&volume, fakePodID, rootDir)
+			if v != nil {
-		expectedPath := expectedPaths[i]
+				t.Errorf("Expected volume to be nil")
-		path := extVolume.GetPath()
+			}
-		if expectedPath != path {
+			continue
-			t.Errorf("Unexpected bind path. Expected %v, got %v", expectedPath, path)
+		}
+		if tt.volume.Source.HostDirectory == nil && tt.volume.Source.EmptyDirectory == nil {
+			if err != ErrUnsupportedVolumeType {
+				t.Errorf("Unexpected error: %v", err)
+			}
+			continue
+		}
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		err = v.SetUp()
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		path := v.GetPath()
+		if path != tt.path {
+			t.Errorf("Unexpected bind path. Expected %v, got %v", tt.path, path)
+		}
+		err = v.TearDown()
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
 		}
 	}
 }