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