Optimizing volumepropverification_test #1310

ashahi1 · 2017-05-30T21:22:17Z

Addresses Issue #1096

Also converts volumepropverification_test.go to use gocheck testing framework

Ran the test locally. Test output is as follows:

2017/06/06 21:35:21 START: volumeproperty_test.TestVolumeProperties
2017/06/06 21:35:21 Creating volume [thin_volume_1496784921] with options [ -o diskformat=thin] on VM [10.192.66.52]
2017/06/06 21:35:23 Creating volume [zeroedthick_volume_1496784923] with options [ -o diskformat=zeroedthick] on VM [10.192.66.52]
2017/06/06 21:35:25 Creating volume [eagerzeroedthick_volume_1496784925] with options [ -o diskformat=eagerzeroedthick] on VM [10.192.66.52]
2017/06/06 21:35:28 Getting size, disk-format and attached-to-vm for volume [thin_volume_1496784921] from ESX [10.192.87.211] using admin cli.
2017/06/06 21:35:30 Getting size, disk-format and attached-to-vm for volume [zeroedthick_volume_1496784923] from ESX [10.192.87.211] using admin cli.
2017/06/06 21:35:31 Getting size, disk-format and attached-to-vm for volume [eagerzeroedthick_volume_1496784925] from ESX [10.192.87.211] using admin cli.
2017/06/06 21:35:33 Getting size, disk-format and attached-to-vm for volume [thin_volume_1496784921] from vm [10.192.66.52] using docker cli
2017/06/06 21:35:33 Getting size, disk-format and attached-to-vm for volume [zeroedthick_volume_1496784923] from vm [10.192.66.52] using docker cli
2017/06/06 21:35:35 Getting size, disk-format and attached-to-vm for volume [eagerzeroedthick_volume_1496784925] from vm [10.192.66.52] using docker cli
2017/06/06 21:35:36 Verify docker and ESX properties of all three volumes are same
2017/06/06 21:35:36 Attaching volume [thin_volume_1496784921] on VM [10.192.66.52]
2017/06/06 21:35:38 Getting size, disk-format and attached-to-vm for volume [thin_volume_1496784921] from ESX [10.192.87.211] using admin cli.
2017/06/06 21:35:39 Getting size, disk-format and attached-to-vm for volume [thin_volume_1496784921] from vm [10.192.66.52] using docker cli
2017/06/06 21:35:40 Verify docker and ESX properties of all three volumes are same
2017/06/06 21:35:40 END: volumeproperty_test.TestVolumeProperties
2017/06/06 21:35:40 Removing all containers on VM [10.192.66.52]
2017/06/06 21:35:43  CLEAN-UP Finished: volumeproperty_test.TestVolumeProperties

govint · 2017-05-31T08:53:43Z

tests/constants/dockercli/cmd.go

@@ -76,4 +76,13 @@ const (

 	// QueryContainer checks whether container exists or not
 	QueryContainer = docker + "ps -aq --filter name="
+
+	// StopAllContainers refers to stopping all the containers
+	StopAllContainers = docker + "kill $(docker ps -q)"


Can this be "docker ps -aq"?

govint · 2017-05-31T08:54:50Z

tests/constants/dockercli/cmd.go

+	RemoveAllContainers = docker + "rm $(docker ps -a -q)"
+
+	// DeleteAllDockerVolumes refers to removing all the docker volumes
+	DeleteAllDockerVolumes = ListVolumes + "-qf dangling=true | xargs -r docker volume rm"


Thanks for adding this.

govint · 2017-05-31T09:01:14Z

tests/e2e/volumepropverification_test.go

+func (s *VolumePropVerificationSuite) SetUpTest(c *C) {
+	s.dockerHostIP = inputparams.GetDockerHostIP("VM2")
+	s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP)
+	s.esxIP = os.Getenv("ESX")


Pls. use inputparams.GetEsxIP()

govint · 2017-05-31T09:02:04Z

tests/e2e/volumepropverification_test.go

+	s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP)
+	s.esxIP = os.Getenv("ESX")
+	s.volumeName = inputparams.GetVolumeNameWithTimeStamp("volumepropverification_test")
+	s.containerName = inputparams.GetContainerNameWithTimeStamp("volumepropverificationSuite_test")


These two volume name substrings can be as constants at the top of this file.

Can check if any of the needed params is not valid and call c.Skip() so that the test is skipped vs. running and spewing errors or Assert()s.

govint · 2017-05-31T09:10:28Z

tests/e2e/volumepropverification_test.go

-			log.Printf("Actual values : %s. Does not contains %s ", op, volmProps[i])
+func hasElement(volumeProps []string, op string) bool {
+	for i := 0; i < len(volumeProps); i++ {
+		if !strings.Contains(op, volumeProps[i]) {


Use range to iterate over the array?

govint · 2017-05-31T09:13:40Z

tests/e2e/volumepropverification_test.go

+func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) {
+	log.Printf("START: TestVolumeProperties")
+	formatTypes := []string{"thin", "zeroedthick", "eagerzeroedthick"}
+	for i := 0; i < len(formatTypes); i++ {


Pls. use "range" to iterate over the array.

govint · 2017-05-31T09:17:05Z

tests/e2e/volumepropverification_test.go

+
+		if formatTypes[i] == "thin" {
+			out, err = dockercli.AttachVolume(s.dockerHostIP, s.volumeName, s.containerName)
+			c.Assert(err, IsNil, Commentf("Failed to attach the volume [%s]", s.volumeName))


Please print value of "out" so test output shows the cause of failure.

govint · 2017-05-31T09:33:45Z

tests/e2e/volumepropverification_test.go

+		c.Assert(err, IsNil, Commentf(out))
+
+		volumePropsAdminCli := verification.GetVolumePropertiesAdminCli(s.volumeName, s.esxIP)
+		expectedPropsAdminCli := []string{"100MB", formatTypes[i], "detached"}


The verification functions themselves seem to be doing comparison and verification. Suggest defining new functions or update existing functions to do the verification needed here.

Or if the expected and actual values are of a specific format (like strings) then create a function in this file and call it for each of the verifications here vs. adding code for each type of verification below.

govint · 2017-05-31T09:35:18Z

tests/e2e/volumepropverification_test.go

+		c.Assert(err, IsNil, Commentf(out))
+
+		volumePropsAdminCli := verification.GetVolumePropertiesAdminCli(s.volumeName, s.esxIP)
+		expectedPropsAdminCli := []string{"100MB", formatTypes[i], "detached"}


Please make the expected values as constants (top of the file) with appropriate names so its easy to read and understand the code.

govint · 2017-05-31T09:38:31Z

tests/e2e/volumepropverification_test.go

+		c.Assert(hasElement(expectedPropsAdminCli, volumePropsAdminCli), Equals, true, Commentf("Volume %s properties "+
+			"on ESX fetched using admin cli does not matches with the expected values", s.volumeName))
+
+		volumePropsDockerCli := verification.GetVolumePropertiesDockerCli(s.volumeName, s.dockerHostIP)


This function is buggy already - please fix it in this PR - its logging a fatal log if inspect fails. We've already decided that fatal logs are disallowed. Instead return a string and error type from this function.

govint · 2017-05-31T09:46:48Z

tests/e2e/volumepropverification_test.go

+			"on ESX fetched using admin cli does not matches with the expected values", s.volumeName))
+
+		volumePropsDockerCli := verification.GetVolumePropertiesDockerCli(s.volumeName, s.dockerHostIP)
+		expectedPropsDockerCli := []string{"100MB", formatTypes[i], "<no value>"}


Pls. use a constant with a meaningful name instead.

govint · 2017-05-31T09:51:11Z

tests/e2e/volumepropverification_test.go

+			vmNameFromAdminCli := verification.GetVMAttachedToVolUsingAdminCli(s.volumeName, s.esxIP)
+			vmNameFromDockerCli := verification.GetVMAttachedToVolUsingDockerCli(s.volumeName, s.dockerHostIP)
+
+			// Get vm name based on ip and compare ith with the docker cli and admin cli


comment - compare ith with

govint · 2017-05-31T09:52:31Z

tests/e2e/volumepropverification_test.go

+			c.Assert(vmNameFromAdminCli, Equals, vmNameFromDockerCli, Commentf("Information mis-match - "+
+				"Attached-to-VM field for volume from docker cli is [%s] and attched-to-vm field from admin cli is [%s]", vmNameFromDockerCli, vmNameFromAdminCli))
+
+			// Again verifyying volume properties after starting a container


Comment - verifyying

govint · 2017-05-31T09:53:05Z

tests/e2e/volumepropverification_test.go

+
+			// Again verifyying volume properties after starting a container
+			volumePropsDockerCli = verification.GetVolumePropertiesDockerCli(s.volumeName, s.dockerHostIP)
+			expectedProprtyDockerCli := []string{"100MB", formatTypes[i]}


Use appropriately named constant instead.

govint · 2017-05-31T09:55:21Z

tests/e2e/volumepropverification_test.go

+				"for volume %s is not same as that fetched from its IP.", s.volumeName))
+
+			c.Assert(vmNameFromAdminCli, Equals, vmNameFromDockerCli, Commentf("Information mis-match - "+
+				"Attached-to-VM field for volume from docker cli is [%s] and attched-to-vm field from admin cli is [%s]", vmNameFromDockerCli, vmNameFromAdminCli))


Why shouldn't these two checks on the VM name be done for all disk format types - why is it done only for "thin" disks?

Test was taking long time to finish and intermittently test was failing on CI because test ran too long. So it was decided to perform certain steps only for "thin" disk format to reduce the overall test duration. Basically wanted to finish CI runs faster.

Please refer to Issue #1096

govint · 2017-05-31T09:56:57Z

tests/e2e/volumepropverification_test.go

+		c.Assert(hasElement(expectedPropsDockerCli, volumePropsDockerCli), Equals, true, Commentf("Volume %s properties "+
+			"on docker vm fetched using docker cli does not matches with the expected values", s.volumeName))
+
+		if formatTypes[i] == "thin" {


Move this handling for thin disks into a separate function.

Add a new function that does "mandatory checks on volume properties"

Get the admin CLI and docker CLI volume properties and pass to function in (2) to make the checks.

If volume is thin then call another function to verify volume properties specific for thin disks.

Make this function smaller by breaking it up into separate functions

One function to create the volumes,

Then loop over the volumes and get the admin and docker CLI properties for each volume.

Call the property validation function for each volume.

govint · 2017-05-31T09:57:49Z

tests/e2e/volumepropverification_test.go

+				"on docker vm fetched using docker cli does not matches with the expected values", s.volumeName))
+
+			volumePropsAdminCli = verification.GetVolumePropertiesAdminCli(s.volumeName, s.esxIP)
+			expectedPropsAdminCli = []string{"100MB", formatTypes[i]}


Why do we keep getting docker CLI and admin CLI volume properties each time. Get it once for each volume and do the property validations with it.

govint

Set of changes to layout, simpler functions, more properties to validate.

shuklanirdesh82 · 2017-05-31T22:15:19Z

tests/e2e/volumepropverification_test.go


+	con "github.com/vmware/docker-volume-vsphere/tests/constants/dockercli"


minor: con?? ... how about dockerconst

shuklanirdesh82 · 2017-05-31T22:18:11Z

tests/e2e/volumepropverification_test.go

-	ssh.InvokeCommand(vmIP, dockerVolmRmvCmd)
-	log.Println("-----Clean-up finished - current time: ", time.Now())
+func (s *VolumePropVerificationSuite) TearDownTest(c *C) {
+	out, err := dockercli.StopAllContainers(s.dockerHostIP)


it is better to avoid this step and invoke forceful removal of containers .. unless it is required to have this extra step.

shuklanirdesh82 · 2017-05-31T22:19:29Z

tests/constants/dockercli/cmd.go

+	StopAllContainers = docker + "kill $(docker ps -q)"
+
+	// RemoveAllContainers refers to removing all the containers
+	RemoveAllContainers = docker + "rm $(docker ps -a -q)"


docker ps -a -q => docker ps -aq ??

shuklanirdesh82 · 2017-05-31T22:22:16Z

tests/utils/dockercli/volumelifecycle.go

+}
+
+// DeleteAllDockerVolumes - removes all the containers on a particular vm
+func DeleteAllDockerVolumes(ip string) (string, error) {


it looks cool but dangerous .. it may delete unwanted volumes as well. one serious problem I can think of running test in parallel.

Ideal approach should be deleting volumes those are created as part of current running test instance.

+1. This is useful only if we are sure we are doing the right thing (i.e. no side effects).

shaominchen · 2017-05-31T23:22:20Z

tests/constants/dockercli/cmd.go

@@ -76,4 +76,13 @@ const (

 	// QueryContainer checks whether container exists or not
 	QueryContainer = docker + "ps -aq --filter name="
+
+	// StopAllContainers refers to stopping all the containers


Nit: Just for consistency, let's remove "refers to" to keep the comments simple.

shaominchen · 2017-05-31T23:24:40Z

tests/utils/dockercli/volumelifecycle.go

+	return ssh.InvokeCommand(ip, dockercli.RemoveAllContainers)
+}
+
+// DeleteAllDockerVolumes - removes all the containers on a particular vm


The comments need to be updated.

shaominchen · 2017-05-31T23:28:13Z

tests/utils/dockercli/volumelifecycle.go

+}
+
+// DeleteAllDockerVolumes - removes all the containers on a particular vm
+func DeleteAllDockerVolumes(ip string) (string, error) {


+1. This is useful only if we are sure we are doing the right thing (i.e. no side effects).

shaominchen · 2017-05-31T23:34:06Z

tests/e2e/volumepropverification_test.go

+	s.dockerHostIP = inputparams.GetDockerHostIP("VM2")
+	s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP)
+	s.esxIP = os.Getenv("ESX")
+	s.volumeName = inputparams.GetVolumeNameWithTimeStamp("volumepropverification_test")


This name is just too long - can we rename it (and the file name) to volumeproperty_test?

shaominchen · 2017-05-31T23:43:29Z

tests/e2e/volumepropverification_test.go

+func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) {
+	log.Printf("START: TestVolumeProperties")
+	formatTypes := []string{"thin", "zeroedthick", "eagerzeroedthick"}
+	for i := 0; i < len(formatTypes); i++ {


shaominchen · 2017-05-31T23:44:20Z

tests/e2e/volumepropverification_test.go

-			}
+
+func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) {
+	log.Printf("START: TestVolumeProperties")


Nit: Just for consistency, let's include the test group name in the log as well.

shuklanirdesh82 · 2017-06-02T00:53:32Z

tests/e2e/volumeproperty_test.go

+)
+
+const (
+	TestName           = "volumeproperty"


all this should start with lower case unless it is meant for exporting.

shuklanirdesh82 · 2017-06-02T00:56:04Z

tests/utils/verification/volumeproperties.go

-			"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op)
+	out, err := ssh.InvokeCommand(hostname, cmd)
+	if err != nil {
+		log.Printf("Null value is returned by admin cli when looking for, size, disk-format and attached-to-vm for volume [%s].", volName)


this is not needed as InvokeCommand takes care of this one.

shuklanirdesh82 · 2017-06-02T00:58:35Z

tests/utils/verification/volumeproperties.go

+	cmd := dockercli.InspectVolume + " --format ' {{index .Name}} {{index .Status.capacity.size}} {{index .Status.diskformat}} {{index .Status \"attached to VM\"}}' " + volName
+	out, err := ssh.InvokeCommand(hostname, cmd)
+	if err != nil {
+		log.Printf("Null value is returned by docker cli when looking for, size, disk-format and attached-to-vm for volume [%s].", volName)


same as above

shuklanirdesh82 · 2017-06-02T01:41:19Z

tests/e2e/volumeproperty_test.go

+	for i, formatType := range s.formatTypes {
+		s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName)
+		s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName)
+		out, err := ssh.InvokeCommand(s.dockerHostIP, dockerconst.CreateVolume+"--name="+s.volumeNames[i]+" -o diskformat="+formatType)


can't we use https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/dockercli/volumelifecycle.go#L37?

I knew that it was in review but wasn't sure if it got checked-in. Will remove it.

shuklanirdesh82 · 2017-06-02T01:42:49Z

tests/e2e/volumeproperty_test.go

+// createVolumes - creates volumes of each format type
+func (s *VolumePropVerificationSuite) createVolumes(c *C) {
+	for i, formatType := range s.formatTypes {
+		s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName)


isn't it keep setting and overriding value and set with the last loop?

I missed it. Will remove it.

shuklanirdesh82 · 2017-06-02T02:05:25Z

tests/e2e/volumeproperty_test.go

+
+// getPropertiesAndVerify - gets properties of a volume from esx and docker host
+func (s *VolumePropVerificationSuite) getPropertiesAndVerify(c *C, volumeName string) {
+	valuesAdminCli, err := verification.GetVolumePropertiesAdminCli(volumeName, s.esxIP)


It is better to invoke admin_cli volume ls once > save the response and perform validation this reduces unnecessary ssh calls for each volumes following.

shuklanirdesh82 · 2017-06-02T02:18:17Z

tests/e2e/volumeproperty_test.go

+	sliceDockerCliValues := s.arrangeDockerCliValues(valuesDockerCli)
+	for i := range sliceDockerCliValues {
+		c.Assert(len(sliceDockerCliValues), Equals, len(sliceAdminCliValues), Commentf("Values mis-match between admin cli and docker cli for volume %s", volumeName))
+		if i == 2 && !s.vmAttached {


what is so special about i==2? you may want to put some inline comment to make reviewer's life easier.

govint

Updated comments on the approach to verification. Please see comments on the GetVolumeProperties*() functions.

govint · 2017-06-02T13:11:36Z

tests/e2e/volumeproperty_test.go

+	for _, volumeName := range s.volumeNames {
+		// getting and verifying the admin cli and docker cli values
+		s.getPropertiesAndVerify(c, volumeName)
+		s.dockerVolumeRemoveCmd = s.dockerVolumeRemoveCmd + " " + volumeName


This is a global and why is it getting initialized here? if getPropertiesAndVerify() wants to use this global let it use it. Seems odd why this global is set here and not used.

govint · 2017-06-02T13:14:36Z

tests/e2e/volumeproperty_test.go

+func (s *VolumePropVerificationSuite) createVolumes(c *C) {
+	for i, formatType := range s.formatTypes {
+		s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName)
+		s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName)


Watchout, the GetVolumeNameWithTimeStamp() is using time.New().Unix() which returns the number of seconds since some time. Successive calls can return the same timestamp. I'd seen this and use names with a random generated number attached to it. For clarity sake you could have different names - thick, zeroedthick, thin and attach the timestamp that will make it unique anyway with different names - thick-timestamp, zeroedthick-timestamp, thin-timestamp.

I've filed issue #1344 for this.

govint · 2017-06-02T13:14:55Z

tests/e2e/volumeproperty_test.go

+	for i, formatType := range s.formatTypes {
+		s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName)
+		s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName)
+		out, err := ssh.InvokeCommand(s.dockerHostIP, dockerconst.CreateVolume+"--name="+s.volumeNames[i]+" -o diskformat="+formatType)


Yes, please use dockercli.CreateVolume() API.

govint · 2017-06-02T13:28:43Z

tests/e2e/volumeproperty_test.go

+	valuesAdminCli, err := verification.GetVolumePropertiesAdminCli(volumeName, s.esxIP)
+	c.Assert(err, IsNil, Commentf(valuesAdminCli))
+
+	valuesDockerCli, err := verification.GetVolumePropertiesDockerCli(volumeName, s.dockerHostIP)


Only a suggestion and you can decide how difficult or easy it may be to do this. The verifyProperties() function is may be a bit complex. Is it possible instead for the GetVolumeProperties*() functions to return a map of values so that the verification is easy to do. That would be ideal as it makes verifyProperties() pretty easy.

govint · 2017-06-02T13:30:29Z

tests/e2e/volumeproperty_test.go

+			c.Assert(sliceDockerCliValues[i], Equals, sliceAdminCliValues[i], Commentf("Value from admincli is [%s] and that from dockercli is [%s]", sliceAdminCliValues[i], sliceDockerCliValues[i]))
+		}
+	}
+	// Get vm name based on ip and compare ith with the docker cli/admin cli only when vm is attached


Nit comment - "ith with"

govint · 2017-06-02T13:32:42Z

tests/e2e/volumeproperty_test.go

+	// Get vm name based on ip and compare ith with the docker cli/admin cli only when vm is attached
+	if s.vmAttached {
+		c.Assert(sliceDockerCliValues[len(sliceDockerCliValues)-1], Equals, s.dockerHostName, Commentf("VM name fetched from attached-to-VM field "+
+			"for volume %s is not same as that fetched from its IP.", volumeName))


This code can be very delicate and can easily fail with these assumed locations of volume properties. Its also the wrong way to compare attributes of a volume based on position of values at specific indices. Lets make the GetVolumeProperties*() functions to return maps vs strings. May be more work but its worth the effort to make the test code here accurate without complexity.

govint · 2017-06-02T13:33:50Z

tests/e2e/volumeproperty_test.go

+	sliceDockerCliValues := arrDockerCliValues[1:len(arrDockerCliValues)]
+	if !s.vmAttached {
+		sliceDockerCliValues[len(sliceDockerCliValues)-2] = sliceDockerCliValues[len(sliceDockerCliValues)-2] + sliceDockerCliValues[len(sliceDockerCliValues)-1]
+		sliceDockerCliValues = sliceDockerCliValues[0 : len(sliceDockerCliValues)-1]


:-))))), but no Sir, this isn't right at all. Please make those util function return maps and we all can use those to benefit of more tests.

shaominchen · 2017-06-02T22:27:50Z

tests/constants/dockercli/cmd.go

@@ -29,7 +29,7 @@ const (
 	// CreateVolume create a volume with vsphere driver
 	CreateVolume = dockerVol + " create --driver=vsphere "

-	// RemoveVolume constant refers delete volume command
+	// RemoveVolume delete volume command


Nit: just for consistency, let's remove " command" from the comments. Same for below constants.

shaominchen · 2017-06-02T22:30:44Z

tests/e2e/volumeproperty_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This is an end-to-end test. Test will ssh to a vm and create a volume using docker cli.


Let's update this comment to briefly explain the main purpose of this test group, instead of explaining detailed test steps here.

shaominchen · 2017-06-02T22:32:23Z

tests/e2e/volumeproperty_test.go

+	DiskStatusEmpty    = "<novalue>"
+)
+
+type VolumePropVerificationSuite struct {


Let's rename this to VolumePropertyTestSuite.

shaominchen · 2017-06-02T22:38:17Z

tests/e2e/volumeproperty_test.go

+	containerName         string
+	esxIP                 string
+	formatTypes           []string
+	dockerVolumeRemoveCmd string


This field doesn't make much sense. Why don't use dockerconst.RemoveVolume directly?

I am creating a single command to remove all the volumes so that we don't need to ssh for each volume.
This is how final command looks like: docker volume rm vol1 vol2 vol3

shaominchen · 2017-06-02T22:40:37Z

tests/e2e/volumeproperty_test.go

+*/
+
+func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) {
+	log.Printf("START: TestVolumeProperties")


Nit: can we add test group name as well?
log.Printf("START: volumeproperty_test.TestVolumeProperties")

shaominchen · 2017-06-02T22:41:11Z

tests/e2e/volumeproperty_test.go

+	s.vmAttached = true
+	s.getPropertiesAndVerify(c, s.volumeNames[0])
+
+	log.Printf("END: TestVolumeProperties")


Same as above.

shaominchen · 2017-06-02T23:15:27Z

tests/e2e/volumeproperty_test.go

+func (s *VolumePropVerificationSuite) createVolumes(c *C) {
+	for i, formatType := range s.formatTypes {
+		s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName)
+		s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName)


I've filed issue #1344 for this.

govint

Comments on current code changes.

govint · 2017-06-05T06:54:49Z

tests/e2e/volumeproperty_test.go

+	s.dockerHostIP = inputparams.GetDockerHostIP("VM2")
+	s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP)
+	s.esxIP = inputparams.GetEsxIP()
+	s.containerName = inputparams.GetContainerNameWithTimeStamp(testName)


PR #1286 is adding up a inputparams/testparams.go:GetTestConfig() which gets all these in a single call inside a struct. You could use that once its merged (ready for merge now). The test code doesn't need to use multiple API (inputparams, govc) just to set up params.

govint · 2017-06-05T06:55:44Z

tests/e2e/volumeproperty_test.go

+func (s *VolumePropertyTestSuite) verifyParams(c *C) {
+	if s.dockerHostIP == "" || s.dockerHostName == "" || s.esxIP == "" {
+		c.Skip("Empty values - hence skipping the test")
+	}


This is simplified with GetTestConfig() from above comment (see #1286)

govint · 2017-06-05T06:57:28Z

tests/e2e/volumeproperty_test.go

+	c.Assert(err, IsNil, Commentf(out))
+	out, err = ssh.InvokeCommand(s.dockerHostIP, s.dockerVolumeRemoveCmd)
+	c.Assert(err, IsNil, Commentf(out))
+}


Can add a log with timestamp at the end of Setup and Teardown - just so we know when each test starts and when it ends, what takes time etc.

OK, I will add log statements at end of setup and teardown and that automatically prints current time.
e.g.

2017/06/05 17:01:15 SETUP Finished: volumeproperty_test.TestVolumeProperties 2017/06/05 17:01:40 CLEAN-UP Finished: volumeproperty_test.TestVolumeProperties

govint · 2017-06-05T08:05:31Z

tests/e2e/volumeproperty_test.go

+
+	// Verifying docker cli and admin cli properties of a volume are same
+	status := reflect.DeepEqual(dockerCliMap, adminCliMap)
+	c.Assert(status, Equals, true, Commentf("Admin cli and docker cli properties of volumes are not same"))


If status is false, its good to print the maps, when an error happens we can immediately figure what the issue is, from the logs.

govint · 2017-06-05T08:06:14Z

tests/e2e/volumeproperty_test.go

+		s.getVolumePropertiesDockerCli(c, volumeName)
+	}
+
+	// Verifying docker cli and admin cli properties of a volume are same


Verify docker and ESX properties of "all three volumes" are the same. Thats what we are doing, using the CLIs on both platforms and figuring they have the same values.

govint · 2017-06-05T08:58:14Z

tests/e2e/volumeproperty_test.go

+			expectedValues[volumeName] = [3]string{size, s.formatTypes[i], diskStatusDetached}
+		}
+	}
+	status := reflect.DeepEqual(expectedValues, adminCliMap)


Aren't we comparing the docker-side volume props for three volumes with ESX-side volume props for all volumes. DeepEqual will fail here, unless its ensured that there are only three volumes in ESX. IN whcih case set up suite() should delete all volumes in the vmgroup to ensure only the three volumes will exist in the vmgroup.

govint · 2017-06-05T09:03:00Z

tests/e2e/volumeproperty_test.go

+func (s *VolumePropertyTestSuite) verifyProperties(c *C) {
+	for i, volumeName := range s.volumeNames {
+		if s.volumeAttached {
+			tmp := expectedValues[s.volumeNames[0]]


If this is the first call; to verifyProperties then expectedValues[] won't have the key (volume names) and will cause a panic of the GO run time.need to do like,
tmp, ok := expectedValues[volumeName]; ok == true {
_ assign the volume name_
}

s.volumeAttached is 'true' only if volume is attached. So this piece of code is only executed if volume is attached else map of expected values is populated. I had tested it locally and it ran fine. So things work fine even for first call.

govint · 2017-06-05T09:05:04Z

tests/e2e/volumeproperty_test.go

+// Only for 'thin', after volume is attached, modifying attached-to-vm field with vm name.
+func (s *VolumePropertyTestSuite) verifyProperties(c *C) {
+	for i, volumeName := range s.volumeNames {
+		if s.volumeAttached {


This is common for all volumes and hence when thin disk is attached, we will always go thru this path and mark all with the docker host name like as though they are attached, which isn't the case.

No, this is not what's happening. Please see the below output (map of expected values) when thin volume is attached.

2017/06/05 20:53:26 Expected values: map[thin_volume_1496695990:[100MB thin ubuntu-VM2.3] zeroedthick_volume_1496695993:[100MB zeroedthick detached] eagerzeroedthick_volume_1496695995:[100MB eagerzeroedthick detached]]

govint · 2017-06-05T09:20:38Z

tests/utils/verification/volumeproperties.go

+	admincliValues := strings.Fields(out)
+	adminCliMap := make(map[string][3]string)
+	for i := 0; i < len(admincliValues); {
+		adminCliMap[admincliValues[i]] = [3]string{admincliValues[i+1], admincliValues[i+2], admincliValues[i+3]}


[3] isn't needed []string{......} is ok

govint · 2017-06-05T09:20:55Z

tests/utils/verification/volumeproperties.go

 	}
-	return op
+	return adminCliMap


Its a global, does it need to be returned?

Its global in a test file tests/e2e/volumeproperty_test.go and not under tests/utils/verification/volumeproperties.go

shaominchen

Looks good. Please address merge conflicts. A few comments below.

shaominchen · 2017-06-07T01:44:45Z

tests/utils/verification/volumeproperties.go

-			"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op)
+// GetPropertiesForAllVolumesAdminCli returns a map of all the volumes from ESX and their corresponding
+// properties returned - capacity, attached-to-vm and disk-format
+func GetPropertiesForAllVolumesAdminCli(hostName string) map[string][]string {


I strongly suggest to add a common util function to return a map of all volume properties in which the key is the property name and the value is the property value. That will be very useful to avoid such complicated function which is only useful for a specific test.

Can we file a issue to track this improvement?

Filed issue #1365

I would prefer to have Map(volumeName,VolPropStruct) where volume name is the key and Struct object is for various properties.

struct VolPropStruct { capacity attached-to-vm disk-format }

shaominchen · 2017-06-07T01:47:05Z

tests/e2e/volumeproperty_test.go

+	s.containerName = inputparams.GetContainerNameWithTimeStamp(testName)
+	s.formatTypes = []string{"thin", "zeroedthick", "eagerzeroedthick"}
+	s.dockerVolumeRemoveCmd = dockerconst.RemoveVolume
+	log.Println(" SETUP Finished: volumeproperty_test.TestVolumeProperties")


Nit: remove the space before "SETUP"

Ok. I will remove it.

shaominchen · 2017-06-07T01:49:38Z

tests/e2e/volumeproperty_test.go

+}
+
+var _ = Suite(&VolumePropertyTestSuite{})
+var dockerCliMap = make(map[string][]string)


Can we add some comments for these 3 maps?

Ok. I will add it.

shuklanirdesh82 · 2017-06-07T03:36:29Z

tests/utils/verification/volumeproperties.go

-			"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op)
+// GetPropertiesForAllVolumesAdminCli returns a map of all the volumes from ESX and their corresponding
+// properties returned - capacity, attached-to-vm and disk-format
+func GetPropertiesForAllVolumesAdminCli(hostName string) map[string][]string {


The method name is too big .. it should be simplified .. how about his GetAllVolProperties and the method doc mentions how are we retrieving.

Ok. I will change it.

Possibly also remove the AdminCLI and DockerCLI from the function names

shuklanirdesh82 · 2017-06-07T03:44:13Z

tests/e2e/volumeproperty_test.go

+
+const (
+	testName           = "volumeproperty"
+	diskStatusDetached = "detached"


better to use https://github.com/vmware/docker-volume-vsphere/blob/master/tests/constants/properties/properties.go#L21

shuklanirdesh82 · 2017-06-07T03:45:05Z

tests/e2e/volumeproperty_test.go

+	volumeNames           []string
+	containerName         string
+	formatTypes           []string
+	dockerVolumeRemoveCmd string


this should be removed and used the const directly.

why? and how?

Addressed it.

shuklanirdesh82 · 2017-06-07T03:46:43Z

tests/e2e/volumeproperty_test.go

+	s.containerName = inputparams.GetContainerNameWithTimeStamp(testName)
+	s.formatTypes = []string{"thin", "zeroedthick", "eagerzeroedthick"}
+	s.dockerVolumeRemoveCmd = dockerconst.RemoveVolume
+	log.Println(" SETUP Finished: volumeproperty_test.TestVolumeProperties")


This should be replaced with something similar to https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/misc/misc.go#L53 .. you may introduce new log util.

on a second thought .. can we remove this line? IMO, it will not be helpful by looking at SetUpTest .. if agree please remove otherwise share more details.

shuklanirdesh82 · 2017-06-07T03:47:05Z

tests/e2e/volumeproperty_test.go

+*/
+
+func (s *VolumePropertyTestSuite) TestVolumeProperties(c *C) {
+	log.Printf("START: volumeproperty_test.TestVolumeProperties")


should be replaced with https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/misc/misc.go#L53

shuklanirdesh82 · 2017-06-07T03:47:20Z

tests/e2e/volumeproperty_test.go

+	// Verify if docker and ESX properties of volumes are same and as expected.
+	s.verifyProperties(c)
+
+	log.Printf("END: volumeproperty_test.TestVolumeProperties")


let's use appropriate util from https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/misc/misc.go#L53

shuklanirdesh82 · 2017-06-07T03:50:46Z

tests/e2e/volumeproperty_test.go

+}
+
+// getVolumeProperties - get properties of three volumes from ESX and docker host
+func (s *VolumePropertyTestSuite) getVolumeProperties(c *C) {


sorry need to improve ... as this method is not returning anything .. we should avoid using getXXX; it spoils objective of get (leads to confusion)

shuklanirdesh82 · 2017-06-07T03:53:17Z

tests/e2e/volumeproperty_test.go

+}
+
+// getVolumePropertiesAdminCli - get properties of a volume from ESX
+func (s *VolumePropertyTestSuite) getVolumePropertiesAdminCli(c *C, volumeName string) {


it is better to return the map from this and let caller to handle and process instead of setting global variable .. it spoils the code continuation and hard to track who sets it.

shuklanirdesh82 · 2017-06-07T03:55:00Z

tests/utils/verification/volumeproperties.go

+	log.Printf("Getting size, disk-format and attached-to-vm for volume [%s] from ESX [%s] using admin cli. \n", volumeName, hostName)
+	cmd := admincli.ListVolumes + "-c volume,capacity,disk-format,attached-to 2>/dev/null | grep " + volumeName + " | awk -v OFS='\t' '{print $2, $3, $4}' "
+	out, _ := ssh.InvokeCommand(hostName, cmd)
+	admincliValues := strings.Fields(out)


it is better to return from here itself

shuklanirdesh82

Overall looks good to me! Now proposed code looks much better and cleaner.

shuklanirdesh82 · 2017-06-07T03:56:09Z

tests/e2e/swarm_test.go

@@ -112,7 +112,7 @@ func (s *SwarmTestSuite) TestDockerSwarm(c *C) {
 	status, host := verification.IsDockerContainerRunning(s.swarmNodes, s.serviceName, 2)
 	c.Assert(status, Equals, true, Commentf("Container %s is not running on any hosts", s.serviceName))

-	containerName, err = dockercli.GetContainerName(host, s.serviceName+".1")
+	containerName, err := dockercli.GetContainerName(host, s.serviceName+".1")


taken care .. need to remove

shuklanirdesh82 · 2017-06-07T03:59:10Z

tests/e2e/volumeproperty_test.go

+	s.containerName = inputparams.GetContainerNameWithTimeStamp(testName)
+	s.formatTypes = []string{"thin", "zeroedthick", "eagerzeroedthick"}
+	s.dockerVolumeRemoveCmd = dockerconst.RemoveVolume
+	log.Println(" SETUP Finished: volumeproperty_test.TestVolumeProperties")


on a second thought .. can we remove this line? IMO, it will not be helpful by looking at SetUpTest .. if agree please remove otherwise share more details.

shuklanirdesh82 · 2017-06-07T03:59:32Z

tests/e2e/volumeproperty_test.go

+	s.dockerVolumeRemoveCmd = s.dockerVolumeRemoveCmd + strings.Join(s.volumeNames, " ")
+	out, err = ssh.InvokeCommand(s.config.DockerHosts[1], s.dockerVolumeRemoveCmd)
+	c.Assert(err, IsNil, Commentf(out))
+	log.Println(" CLEAN-UP Finished: volumeproperty_test.TestVolumeProperties")


same as above .. not needed.

Is it needed to be done for every test if not then lets do this in TearDownSuite() else it happens for every test.

shuklanirdesh82 · 2017-06-07T04:04:56Z

tests/utils/verification/volumeproperties.go

+	out, _ := ssh.InvokeCommand(hostName, cmd)
+	admincliValues := strings.Fields(out)
+	adminCliMap := make(map[string][]string)
+	for i := 0; i < len(admincliValues); {


any reason for not using struct object?

govint

Minor comments.

govint · 2017-06-07T04:38:48Z

tests/e2e/volumeproperty_test.go

+	s.config = inputparams.GetTestConfig()
+	if s.config == nil {
+		c.Skip("Unable to retrieve test config, skipping volumeproperty_test.TestVolumeProperties.")
+	}


Please do this only in SetUpSuite(), SetUpTest() runs for every single test.

govint · 2017-06-07T04:40:04Z

tests/e2e/volumeproperty_test.go

+	s.dockerVolumeRemoveCmd = s.dockerVolumeRemoveCmd + strings.Join(s.volumeNames, " ")
+	out, err = ssh.InvokeCommand(s.config.DockerHosts[1], s.dockerVolumeRemoveCmd)
+	c.Assert(err, IsNil, Commentf(out))
+	log.Println(" CLEAN-UP Finished: volumeproperty_test.TestVolumeProperties")


Is it needed to be done for every test if not then lets do this in TearDownSuite() else it happens for every test.

govint · 2017-06-07T12:42:42Z

tests/utils/verification/volumeproperties.go

-			"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op)
+// GetPropertiesForAllVolumesAdminCli returns a map of all the volumes from ESX and their corresponding
+// properties returned - capacity, attached-to-vm and disk-format
+func GetPropertiesForAllVolumesAdminCli(hostName string) map[string][]string {


Possibly also remove the AdminCLI and DockerCLI from the function names

shuklanirdesh82

contingent on CI pass.

ashahi1 self-assigned this May 30, 2017

vmwclabot added the cla-not-required label May 30, 2017

ashahi1 requested review from shuklanirdesh82, shaominchen and pshahzeb May 30, 2017 21:23

govint reviewed May 31, 2017

View reviewed changes

shuklanirdesh82 suggested changes May 31, 2017

View reviewed changes

shaominchen reviewed May 31, 2017

View reviewed changes

shuklanirdesh82 reviewed Jun 2, 2017

View reviewed changes

govint reviewed Jun 2, 2017

View reviewed changes

shaominchen suggested changes Jun 2, 2017

View reviewed changes

shuklanirdesh82 mentioned this pull request Jun 2, 2017

Some e2e util functions are not generating unique numbers properly #1344

Closed

ashahi1 force-pushed the optimizeVolumePropTest.ashahi1 branch 2 times, most recently from 3c1a33c to c3932a7 Compare June 5, 2017 06:32

govint reviewed Jun 5, 2017

View reviewed changes

ashahi1 force-pushed the optimizeVolumePropTest.ashahi1 branch 3 times, most recently from 680ba57 to 42d6fdb Compare June 6, 2017 21:42

shaominchen approved these changes Jun 7, 2017

View reviewed changes

shuklanirdesh82 reviewed Jun 7, 2017

View reviewed changes

shuklanirdesh82 suggested changes Jun 7, 2017

View reviewed changes

govint reviewed Jun 7, 2017

View reviewed changes

Addressed minor review comments from Sam, Nirdesh and Govindan

711e966

ashahi1 force-pushed the optimizeVolumePropTest.ashahi1 branch from 42d6fdb to 711e966 Compare June 7, 2017 22:21

shuklanirdesh82 approved these changes Jun 7, 2017

View reviewed changes

shuklanirdesh82 merged commit 745c52a into vmware-archive:master Jun 7, 2017

shuklanirdesh82 mentioned this pull request Jun 7, 2017

Optimize TestVolumeProperties #1096

Closed

ashahi1 deleted the optimizeVolumePropTest.ashahi1 branch June 26, 2017 19:35


		con "github.com/vmware/docker-volume-vsphere/tests/constants/dockercli"

Optimizing volumepropverification_test #1310

Optimizing volumepropverification_test #1310

Conversation

ashahi1 commented May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashahi1 commented May 30, 2017 •

edited

Loading

govint May 31, 2017 •

edited

Loading

govint May 31, 2017 •

edited

Loading

govint May 31, 2017 •

edited

Loading

govint Jun 2, 2017 •

edited

Loading