Giuseppe Vallarelli has posted comments on this change. Change subject: Functional Tests [2/3]: Added ipwrapper.ruleExists+routeExists ......................................................................
Patch Set 1: Code-Review-1 (4 comments) It's pretty good very minor comments. .................................................... File lib/vdsm/ipwrapper.py Line 246: Line 247: Line 248: def _validate(validator, entry): Line 249: try: Line 250: validator.fromText(entry) I don't understand this choice of having fromText raising an exception and using the exception as a signal of meaning entry valid or invalid. fromText should return already this boolean instead of having clients of this method wrapping it in try..except. Line 251: except: Line 252: return False Line 253: else: Line 254: return True .................................................... File tests/functional/networkTests.py Line 21: from testrunner import (VdsmTestCase as TestCaseBase, Line 22: expandPermutations, permutations) Line 23: from testValidation import RequireDummyMod, ValidateRunningAsRoot Line 24: Line 25: import dummyUtils I found the Utils word a bit overused, why not simply dummy we already have an utils module in tests/functional Line 26: from dummyUtils import dummyIf Line 27: from utils import cleanupNet, restoreNetConfig, SUCCESS, VdsProxy Line 28: Line 29: from vdsm.ipwrapper import ruleAdd, ruleDel, routeAdd, routeDel, routeExists, \ Line 25: import dummyUtils Line 26: from dummyUtils import dummyIf Line 27: from utils import cleanupNet, restoreNetConfig, SUCCESS, VdsProxy Line 28: Line 29: from vdsm.ipwrapper import ruleAdd, ruleDel, routeAdd, routeDel, routeExists, \ Don't use \ use parenthesis instead (...) Line 30: ruleExists, Route, Rule Line 31: Line 32: Line 33: NETWORK_NAME = 'test-network' Line 410: @ValidateRunningAsRoot Line 411: def testRuleExists(self): Line 412: with dummyIf(1) as nics: Line 413: nic = nics[0] Line 414: dummyUtils.setIP(nic, IP_ADDRESS + '/' + IP_CIDR) you're creating IP_ADDRESS '/' + IP_CIDR twice across the two tests can we do only once? You can set up vars shared by more tests in the setUp. Line 415: dummyUtils.setLinkUp(nic) Line 416: Line 417: rules = [Rule(source=IP_NETWORK_AND_CIDR, table=IP_TABLE), Line 418: Rule(destination=IP_NETWORK_AND_CIDR, table=IP_TABLE, -- To view, visit http://gerrit.ovirt.org/18040 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf7c28e85f53f51879750308f3a9f93fc635cf33 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Assaf Muller <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Assaf Muller <[email protected]> Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
