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

nsoffer at redhat.com nsoffer at redhat.com
Thu Sep 4 22:32:59 UTC 2014


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 <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