Antoni Segura Puimedon has posted comments on this change. Change subject: tests: addNetwork bond with one or many vlans. ......................................................................
Patch Set 3: I would prefer that you didn't submit this (4 inline comments) Good work with the tests. Just some minor comments. .................................................... File tests/functional/networkTests.py Line 301: @cleanupNet Line 302: @permutations([[True], [False]]) Line 303: @RequireDummyMod Line 304: @ValidateRunningAsRoot Line 305: def testAddNetworkBondWithManyVlans(self, brdgd): I'd rather have bridged that brdgd Line 306: with dummyIf(1) as nics: Line 307: VLAN_COUNT = 5 Line 308: NET_VLANS = [(NETWORK_NAME + str(id), str(id)) Line 309: for id in xrange(VLAN_COUNT)] Line 304: @ValidateRunningAsRoot Line 305: def testAddNetworkBondWithManyVlans(self, brdgd): Line 306: with dummyIf(1) as nics: Line 307: VLAN_COUNT = 5 Line 308: NET_VLANS = [(NETWORK_NAME + str(id), str(id)) as I asked in another patchset, s/id/index/ and s/xrange/range/ Line 309: for id in xrange(VLAN_COUNT)] Line 310: for net_vlan, vlan_id in NET_VLANS: Line 311: status, msg = self.vdsm_net.addNetwork(net_vlan, Line 312: vlan=vlan_id, Line 316: self.assertEquals(status, SUCCESS, msg) Line 317: self.assertTrue(self.vdsm_net.networkExists(net_vlan, Line 318: bridged=brdgd)) Line 319: for _, vlan_id in NET_VLANS: Line 320: msg = 'vlan %s doesn\'t exist' % vlan_id I think this is cleaner with "vlan %s doesn't exist" Line 321: vlan_name = '%s.%s' % (BONDING_NAME, vlan_id) Line 322: self.assertEqual(BONDING_NAME, Line 323: self.vdsm_net.getIfaceForVlan(vlan_name), msg) Line 324: .................................................... File tests/functional/utils.py Line 186: return bond_name in self.netinfo.bondings and \ Line 187: (not nics or set(nics) == Line 188: set(self.netinfo.bondings[bond_name]['slaves'])) Line 189: Line 190: def vlanExists(self, vlan_name): I'd rather add a 'device' opt parameter to vlanExists that allows you to pass the underlying device if you want to make sure that you got the right device under the vlan. Just like in bondExists Line 191: return vlan_name in self.netinfo.vlans Line 192: Line 193: def getIfaceForVlan(self, vlan_name): Line 194: try: -- To view, visit http://gerrit.ovirt.org/17387 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84b1582bbc9de09009a7e076386658035317f7db Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: Livnat Peer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
