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

nsoffer at redhat.com nsoffer at redhat.com
Thu Sep 4 17:31:49 UTC 2014


Nir Soffer has posted comments on this change.

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


Patch Set 2:

(7 comments)

Some more comments.

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

Line 2: import logging
Line 3: import vdsm.vdscli
Line 4: import socket
Line 5: import vdsm.config
Line 6: import time
Use this scheme for imports:

stdlib imports

vdsm imports

testlib imports

And keep imports sorted in each group.
Line 7: 
Line 8: class ControlVDSM:
Line 9:     def cleanup(self):
Line 10:         self._stopService()


Line 4: import socket
Line 5: import vdsm.config
Line 6: import time
Line 7: 
Line 8: class ControlVDSM:
Please rename the module to vdsm and this class to Controller:

Then we will use it as:

    from . import vdsm.Controller
Line 9:     def cleanup(self):
Line 10:         self._stopService()
Line 11:         assert not self._serviceRunning()
Line 12:         self._brutallyCleanFiles()


Line 22:                 vdsmClient.getStorageDomainsList()
Line 23:                 logging.info('connection to VDSM succeeded')
Line 24:                 return
Line 25:             except socket.error as e:
Line 26:                 logging.warning('could not talk to VDSM: %s' % e)
I think we just talked to vdsm, but it would not listen :-)

"Error communicating with vdsm"?
Line 27:                 time.sleep(1)
Line 28: 
Line 29:         raise Exception('could not connect to VDSM')
Line 30: 


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?
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 your command line, specify the arguments to exec.
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.
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/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.

We should run these tests with:

    nosetests funcional/storage.py

This script does not allow running the tests with additional options to the nosetests module. This solution is bad and we should not repeat this bad idea in new tests.


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