[prev in list] [next in list] [prev in thread] [next in thread] 

List:       vdsm-patches
Subject:    Change in vdsm[master]: tests: setupNetworks add one or more vlans.
From:       asegurap () redhat ! com
Date:       2013-07-31 21:14:47
Message-ID: 201307312114.r6VLEmcj006287 () gerrit ! ovirt ! org
[Download RAW message or body]

Antoni Segura Puimedon has posted comments on this change.

Change subject: tests: setupNetworks add one or more vlans.
......................................................................


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

(4 inline comments)

Thanks for the tests Giuseppe!

....................................................
File tests/functional/networkTests.py
Line 347:     @RequireDummyMod
Line 348:     @ValidateRunningAsRoot
Line 349:     def testSetupNetworksAddManyVlans(self, bridged):
Line 350:         VLAN_COUNT = 5
Line 351:         NET_VLANS = [(NETWORK_NAME + str(id), str(id))
since id is a python builtin, could we use 'index' here to prevent shadowing?
Line 352:                      for id in xrange(VLAN_COUNT)]
Line 353: 
Line 354:         with dummyIf(1) as nics:
Line 355:             nic = nics[0]


Line 348:     @ValidateRunningAsRoot
Line 349:     def testSetupNetworksAddManyVlans(self, bridged):
Line 350:         VLAN_COUNT = 5
Line 351:         NET_VLANS = [(NETWORK_NAME + str(id), str(id))
Line 352:                      for id in xrange(VLAN_COUNT)]
xrange is not necessary, range should be preferred for forwards compatibility (and \
also because list comprehensions are already special cased and don't need the boost \
xrange used to give). Line 353: 
Line 354:         with dummyIf(1) as nics:
Line 355:             nic = nics[0]
Line 356:             networks = dict((vlan_net,


Line 351:         NET_VLANS = [(NETWORK_NAME + str(id), str(id))
Line 352:                      for id in xrange(VLAN_COUNT)]
Line 353: 
Line 354:         with dummyIf(1) as nics:
Line 355:             nic = nics[0]
That's a minor thing, but I much prefer to use unpacking since it detects wrong \
sizing of the list.

nic, = nics
Line 356:             networks = dict((vlan_net,
Line 357:                              {'vlan': str(tag), 'nic': nic,
Line 358:                               'bridged': bridged})
Line 359:                             for vlan_net, tag in NET_VLANS)


Line 368:                     self.assertTrue(self.vdsm_net.vlanExists(nic + '.' + \
tag)) Line 369: 
Line 370:             with self.vdsm_net.pinger():
Line 371:                 for vlan_net, tag in NET_VLANS:
Line 372:                     status, msg = self.vdsm_net.setupNetworks(
I would probably change this into a single setupNetworks that removes all the nets, \
just like the one in line 362 adds all the nets. Line 373:                         \
{vlan_net: {'remove': True}}, {}, {}) Line 374:                     \
self.assertEqual(status, SUCCESS, msg) Line 375:                     \
self.assertFalse(self.vdsm_net.networkExists(vlan_net, Line 376:                      \
bridged))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I62fa6de1faa7d4e886c45361a001b0009fd92502
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Giuseppe Vallarelli <gvallare@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken@redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare@redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches


[prev in list] [next in list] [prev in thread] [next in thread]