Giuseppe Vallarelli has posted comments on this change.
Change subject: iproute2 binary wrapper
......................................................................
Patch Set 18: (2 inline comments)
There a couple of improvements that can be made that do not require lots of
efforts.
....................................................
File tests/ipwrapperTests.py
Line 23:
Line 24: from testrunner import VdsmTestCase as TestCaseBase
Line 25:
Line 26:
Line 27: class TestIpwrapper(TestCaseBase):
Unit tests are named unit test for a reason, they should test a unit of
behaviour if everything is conflated in one test it takes more time to
create/read the test. Do as you prefer.
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)
You're supposed to test a Route object not 4 attributes. What you're doing here
is testing the internal representation, you should ask the object if it's equal
to another one, you shouldn't do this by hand...
It may be not the case but If you add to a Route different attributes would you
remember to update this test ? It's highly unlikely that you would notice.
Tests are supposed to fail, when a regression is introduced, if they don't fail
there's a big problem, they're testing only a little happy path, but ehy we
have tests! :-)
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':
--
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]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
