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