Zhou Zheng Sheng has posted comments on this change.
Change subject: Adding supervdsm unit tests
......................................................................
Patch Set 11: (2 inline comments)
The idea is good. Just two minor problems.
....................................................
File tests/superVdsmTests.py
Line 15: # changing default constants for test. remember to return the
origin
Line 16: # value
Line 17: self._originpidfile = self._proxy.pidfile
Line 18: self._origintimestempfile = self._proxy.timestamp
Line 19: self._originsock = self._proxy.address
1) I think it needn't remember the default values and then restore them. The
default values are for running superVdsm in production, the generated values
are for testing. The default values are useless for us in UT.
2) I think it's hard to remember to set all of the generated file path and
their names. The relationship of those file paths are not very clear. User of
the SuperVdsmProxy have to remember to set all of the 3 values in ad-hoc
manner. If new files added, need to remember to set more values. The names of
the value are not related to each other neither. Without reading the code and
the comment, I can not get any clue to use the interface properly. You can just
add a method named SuperVdsmProxy.setIPCPaths() to explicitly express the
relationship of those file paths. In the test case, you can just invoke the
SuperVdsmProxy.setIPCPaths() duiring setUp(). For example, in supervdsm.py, it
could be.
class SuperVdsmProxy(object):
...
def setIPCPaths(self, pidfile, timestamp, address):
self._pidfile = pidfile
self._timestamp = timestamp
self._address = address
...
Line 20:
Line 21: # temporary values to run temporary svdsm
Line 22: self.pidfd, self._proxy.pidfile = tempfile.mkstemp()
Line 23: self.timefd, self._proxy.timestamp = tempfile.mkstemp()
....................................................
File vdsm/supervdsmServer.py
Line 298: while True:
Line 299: os.kill(parentPid, 0)
Line 300: sleep(2)
Line 301: except Exception:
Line 302: if address is not None and os.path.exists(address):
This is a bit defensive. The function wants to delete the "address" quietly. I
suggest remove the check for "address is not None". This is because address is
in your function argument list, if address is None, the caller code is written
incorrectly, meaning the programme logic is broken. This function is not
designed for the case where address can be wrong. So just let it explode to
expose the bugs of the caller more quickly so that we can fix them.
Line 303: os.unlink(address)
Line 304: os.kill(os.getpid(), signal.SIGTERM)
Line 305:
Line 306:
--
To view, visit http://gerrit.ovirt.org/8600
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I08880b12db8a6ca9cc57d74d2ef2a86980a5b097
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
