Change in vdsm[master]: tests: new functional tests for vdsm storage

ykleinbe at redhat.com ykleinbe at redhat.com
Thu Sep 4 21:41:30 UTC 2014


Yoav Kleinberger has posted comments on this change.

Change subject: tests: new functional tests for vdsm storage
......................................................................


Patch Set 2:

(13 comments)

http://gerrit.ovirt.org/#/c/32496/2/tests/functional/testlib/controlvdsm.py
File tests/functional/testlib/controlvdsm.py:

Line 25:             except socket.error as e:
Line 26:                 logging.warning('could not talk to VDSM: %s' % e)
Line 27:                 time.sleep(1)
Line 28: 
Line 29:         raise Exception('could not connect to VDSM')
> It this a test error or failure?
Error. Could be a million reasons, and this is not in the flow of the test.
Line 30: 
Line 31:     def _stopService(self):
Line 32:         self._run("sudo service vdsmd stop")
Line 33: 


Line 28: 
Line 29:         raise Exception('could not connect to VDSM')
Line 30: 
Line 31:     def _stopService(self):
Line 32:         self._run("sudo service vdsmd stop")
> Don't use strings for running command, only lists. Don't let Python parse y
see my reply below on the same issue.
Line 33: 
Line 34:     def _serviceRunning(self):
Line 35:         returnCode = subprocess.call('sudo service vdsmd status', shell=True, stdout=open('/dev/null','w'), stderr=open('/dev/null','w'))
Line 36:         logging.info('vdsm running: %s' % (returnCode == 0))


Line 31:     def _stopService(self):
Line 32:         self._run("sudo service vdsmd stop")
Line 33: 
Line 34:     def _serviceRunning(self):
Line 35:         returnCode = subprocess.call('sudo service vdsmd status', shell=True, stdout=open('/dev/null','w'), stderr=open('/dev/null','w'))
> Never use shell=True.
My guess is that you're against this because it's a potential security issue. But this is test code, not production code. It's more readable this way than using annoying lists.
Line 36:         logging.info('vdsm running: %s' % (returnCode == 0))
Line 37:         return returnCode == 0
Line 38: 
Line 39:     def _restartService(self):


http://gerrit.ovirt.org/#/c/32496/2/tests/functional/testlib/testcontexts/base.py
File tests/functional/testlib/testcontexts/base.py:

Line 5: import uuid
Line 6: import logging
Line 7: import time
Line 8: 
Line 9: class Verify:
> Verifier?
I think verify.thingHappened() is nicer than verifier.thingHappened, and I like to call the object by the same word as the class.
Line 10:     def __init__(self, vdsm):
Line 11:         self._vdsm = vdsm
Line 12: 
Line 13:     def assertPathExists(self, path, link = False):


Line 22:             assert not os.path.lexists(path)
Line 23:         else:
Line 24:             assert not os.path.exists(path)
Line 25: 
Line 26:     def waitForVDSMToFinishTask(self, duration = 1):
> This a lie - there is no point in having this method. Just use time.sleep()
I disagree. This tells you why we're waiting.
I could use the pattern
  SPECIFY_REASON = 1
  time.sleep( SPECIFY_REASON )

but it would get repetitive (I have a lot of this)
Line 27:         time.sleep(duration)
Line 28: 
Line 29:     def storagePoolCreated(self, poolID, masterDomainID):
Line 30:         self.waitForVDSMToFinishTask()


Line 28: 
Line 29:     def storagePoolCreated(self, poolID, masterDomainID):
Line 30:         self.waitForVDSMToFinishTask()
Line 31:         linkToDomain = os.path.join('/rhev/data-center', poolID, masterDomainID)
Line 32:         masterAliasLink = os.path.join('/rhev/data-center', poolID, 'mastersd')
> Why not "link_to_master_domain" - use consistent names.
will fix
Line 33:         logging.info('verifying domain %s in pool %s' % (masterDomainID, poolID))
Line 34:         assert os.path.lexists(linkToDomain)
Line 35:         logging.info('verifying symlink to master domain')
Line 36:         assert os.path.lexists(masterAliasLink)


Line 32:         masterAliasLink = os.path.join('/rhev/data-center', poolID, 'mastersd')
Line 33:         logging.info('verifying domain %s in pool %s' % (masterDomainID, poolID))
Line 34:         assert os.path.lexists(linkToDomain)
Line 35:         logging.info('verifying symlink to master domain')
Line 36:         assert os.path.lexists(masterAliasLink)
> I don't think we need to check the files here, we can test this in a unitte
the idea is to check for some observable difference. I'm open to suggestions.
Line 37: 
Line 38:     def waitFor(self, timeout, description, predicate, *args, **kwargs):
Line 39:         logging.info('waiting for "%s"' % description)
Line 40:         start = time.time()


Line 34:         assert os.path.lexists(linkToDomain)
Line 35:         logging.info('verifying symlink to master domain')
Line 36:         assert os.path.lexists(masterAliasLink)
Line 37: 
Line 38:     def waitFor(self, timeout, description, predicate, *args, **kwargs):
> This should accept only integers, so lest make it clear by calling it "seco
YAGNI. When some lunatic calls this function with a float and it fails, he is more than welcome to change names/implementation.
Line 39:         logging.info('waiting for "%s"' % description)
Line 40:         start = time.time()
Line 41:         for _ in xrange(timeout):
Line 42:             if predicate(*args, **kwargs):


Line 46: 
Line 47:         assert False, 'waited %s seconds for "%s" but it did not happen' % (timeout, description)
Line 48: 
Line 49:     def spmStarted(self, poolID):
Line 50:         self.waitForVDSMToFinishTask()
> "wait to vdsm to finish task" is not english. "wait until vdsm finishes tas
I wrote "wait for" not "wait to". The English is just fine.

I can't use self.vdsm here without considerable effort. I agree it's not the most beautifull name, but it conveys the meaning.
Line 51:         masterDomainDirectory = '/rhev/data-center/%s/mastersd' % poolID
Line 52:         master = os.path.join(masterDomainDirectory, 'master')
Line 53:         tasks = os.path.join(master, 'tasks')
Line 54:         vms = os.path.join(master, 'vms')


Line 47:         assert False, 'waited %s seconds for "%s" but it did not happen' % (timeout, description)
Line 48: 
Line 49:     def spmStarted(self, poolID):
Line 50:         self.waitForVDSMToFinishTask()
Line 51:         masterDomainDirectory = '/rhev/data-center/%s/mastersd' % poolID
> Too long - we don't need so much context in a temporary. "mastersd" is enou
I don't do shorthand, sorry.
Line 52:         master = os.path.join(masterDomainDirectory, 'master')
Line 53:         tasks = os.path.join(master, 'tasks')
Line 54:         vms = os.path.join(master, 'vms')
Line 55:         allExist = lambda: ( os.path.exists(master) and os.path.exists(tasks) and os.path.exists(vms) )


Line 90:         self.verifyVDSMSuccess(result)
Line 91: 
Line 92:     def activateStorageDomain(self, domainID, poolID):
Line 93:         result = self._vdsm.activateStorageDomain(domainID,poolID)
Line 94:         self.verifyVDSMSuccess(result)
> You can see that every vdsm api need this verify - it should move to vdsm o
good idea.
Line 95: 
Line 96:     def stringForXMLRPC(self, number):
Line 97:         return str(number)
Line 98: 


Line 97:         return str(number)
Line 98: 
Line 99:     def verifyVDSMSuccess(self, result):
Line 100:         if result[ 'status' ][ 'code' ] != 0:
Line 101:             raise Exception('expected OK result from VDSM, got "%s" instead' % str(result))
> This should be part of the vdsm api. It should raise when vdsm failed, inst
OK


http://gerrit.ovirt.org/#/c/32496/2/tests/run_functional_storage_tests.sh
File tests/run_functional_storage_tests.sh:

Line 5: fi
Line 6: if [[ "$1" = "--verbose" ]]; then
Line 7:     verbose="--nologcapture"
Line 8: fi
Line 9: sudo PYTHONPATH=../lib:../vdsm:functional nosetests -s $verbose functional/basicStorageTest.py
> Please remove this wrapper, and do the needed setup in the test module.
I agree in principle - but will people really know how to run this?

perhaps include a, I hate myself now, comment explaining it?


-- 
To view, visit http://gerrit.ovirt.org/32496
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1703e7c1dc223ff707775865cd14c7dd62314caf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list