Change in vdsm[master]: Add ut to supervdsmServer zombie reaper

ybronhei at redhat.com ybronhei at redhat.com
Tue Nov 20 15:07:57 UTC 2012


Yaniv Bronhaim has posted comments on this change.

Change subject: Add ut to supervdsmServer zombie reaper
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(3 inline comments)

Thanks! Some small comments.. Still working on http://gerrit.ovirt.org/#/c/9278/ to actually run the tests.

....................................................
File tests/superVdsmTests.py
Line 16:         # temporary values to run temporary svdsm
Line 17:         self.pidfd, pidfile = tempfile.mkstemp()
Line 18:         self.timefd, timestamp = tempfile.mkstemp()
Line 19:         self.addfd, address = tempfile.mkstemp()
Line 20:         self.pidfile = pidfile
why do you need to keep the file name ?
Line 21: 
Line 22:         self._proxy.setIPCPaths(pidfile, timestamp, address)
Line 23:         self._proxy.ping()
Line 24:         self.assertTrue(self._proxy.isRunning())


Line 19:         self.addfd, address = tempfile.mkstemp()
Line 20:         self.pidfile = pidfile
Line 21: 
Line 22:         self._proxy.setIPCPaths(pidfile, timestamp, address)
Line 23:         self._proxy.ping()
You've changed my tests flow here, the point was to test if calling to svdsm ping function will start and initialize super vdsm process. With your change we won't know if something is wrong with super vdsm initialization, because during the setUp we already start it. This is the reason for testIsSuperUp that you omit... But if you prefer it that way, I accept the change, just remove all testIsSuperUp function and write a comment next to ping() call that it will cause svdsm execution (because you can't really understand it from reading the code)
Line 24:         self.assertTrue(self._proxy.isRunning())
Line 25: 
Line 26:     def tearDown(self):
Line 27:         for fd in (self.pidfd, self.timefd, self.addfd):


....................................................
File vdsm/supervdsmServer.py
Line 89: 
Line 90: class _SuperVdsm(object):
Line 91: 
Line 92:     @logDecorator
Line 93:     def ping(self, *args, **kwargs):
I think you should add another function to check this, because when I wrote ping I met that all it does is to verify that a message got to svdsm and the return value returned. here you call to _runAs that can throw exception or whatever that can happened and it changes the meaning of ping...
Line 94:         # This method exists for testing purposes
Line 95:         res = self._runAs(DISKIMAGE_USER, (DISKIMAGE_GROUP,), os.getpid)
Line 96:         return res
Line 97: 


--
To view, visit http://gerrit.ovirt.org/9354
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fb8f3dcd575266febe11967e3f8df7d3588acb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list