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