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,