Merge pull request #2374 from dgageot/more-vb-tests

Add more tests to the VirtualBox driver

drivers/virtualbox/disk.go

@@ -2,7 +2,6 @@ package virtualbox
 
 import (
 	"bufio"
-	"io"
 	"strings"
 )
 
@@ -11,28 +10,31 @@ type VirtualDisk struct {
 	Path string
 }
 
-func (d *Driver) getVMDiskInfo(name string) (*VirtualDisk, error) {
+func getVMDiskInfo(name string, vbox VBoxManager) (*VirtualDisk, error) {
-	out, err := d.vbmOut("showvminfo", name, "--machinereadable")
+	out, err := vbox.vbmOut("showvminfo", name, "--machinereadable")
 	if err != nil {
 		return nil, err
 	}
 
-	r := strings.NewReader(out)
+	return parseDiskInfo(out)
-	return parseDiskInfo(r)
 }
 
-func parseDiskInfo(r io.Reader) (*VirtualDisk, error) {
+func parseDiskInfo(out string) (*VirtualDisk, error) {
-	s := bufio.NewScanner(r)
 	disk := &VirtualDisk{}
+
+	r := strings.NewReader(out)
+	s := bufio.NewScanner(r)
 	for s.Scan() {
 		line := s.Text()
 		if line == "" {
 			continue
 		}
+
 		res := reEqualQuoteLine.FindStringSubmatch(line)
 		if res == nil {
 			continue
 		}
+
 		key, val := res[1], res[2]
 		switch key {
 		case "SATA-1-0":
@@ -44,5 +46,6 @@ func parseDiskInfo(r io.Reader) (*VirtualDisk, error) {
 	if err := s.Err(); err != nil {
 		return nil, err
 	}
+
 	return disk, nil
 }

drivers/virtualbox/disk_test.go

@@ -1,12 +1,14 @@
 package virtualbox
 
 import (
-	"strings"
 	"testing"
+
+	"github.com/docker/machine/drivers/vmwarevsphere/errors"
+	"github.com/stretchr/testify/assert"
 )
 
-var (
+const (
-	testDiskInfoText = `
+	validDiskInfoText = `
 storagecontrollerbootable0="on"
 "SATA-0-0"="/home/ehazlett/.boot2docker/boot2docker.iso"
 "SATA-IsEjected"="off"
@@ -18,19 +20,39 @@ nic1="nat"
 )
 
 func TestVMDiskInfo(t *testing.T) {
-	r := strings.NewReader(testDiskInfoText)
+	vbox := &VBoxManagerMock{
-	disk, err := parseDiskInfo(r)
+		args:   "showvminfo default --machinereadable",
-	if err != nil {
+		stdOut: validDiskInfoText,
-		t.Fatal(err)
 	}
 
-	diskPath := "/home/ehazlett/vm/test/disk.vmdk"
+	disk, err := getVMDiskInfo("default", vbox)
-	diskUUID := "12345-abcdefg"
-	if disk.Path != diskPath {
-		t.Fatalf("expected disk path %s", diskPath)
-	}
 
-	if disk.UUID != diskUUID {
+	assert.Equal(t, "/home/ehazlett/vm/test/disk.vmdk", disk.Path)
-		t.Fatalf("expected disk uuid %s", diskUUID)
+	assert.Equal(t, "12345-abcdefg", disk.UUID)
-	}
+	assert.NoError(t, err)
+}
+
+func TestVMDiskInfoError(t *testing.T) {
+	vbox := &VBoxManagerMock{
+		args: "showvminfo default --machinereadable",
+		err:  errors.New("BUG"),
+	}
+
+	disk, err := getVMDiskInfo("default", vbox)
+
+	assert.Nil(t, disk)
+	assert.EqualError(t, err, "BUG")
+}
+
+func TestVMDiskInfoInvalidOutput(t *testing.T) {
+	vbox := &VBoxManagerMock{
+		args:   "showvminfo default --machinereadable",
+		stdOut: "INVALID",
+	}
+
+	disk, err := getVMDiskInfo("default", vbox)
+
+	assert.Empty(t, disk.Path)
+	assert.Empty(t, disk.UUID)
+	assert.NoError(t, err)
 }

drivers/virtualbox/network.go

@@ -31,75 +31,65 @@ type hostOnlyNetwork struct {
 	NetworkName string // referenced in DHCP.NetworkName
 }
 
-// TODO: use VBoxManager.vbm() instead
-func vbm(args ...string) error {
-	vBoxManager := &VBoxCmdManager{}
-	_, _, err := vBoxManager.vbmOutErr(args...)
-	return err
-}
-
-// TODO: use VBoxManager.vbmOut() instead
-func vbmOut(args ...string) (string, error) {
-	vBoxManager := &VBoxCmdManager{}
-	stdout, _, err := vBoxManager.vbmOutErr(args...)
-	return stdout, err
-}
-
 // Save changes the configuration of the host-only network.
-func (n *hostOnlyNetwork) Save() error {
+func (n *hostOnlyNetwork) Save(vbox VBoxManager) error {
 	if n.IPv4.IP != nil && n.IPv4.Mask != nil {
-		if err := vbm("hostonlyif", "ipconfig", n.Name, "--ip", n.IPv4.IP.String(), "--netmask", net.IP(n.IPv4.Mask).String()); err != nil {
+		if err := vbox.vbm("hostonlyif", "ipconfig", n.Name, "--ip", n.IPv4.IP.String(), "--netmask", net.IP(n.IPv4.Mask).String()); err != nil {
 			return err
 		}
 	}
 
 	if n.IPv6.IP != nil && n.IPv6.Mask != nil {
 		prefixLen, _ := n.IPv6.Mask.Size()
-		if err := vbm("hostonlyif", "ipconfig", n.Name, "--ipv6", n.IPv6.IP.String(), "--netmasklengthv6", fmt.Sprintf("%d", prefixLen)); err != nil {
+		if err := vbox.vbm("hostonlyif", "ipconfig", n.Name, "--ipv6", n.IPv6.IP.String(), "--netmasklengthv6", fmt.Sprintf("%d", prefixLen)); err != nil {
 			return err
 		}
 	}
 
 	if n.DHCP {
-		vbm("hostonlyif", "ipconfig", n.Name, "--dhcp") // not implemented as of VirtualBox 4.3
+		vbox.vbm("hostonlyif", "ipconfig", n.Name, "--dhcp") // not implemented as of VirtualBox 4.3
 	}
 
 	return nil
 }
 
 // createHostonlyNet creates a new host-only network.
-func createHostonlyNet() (*hostOnlyNetwork, error) {
+func createHostonlyNet(vbox VBoxManager) (*hostOnlyNetwork, error) {
-	out, err := vbmOut("hostonlyif", "create")
+	out, err := vbox.vbmOut("hostonlyif", "create")
 	if err != nil {
 		return nil, err
 	}
+
 	res := reHostonlyInterfaceCreated.FindStringSubmatch(string(out))
 	if res == nil {
 		return nil, errors.New("failed to create hostonly interface")
 	}
+
 	return &hostOnlyNetwork{Name: res[1]}, nil
 }
 
 // listHostOnlyNetworks gets all host-only networks in a  map keyed by HostonlyNet.NetworkName.
-func listHostOnlyNetworks() (map[string]*hostOnlyNetwork, error) {
+func listHostOnlyNetworks(vbox VBoxManager) (map[string]*hostOnlyNetwork, error) {
-	out, err := vbmOut("list", "hostonlyifs")
+	out, err := vbox.vbmOut("list", "hostonlyifs")
 	if err != nil {
 		return nil, err
 	}
-	s := bufio.NewScanner(strings.NewReader(out))
+
 	m := map[string]*hostOnlyNetwork{}
 	n := &hostOnlyNetwork{}
+
+	s := bufio.NewScanner(strings.NewReader(out))
 	for s.Scan() {
 		line := s.Text()
 		if line == "" {
-			m[n.NetworkName] = n
-			n = &hostOnlyNetwork{}
 			continue
 		}
+
 		res := reColonLine.FindStringSubmatch(line)
 		if res == nil {
 			continue
 		}
+
 		switch key, val := res[1], res[2]; key {
 		case "Name":
 			n.Name = val
@@ -131,8 +121,11 @@ func listHostOnlyNetworks() (map[string]*hostOnlyNetwork, error) {
 			n.Status = val
 		case "VBoxNetworkName":
 			n.NetworkName = val
+			m[val] = n
+			n = &hostOnlyNetwork{}
 		}
 	}
+
 	if err := s.Err(); err != nil {
 		return nil, err
 	}
@@ -153,36 +146,38 @@ func getHostOnlyNetwork(nets map[string]*hostOnlyNetwork, hostIP net.IP, netmask
 	return nil
 }
 
-func getOrCreateHostOnlyNetwork(hostIP net.IP, netmask net.IPMask, dhcpIP net.IP, dhcpUpperIP net.IP, dhcpLowerIP net.IP) (*hostOnlyNetwork, error) {
+func getOrCreateHostOnlyNetwork(hostIP net.IP, netmask net.IPMask, dhcpIP net.IP, dhcpLowerIP net.IP, dhcpUpperIP net.IP, vbox VBoxManager) (*hostOnlyNetwork, error) {
-	nets, err := listHostOnlyNetworks()
+	nets, err := listHostOnlyNetworks(vbox)
 	if err != nil {
 		return nil, err
 	}
 
 	hostOnlyNet := getHostOnlyNetwork(nets, hostIP, netmask)
+	if hostOnlyNet != nil {
+		return hostOnlyNet, nil
+	}
 
-	if hostOnlyNet == nil {
 	// No existing host-only interface found. Create a new one.
-		hostOnlyNet, err = createHostonlyNet()
+	hostOnlyNet, err = createHostonlyNet(vbox)
 	if err != nil {
 		return nil, err
 	}
+
 	hostOnlyNet.IPv4.IP = hostIP
 	hostOnlyNet.IPv4.Mask = netmask
-		if err := hostOnlyNet.Save(); err != nil {
+	if err := hostOnlyNet.Save(vbox); err != nil {
 		return nil, err
 	}
 
 	dhcp := dhcpServer{}
 	dhcp.IPv4.IP = dhcpIP
 	dhcp.IPv4.Mask = netmask
-		dhcp.LowerIP = dhcpUpperIP
+	dhcp.LowerIP = dhcpLowerIP
-		dhcp.UpperIP = dhcpLowerIP
+	dhcp.UpperIP = dhcpUpperIP
 	dhcp.Enabled = true
-		if err := addHostonlyDHCP(hostOnlyNet.Name, dhcp); err != nil {
+	if err := addHostonlyDHCP(hostOnlyNet.Name, dhcp, vbox); err != nil {
 		return nil, err
 	}
-	}
 
 	return hostOnlyNet, nil
 }
@@ -196,12 +191,12 @@ type dhcpServer struct {
 	Enabled     bool
 }
 
-func addDHCPServer(kind, name string, d dhcpServer) error {
+func addDHCPServer(kind, name string, d dhcpServer, vbox VBoxManager) error {
 	command := "modify"
 
 	// On some platforms (OSX), creating a hostonlyinterface adds a default dhcpserver
 	// While on others (Windows?) it does not.
-	dhcps, err := getDHCPServers()
+	dhcps, err := getDHCPServers(vbox)
 	if err != nil {
 		return err
 	}
@@ -222,17 +217,18 @@ func addDHCPServer(kind, name string, d dhcpServer) error {
 	} else {
 		args = append(args, "--disable")
 	}
-	return vbm(args...)
+
+	return vbox.vbm(args...)
 }
 
 // addHostonlyDHCP adds a DHCP server to a host-only network.
-func addHostonlyDHCP(ifname string, d dhcpServer) error {
+func addHostonlyDHCP(ifname string, d dhcpServer, vbox VBoxManager) error {
-	return addDHCPServer("--netname", "HostInterfaceNetworking-"+ifname, d)
+	return addDHCPServer("--netname", "HostInterfaceNetworking-"+ifname, d, vbox)
 }
 
 // getDHCPServers gets all DHCP server settings in a map keyed by DHCP.NetworkName.
-func getDHCPServers() (map[string]*dhcpServer, error) {
+func getDHCPServers(vbox VBoxManager) (map[string]*dhcpServer, error) {
-	out, err := vbmOut("list", "dhcpservers")
+	out, err := vbox.vbmOut("list", "dhcpservers")
 	if err != nil {
 		return nil, err
 	}

drivers/virtualbox/network_test.go

@@ -4,8 +4,49 @@ import (
 	"net"
 	"reflect"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
+const stdOutOneHostOnlyNetwork = `Name:            vboxnet0
+GUID:            786f6276-656e-4074-8000-0a0027000000
+DHCP:            Disabled
+IPAddress:       192.168.99.1
+NetworkMask:     255.255.255.0
+IPV6Address:
+IPV6NetworkMaskPrefixLength: 0
+HardwareAddress: 0a:00:27:00:00:00
+MediumType:      Ethernet
+Status:          Up
+VBoxNetworkName: HostInterfaceNetworking-vboxnet0
+
+`
+const stdOutTwoHostOnlyNetwork = `Name:            vboxnet0
+GUID:            786f6276-656e-4074-8000-0a0027000000
+DHCP:            Disabled
+IPAddress:       192.168.99.1
+NetworkMask:     255.255.255.0
+IPV6Address:
+IPV6NetworkMaskPrefixLength: 0
+HardwareAddress: 0a:00:27:00:00:00
+MediumType:      Ethernet
+Status:          Up
+VBoxNetworkName: HostInterfaceNetworking-vboxnet0
+
+Name:            vboxnet1
+GUID:            786f6276-656e-4174-8000-0a0027000001
+DHCP:            Disabled
+IPAddress:       192.168.99.1
+NetworkMask:     255.255.255.0
+IPV6Address:
+IPV6NetworkMaskPrefixLength: 0
+HardwareAddress: 0a:00:27:00:00:01
+MediumType:      Ethernet
+Status:          Up
+VBoxNetworkName: HostInterfaceNetworking-vboxnet1
+
+`
+
 // Tests that when we have a host only network which matches our expectations,
 // it gets returned correctly.
 func TestGetHostOnlyNetworkHappy(t *testing.T) {
@@ -82,3 +123,91 @@ func TestGetHostOnlyNetworkWindows10Bug(t *testing.T) {
 		t.Fatalf("Expected result of calling getHostOnlyNetwork to be the same as expected but it was not:\nexpected: %+v\nactual: %+v\n", expectedHostOnlyNetwork, n)
 	}
 }
+
+func TestListHostOnlyNetworks(t *testing.T) {
+	vbox := &VBoxManagerMock{
+		args:   "list hostonlyifs",
+		stdOut: stdOutOneHostOnlyNetwork,
+	}
+
+	nets, err := listHostOnlyNetworks(vbox)
+
+	assert.Equal(t, 1, len(nets))
+	assert.NoError(t, err)
+
+	net, present := nets["HostInterfaceNetworking-vboxnet0"]
+
+	assert.True(t, present)
+	assert.Equal(t, "vboxnet0", net.Name)
+	assert.Equal(t, "786f6276-656e-4074-8000-0a0027000000", net.GUID)
+	assert.False(t, net.DHCP)
+	assert.Equal(t, "192.168.99.1", net.IPv4.IP.String())
+	assert.Equal(t, "ffffff00", net.IPv4.Mask.String())
+	assert.Empty(t, net.IPv6.IP)
+	assert.Equal(t, "0a:00:27:00:00:00", net.HwAddr.String())
+	assert.Equal(t, "Ethernet", net.Medium)
+	assert.Equal(t, "Up", net.Status)
+	assert.Equal(t, "HostInterfaceNetworking-vboxnet0", net.NetworkName)
+}
+
+func TestListTwoHostOnlyNetworks(t *testing.T) {
+	vbox := &VBoxManagerMock{
+		args:   "list hostonlyifs",
+		stdOut: stdOutTwoHostOnlyNetwork,
+	}
+
+	nets, err := listHostOnlyNetworks(vbox)
+
+	assert.Equal(t, 2, len(nets))
+	assert.NoError(t, err)
+
+	net, present := nets["HostInterfaceNetworking-vboxnet1"]
+
+	assert.True(t, present)
+	assert.Equal(t, "vboxnet1", net.Name)
+	assert.Equal(t, "786f6276-656e-4174-8000-0a0027000001", net.GUID)
+	assert.False(t, net.DHCP)
+	assert.Equal(t, "192.168.99.1", net.IPv4.IP.String())
+	assert.Equal(t, "ffffff00", net.IPv4.Mask.String())
+	assert.Empty(t, net.IPv6.IP)
+	assert.Equal(t, "0a:00:27:00:00:01", net.HwAddr.String())
+	assert.Equal(t, "Ethernet", net.Medium)
+	assert.Equal(t, "Up", net.Status)
+	assert.Equal(t, "HostInterfaceNetworking-vboxnet1", net.NetworkName)
+}
+
+func TestListHostOnlyNetworksDontRelyOnEmptyLinesForParsing(t *testing.T) {
+	vbox := &VBoxManagerMock{
+		args: "list hostonlyifs",
+		stdOut: `Name:            vboxnet0
+VBoxNetworkName: HostInterfaceNetworking-vboxnet0
+Name:            vboxnet1
+VBoxNetworkName: HostInterfaceNetworking-vboxnet1`,
+	}
+
+	nets, err := listHostOnlyNetworks(vbox)
+
+	assert.Equal(t, 2, len(nets))
+	assert.NoError(t, err)
+
+	net, present := nets["HostInterfaceNetworking-vboxnet1"]
+	assert.True(t, present)
+	assert.Equal(t, "vboxnet1", net.Name)
+
+	net, present = nets["HostInterfaceNetworking-vboxnet0"]
+	assert.True(t, present)
+	assert.Equal(t, "vboxnet0", net.Name)
+}
+
+func TestGetHostOnlyNetwork(t *testing.T) {
+	vbox := &VBoxManagerMock{
+		args:   "list hostonlyifs",
+		stdOut: stdOutOneHostOnlyNetwork,
+	}
+
+	net, err := getOrCreateHostOnlyNetwork(net.ParseIP("192.168.99.1"), parseIPv4Mask("255.255.255.0"), nil, nil, nil, vbox)
+
+	assert.NotNil(t, net)
+	assert.Equal(t, "HostInterfaceNetworking-vboxnet0", net.NetworkName)
+	assert.NoError(t, err)
+}

drivers/virtualbox/virtualbox.go

@@ -239,7 +239,7 @@ func (d *Driver) Create() error {
 		// make sure vm is stopped
 		_ = d.vbm("controlvm", name, "poweroff")
 
-		diskInfo, err := d.getVMDiskInfo(name)
+		diskInfo, err := getVMDiskInfo(name, d.VBoxManager)
 		if err != nil {
 			return err
 		}
@@ -253,7 +253,7 @@ func (d *Driver) Create() error {
 		}
 
 		log.Debugf("Importing VM settings...")
-		vmInfo, err := d.getVMInfo(name)
+		vmInfo, err := getVMInfo(name, d.VBoxManager)
 		if err != nil {
 			return err
 		}
@@ -698,6 +698,7 @@ func (d *Driver) setupHostOnlyNetwork(machineName string) error {
 		dhcpAddr,
 		lowerDHCPIP,
 		upperDHCPIP,
+		d.VBoxManager,
 	)
 	if err != nil {
 		return err

drivers/virtualbox/virtualbox_test.go

@@ -31,13 +31,22 @@ func TestDefaultSSHUsername(t *testing.T) {
 }
 
 type VBoxManagerMock struct {
-	VBoxCmdManager
 	args   string
 	stdOut string
 	stdErr string
 	err    error
 }
 
+func (v *VBoxManagerMock) vbm(args ...string) error {
+	_, _, err := v.vbmOutErr(args...)
+	return err
+}
+
+func (v *VBoxManagerMock) vbmOut(args ...string) (string, error) {
+	stdout, _, err := v.vbmOutErr(args...)
+	return stdout, err
+}
+
 func (v *VBoxManagerMock) vbmOutErr(args ...string) (string, string, error) {
 	if strings.Join(args, " ") == v.args {
 		return v.stdOut, v.stdErr, v.err

drivers/virtualbox/vm.go

@@ -12,8 +12,8 @@ type VM struct {
 	Memory int
 }
 
-func (d *Driver) getVMInfo(name string) (*VM, error) {
+func getVMInfo(name string, vbox VBoxManager) (*VM, error) {
-	out, err := d.vbmOut("showvminfo", name, "--machinereadable")
+	out, err := vbox.vbmOut("showvminfo", name, "--machinereadable")
 	if err != nil {
 		return nil, err
 	}