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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
