Alon Bar-Lev has uploaded a new change for review.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
configNetworkTests: use context to manage monkey patches
Easier to manage setup and cleanup at on place, then trying to cleanup after a block of code, and catching all exceptions.
Example of use: --- def f(): @contextlib.contextmanager def context(): oldExists = os.path.exists os.path.exists = self._bondingOptExists try: yield {} finally: os.path.exists = oldExists
with context() as value: logic ---
Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Signed-off-by: Alon Bar-Lev alonbl@redhat.com --- M tests/configNetworkTests.py 1 file changed, 115 insertions(+), 98 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/39/6939/1 -- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(3 inline comments)
Thanks for this important cleanup!
May I be rude to ask you to make it even cooler? It would be great to drop the repeated definition the context() function.
.................................................... File tests/configNetworkTests.py Line 161: def context(): eval() is generally evil, but I think it would be cool if we harness its dark powers for a more general solution - an context manager that receives a map like
{ 'os.path.exists': self._bondingOptExists}
perform the monkey-patching on __enter__, and clean them up on __exit__.
We could then use it as a decorator to the test function.
Line 169: with context() as value: do we need the "as value"?
Line 236: self._addNetworkWithExc((netinfoIns, 'fakebrnet', vlan, bonding, nics, these indentations would make pep8 and me cry.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 1:
I will do that.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 2:
Tell me what you think of this one, I avoided the eval to keep us sane.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(7 inline comments)
Thanks! I think we can make it even cooler.
.................................................... File tests/configNetworkTests.py Line 157: Just an idea: it would be even cooler if you write a function decorator that does the monkey-patching:
@MonkeyPatch(os.path, 'exists', self._bondingOptExists) @MonkeyPatch(some, other, patch) def testCase():
Line 163: ]): I find this indentation awkward.
Line 314: # a rather ugly stubbing it is quite beautiful now! I think we can drop the comment.
Line 338: (netinfo, 'NET_CONF_BACK_DIR', os.path.join(self._tempdir, 'netback')), long line
.................................................... File tests/monkeypatch.py Line 40: for f in what: a more pythonic way to write this is
for module, name, func in what:
though I prefer (module, attr, value)
Line 43: func=f[2] this value is not necessarily a func.
Line 47: yield {} what's the point of this empty dict?
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 3: (7 inline comments)
.................................................... File tests/configNetworkTests.py Line 157: Never done this, but I will try.
Line 163: ]): Why? It allows easy patching of new entries.
Line 314: # a rather ugly stubbing Done
Line 338: (netinfo, 'NET_CONF_BACK_DIR', os.path.join(self._tempdir, 'netback')), Done
.................................................... File tests/monkeypatch.py Line 40: for f in what: Done
Line 43: func=f[2] Done
Line 47: yield {} This is by documentation of this package... I can drop it if you like.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 4:
Added the option of decoration, however it can be used only for non members (static) variables, as it needs the missing "self" available.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 5:
1. Need to get used to autotools build system for python :), we really need make distcheck to work, I will invest time in this again.
2. Some pep fixes, also need to perform a complete pass, so my newer pep will be happy.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 5: I would prefer that you didn't submit this
(2 inline comments)
thanks, that's a cool decorator! (but I think it is slightly buggy)
.................................................... File tests/monkeypatch.py Line 38: def decorator(f): I think you should use
@functools.wraps
so that the wrapping function has the same name as the wrapped one.
Line 42: f(*args, **kw) do we really want to swallow the returned value of f()?
I'd return it. But worse: you do not protect here from exceptions.
Cannot you use the contextmanager that you've defined below?
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 5: (2 inline comments)
.................................................... File tests/monkeypatch.py Line 34: # logic Line 35: # --- Line 36: # Line 37: def MonkeyPatch(module, name, that): Line 38: def decorator(f): Done Line 39: def test(*args, **kw): Line 40: old = getattr(module, name) Line 41: setattr(module, name, that) Line 42: f(*args, **kw)
Line 38: def decorator(f): Line 39: def test(*args, **kw): Line 40: old = getattr(module, name) Line 41: setattr(module, name, that) Line 42: f(*args, **kw) I thought to minimize overhead...
Done Line 43: setattr(module, name, old) Line 44: return test Line 45: return decorator Line 46:
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 6:
Build Failed
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/444/ : FAILURE
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 6:
Jenkinds build fail unrelated to this. ERROR: testStressTest (resourceManagerTests.ResourceManagerTests) ---------------------------------------------------------------------- Traceback (most recent call last):
File "/home/jenkins/slave/workspace/patch_vdsm_unit_tests/tests/resourceManagerTests.py", line 662, in tearDown manager.unregisterNamespace("string") File "/home/jenkins/slave/workspace/patch_vdsm_unit_tests/vdsm/storage/resourceManager.py", line 392, in unregisterNamespace raise ResourceManagerError("Cannot unregister Resource Factory '%s'. It has active resources." % (namespace)) ResourceManagerError: Cannot unregister Resource Factory 'string'. It has active resources.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Alon Bar-Lev has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 6: Verified
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Gal Hammer has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 6: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
Patch Set 6: Looks good to me, approved
thanks a lot, Alon.
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has submitted this change and it was merged.
Change subject: configNetworkTests: use context to manage monkey patches ......................................................................
configNetworkTests: use context to manage monkey patches
Easier to manage setup and cleanup at on place, then trying to cleanup after a block of code, and catching all exceptions.
Example of use: --- from monkeypatch import MonkeyPatch
@MonkeyPatch(subprocess, 'Popen', lambda x: None) @MonkeyPatch(os, 'chown', lambda *x: 0) def test(): logic
def test(): with MonkeyPatchScope([ (subprocess, 'Popen', lambda x: None), (os, 'chown', lambda *x: 0) ]): logic ---
Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Signed-off-by: Alon Bar-Lev alonbl@redhat.com --- M tests/Makefile.am M tests/configNetworkTests.py A tests/monkeypatch.py 3 files changed, 111 insertions(+), 49 deletions(-)
Approvals: Alon Bar-Lev: Verified Gal Hammer: Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/6939 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I2ecb4589f2edbf605e62a8963889d314c2535537 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Alon Bar-Lev alonbl@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org