Nir Soffer has posted comments on this change. Change subject: tests: new functional tests for vdsm storage ......................................................................
Patch Set 2: (6 comments) 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: > I think verify.thingHappened() is nicer than verifier.thingHappened, and I When you see self.verify, you expect it to be a boolean, not something that verify things. Lets see if others like this convention. Also your methods are not "thingHappend" - for example: verify.assertPathExists - it does not make sense verify.pathExists - makes sense And you have these strange methods that are not about verifying: verify.waitForVdsmToFinish verify.waitFor verify.waitOnVdsmTask Maybe you mean: verify.taskFinished So if you want to go with Verify, name the method correctly. 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): > I disagree. This tells you why we're waiting. But this does not wait until something, it is confusing the reader. If you like to have this method, implement it correctly. I you like to sleep, use time.sleep. Line 27: time.sleep(duration) Line 28: Line 29: def storagePoolCreated(self, poolID, masterDomainID): Line 30: self.waitForVDSMToFinishTask() 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) > the idea is to check for some observable difference. I'm open to suggestion Well this is indeed the purpose of the patch :-) But do we have a test fro starting spm? If not, we don't have to verify this at all. When we do test for start spm, we should test two hosts, and verify that if we start spm on one of them, the other will fail to start spm. We don't care what start spm does in this level of test. 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): > YAGNI. When some lunatic calls this function with a float and it fails, he This may confuse users of this method. There is no clue that timeout is not the usual float that is used everywhere in Python. 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() > I wrote "wait for" not "wait to". The English is just fine. How about wait_until_task_finish? Do you create task that do not run in vdsm in the tests? 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 > I don't do shorthand, sorry. masterDomain? masterPath, path? directory? Why do we need so long name for temporary variable? 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) ) -- 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
