Change in vdsm[master]: <tests>: Add NetworkTest.testAddDelNetworkDhcp
danken at redhat.com
danken at redhat.com
Mon Nov 11 22:37:36 UTC 2013
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 <pbenas at 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 <pbenas at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list