Dan Kenigsberg has posted comments on this change. Change subject: <tests>: Add NetworkTest.testAddDelNetworkDhcp ......................................................................
Patch Set 2: Code-Review-1 (10 comments) .................................................... Commit Message Line 3: AuthorDate: 2013-11-10 19:37:57 +0100 Line 4: Commit: Petr Benas <pbe...@redhat.com> Line 5: CommitDate: 2013-11-11 19:16:33 +0100 Line 6: Line 7: <tests>: Add NetworkTest.testAddDelNetworkDhcp please drop the angular brackets. Line 8: Line 9: Tests adding and deletion of network with dynamically Line 10: assigned address. The dhcp package is required for this Line 11: test, otherwise the test is skipped. .................................................... File tests/functional/Makefile.am Line 29: utils.py \ Line 30: virtTests.py \ Line 31: storageTests.py \ Line 32: veth.py \ Line 33: dhcpd.py \ please keep sorted Line 34: $(NULL) Line 35: Line 36: dist_vdsmfunctests_DATA = \ Line 37: 60_test_balloon_shrink.policy \ .................................................... File tests/functional/dhcpd.py Line 16: # Line 17: # Refer to the README and COPYING files for full details of the license Line 18: # Line 19: Line 20: import tempfile sort imports, please. Line 21: import os Line 22: Line 23: from nose.plugins.skip import SkipTest Line 24: Line 24: Line 25: from vdsm.utils import CommandPath Line 26: from vdsm.utils import execCmd Line 27: Line 28: _DHCPD_BINARY = CommandPath('dhcpd', '/sbin/dhcpd') you must raise SkipTest here, not later (please try!) Line 29: Line 30: dhcpdConfig = '''# minimalistic dhcpd configuration file Line 31: authoritative; Line 32: Line 37: Line 38: Line 39: class Dhcpd(): Line 40: def __init__(self): Line 41: self.configDir = '' no need for transient value for self.configDir. Initialize it to tempfile.mkdtemp() now. but why do you need a directory? I see a singe configuration file. Line 42: self.proc = '' Line 43: Line 44: def start(self, interface): Line 45: self._checkDhcpdInstalled() Line 38: Line 39: class Dhcpd(): Line 40: def __init__(self): Line 41: self.configDir = '' Line 42: self.proc = '' initialize to None Line 43: Line 44: def start(self, interface): Line 45: self._checkDhcpdInstalled() Line 46: self._writeConfigFile() Line 48: '/dhcpd.conf', interface], sync=False) Line 49: Line 50: def stop(self): Line 51: self.proc.kill() Line 52: execCmd(['rm', '-rf', self.configDir]) use shutil.rmtree(), no need to exec. Line 53: Line 54: def _writeConfigFile(self): Line 55: self.configDir = tempfile.mkdtemp() Line 56: f = open(self.configDir + '/dhcpd.conf', 'w') Line 52: execCmd(['rm', '-rf', self.configDir]) Line 53: Line 54: def _writeConfigFile(self): Line 55: self.configDir = tempfile.mkdtemp() Line 56: f = open(self.configDir + '/dhcpd.conf', 'w') use "with", it is a much better practice. Line 57: f.write(dhcpdConfig) Line 58: f.close() Line 59: Line 60: def _checkDhcpdInstalled(self): .................................................... File tests/functional/networkTests.py Line 1512: @permutations([[True], [False]]) Line 1513: @RequireVethMod Line 1514: @ValidateRunningAsRoot Line 1515: def testAddDelNetworkDhcp(self, bridged): Line 1516: with vethIf() as nics: the temporary "nics" variable is redundant with vethIf() as (right, left) btw, why "right" is on the left hand side of the pair? Line 1517: (right, left) = nics Line 1518: veth.setIP(right, IP_ADDRESS, IP_CIDR) Line 1519: veth.setLinkUp(right) Line 1520: Line 1518: veth.setIP(right, IP_ADDRESS, IP_CIDR) Line 1519: veth.setLinkUp(right) Line 1520: Line 1521: dhcpServer = dhcpd.Dhcpd() Line 1522: dhcpServer.start(right) this could be even cooler as a context manager. for your consideration. Line 1523: Line 1524: try: Line 1525: status, msg = self.vdsm_net.addNetwork(NETWORK_NAME, Line 1526: nics=[left], -- To view, visit http://gerrit.ovirt.org/21113 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9dbb59f3cd420b2071eb0ec42f9816ab52151bce Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Benas <pbe...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches