Change in vdsm[master]: Functional Tests [2/3]: Added ipwrapper.ruleExists+routeExists
gvallare at redhat.com
gvallare at redhat.com
Tue Aug 13 17:04:05 UTC 2013
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 <amuller at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list