Giuseppe Vallarelli has posted comments on this change.
Change subject: iproute2 binary wrapper
......................................................................
Patch Set 18: I would prefer that you didn't submit this
(3 inline comments)
See comments they apply to both Route and Rule objects.
....................................................
File tests/ipwrapperTests.py
Line 23:
Line 24: from testrunner import VdsmTestCase as TestCaseBase
Line 25:
Line 26:
Line 27: class TestIpwrapper(TestCaseBase):
Missing tests for the constructors which may raise exceptions.
Line 28: def testRouteFromText(self):
Line 29: _getRouteAttrs = lambda x: (x.network, x.ipaddr, x.device,
x.table)
Line 30: good_routes = {
Line 31: 'default via 192.168.99.254 dev eth0':
Line 25:
Line 26:
Line 27: class TestIpwrapper(TestCaseBase):
Line 28: def testRouteFromText(self):
Line 29: _getRouteAttrs = lambda x: (x.network, x.ipaddr, x.device,
x.table)
Instead of _getRouteAttrs you may decide to implement the __eq__ equals method
and compare Route objects.
Line 30: good_routes = {
Line 31: 'default via 192.168.99.254 dev eth0':
Line 32: ('0.0.0.0/0', '192.168.99.254', 'eth0', None),
Line 33: 'default via 192.168.99.254 dev eth0 table foo':
Line 27: class TestIpwrapper(TestCaseBase):
Line 28: def testRouteFromText(self):
Line 29: _getRouteAttrs = lambda x: (x.network, x.ipaddr, x.device,
x.table)
Line 30: good_routes = {
Line 31: 'default via 192.168.99.254 dev eth0':
By reading the test fixture is pretty clear that there are 3 specific string
valid formats:
* default via X dev Y
* default via X dev Y table Z
* X via Y dev Z table W
I wonder if it could make sense to enforce it by letting clients use only one
of those formats and providing the needed parameters. Alternatively you can
document what kind of string you expect in the fromText methods (Route/Rule). I
will let you decide which way is the best.
Line 32: ('0.0.0.0/0', '192.168.99.254', 'eth0', None),
Line 33: 'default via 192.168.99.254 dev eth0 table foo':
Line 34: ('0.0.0.0/0', '192.168.99.254', 'eth0', 'foo'),
Line 35: '200.100.50.0/16 via 11.11.11.11 dev eth2 table foo':
--
To view, visit http://gerrit.ovirt.org/15335
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d315c3294fd7f058cdc840dea329d91a658a304
Gerrit-PatchSet: 18
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: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
