10:20 a.m.
Antoni Segura Puimedon has uploaded a new change for review.
Change subject: Added unittests for _addNetworkValidation, vlanId, validateBondingName and
validateBondingOptions.
......................................................................
Added unittests for _addNetworkValidation, vlanId, validateBondingName and
validateBondingOptions.
The test for validateBondingOptions monkey patches os.path.exists to allow to test
just the logic of the method, which uses sysfs to determine if the option is valid.
The test of _addNetworkValidation goes over all the logic branches to test that the
proper exceptions are thrown. It monkey patches the network detection method of the
netinfo module to avoid real network detection at test time.
Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Signed-off-by: Antoni Segura Puimedon <asegurap(a)redhat.com>
---
M tests/configNetworkTests.py
1 file changed, 204 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/98/6198/1
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
2:51 p.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Dan Kenigsberg has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 7: Looks good to me, approved
Thanks!
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
8:30 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Alon Bar-Lev has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 8:
Fixed at[1], in generic way.
[1] http://gerrit.ovirt.org/#/c/6939/
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
7:17 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Alon Bar-Lev has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 8:
Hello,
This change modifies python library functions, while not restore in case of exception
thrown.
As a result other modules and tests are effected.
Alon.
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
2:51 p.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Added tests for _addNetworkValidation and helpers.
Helpers are: validateVlanId, validateBondingName and validateBondingOptions.
The test for validateBondingOptions monkey patches os.path.exists to allow to
test just the logic of the method, which uses sysfs to determine if the option
is valid.
The test of _addNetworkValidation goes over the logic branches to test that
the proper exceptions are thrown. It monkey patches the network detection
method of the netinfo module to avoid real network detection at test time.
Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Signed-off-by: Antoni S. Puimedon <asegurap(a)redhat.com>
---
M tests/configNetworkTests.py
1 file changed, 228 insertions(+), 0 deletions(-)
Approvals:
Dan Kenigsberg: Looks good to me, approved
Igor Lvovsky: Verified
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
6:36 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Igor Lvovsky has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 7: Verified
works for me on RHEL-6
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
3:45 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Dan Kenigsberg has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 6: Looks good to me, approved
Toni, please have someone (and at the last resort - you) verify this on el6.
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
2 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Dan Kenigsberg has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 5: No score; I would prefer that you didn't submit this
yes, I am using Python 2.6. Since el6 has it, our project must build and run with it.
Please reimplement what's needed to make this work with 2.6.
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
9:50 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Antoni Segura Puimedon has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 5: (1 inline comment)
Are you running it with python 2.6? Python 2.7.3 works with it. If that is the case,
I'm going to change the block to make it 2.6 compatible.
....................................................
File tests/configNetworkTests.py
Line 126: return frozenset(re.findall(r'(\w+):', line)[1] for line in \
Done
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
9:29 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Dan Kenigsberg has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 5: Fails
(1 inline comment)
on my el6 host
Traceback (most recent call last):
File "/pvt/vdsm.git/tests/configNetworkTests.py", line 140, in
testValidateBondingOptions
with self.assertRaises(configNetwork.ConfigNetworkError) as cneContext:
TypeError: failUnlessRaises() takes at least 3 arguments (2 given)
....................................................
File tests/configNetworkTests.py
Line 126: return frozenset(re.findall(r'(\w+):', line)[1] for line in \
no need for this trailing backslash
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
2:45 p.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Dan Kenigsberg has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(2 inline comments)
Thanks, two minor comments.
....................................................
File tests/configNetworkTests.py
Line 60: configNetwork.ne.ERR_BAD_VLAN)
it's better to align args according to the opening parenthesis (unless there is a
serious lack of space)
Line 122: return [re.findall(r'(\w+):', line)[1] for line in
out.split('\n') \
we do not care about the order of options, we do not expect anyone to change to output,
but we do care about quick testing of "in" - we'd better return a
frozenset().
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
9:18 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Antoni Segura Puimedon has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File tests/configNetworkTests.py
Line 121: p = subprocess.Popen(['modinfo', 'bonding'],
Finally I memoized it here. We can move it at another occasion.
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
8:32 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Antoni Segura Puimedon has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 3: (8 inline comments)
....................................................
File tests/configNetworkTests.py
Line 52: confNetError = configNetwork.ConfigNetworkError
The (I guess not very well founded) reason for this variable was to have a short name for
the with statement in line 59, as I normally deem with statements of not very good
readability when the span across two lines.
Anyway, deleted the variable now and put the with statement in two lines (wrapped at
80chars).
Line 58: for valueTuple in values:
Name changed to vlanIds.
Message checking removed.
Line 71: values = ['badValue', ' bond14', 'bond14 ',
'bond14a', 'bond0 0']
Done
Line 74: msg = '%r is not a valid bonding device name' % value
Done
Line 87: addresses = ['10.18.1.254', '10.50.25.177',
'250.0.0.1',
Done
Line 119: def _fakeBondingOptsPath(self, path):
Done
Line 120: opt = path[path.rindex('/') + 1:]
Totally right :P Done!
Line 121: p = subprocess.Popen(['modinfo', 'bonding'],
Okay, I'll make now the unrelated patch to vdsm proper adding the memoized function
and, when that is accepted, I will resume this patch and make it use the new vdsm proper
method.
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
7:37 a.m.
New subject: Change in vdsm[master]: Added tests for _addNetworkValidation and helpers.
Dan Kenigsberg has posted comments on this change.
Change subject: Added tests for _addNetworkValidation and helpers.
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(8 inline comments)
Thanks; this time there are more interesting comments I hope.
....................................................
File tests/configNetworkTests.py
Line 52: confNetError = configNetwork.ConfigNetworkError
I do not see any benefit of defining a variable that is to be used only once within this
function.
btw, since this variable is a class, it should begin with an uppercase letter.
Line 58: for valueTuple in values:
having
for (vlan, msg) in values:
is prettier. Finding a clearer name for "values" would be cool.
However, in my opinion, the exact string of the message is not part of the API. We free to
change it, and it should not be part of the test.
Line 71: values = ['badValue', ' bond14', 'bond14 ',
'bond14a', 'bond0 0']
bondNames, maybe?
Line 74: msg = '%r is not a valid bonding device name' % value
again, no need to test the message text.
Line 87: addresses = ['10.18.1.254', '10.50.25.177',
'250.0.0.1',
since these list never change, it is better to use tuples.
Line 119: def _fakeBondingOptsPath(self, path):
I think _bondingOptExists is a better name.
Line 120: opt = path[path.rindex('/') + 1:]
this is os.path.basename, I believe.
Line 121: p = subprocess.Popen(['modinfo', 'bonding'],
It would be better to run modinfo only once, and memoize the result, over running it every
time this function is called.
and I wouldn't mind having such a function in vdsm proper (in an unrelated patch of
course).
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
1:45 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Added unittests for _addNetworkValidation, vlanId, validateBondingName and
validateBondingOptions.
......................................................................
Patch Set 1: I would prefer that you didn't submit this
oh, please do not take the automatic text accompanied to -1 personally. I do want this
patch, with few modifications probably.
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
1:44 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Added unittests for _addNetworkValidation, vlanId, validateBondingName and
validateBondingOptions.
......................................................................
Patch Set 1: (4 inline comments)
Thanks for the impressive work. I gave it only a partial review due to personal time
constraints.
....................................................
File tests/configNetworkTests.py
Line 28: import netinfo as netinfoModule
why do you import this module twice
Line 56: 'vlan id out of range: %r, must be 0..%s' % (tooBig,
tooBig-1))]
pep8 is crying, keep lines shorter
Line 112: opt = path[path.rindex('/')+1:]
please `make rpm` before submitting code, to catch few style issues.
Line 187: netinfo = netinfoModule.NetInfo(_netinfo)
this overrides the netinfo module imported above.
--
To view, visit http://gerrit.ovirt.org/6198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd8bb649685fac00640e894a4484cdb0cc0b2f1c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
4318
days inactive
4343
days old
vdsm-patches@lists.fedorahosted.org
15 comments
4 participants
participants (4)
-
Alon Bar-Lev -
asegurap＠redhat.com -
Dan Kenigsberg -
Igor Lvovsky