Change in vdsm[master]: util: fix execCmd to accept tuples
danken at redhat.com
danken at redhat.com
Thu Mar 27 18:27:24 UTC 2014
Dan Kenigsberg has posted comments on this change.
Change subject: util: fix execCmd to accept tuples
......................................................................
Patch Set 4: Code-Review-1
(3 comments)
http://gerrit.ovirt.org/#/c/26070/4/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 53: import threading
Line 54: import time
Line 55: import zombiereaper
Line 56:
Line 57: import cpopen
I do not mind this change, but it's unrelated to this patch.
Line 58: from .config import config
Line 59: from . import constants
Line 60:
Line 61: # Buffsize is 1K because I tested it on some use cases and 1K was fastest. If
http://gerrit.ovirt.org/#/c/26070/4/tests/utilsTests.py
File tests/utilsTests.py:
Line 24: import logging
Line 25:
Line 26: from testrunner import VdsmTestCase as TestCaseBase
Line 27: from vdsm import utils
Line 28: from vdsm import constants
It's not completely your fault, but please separate:
* stdlib
* out-of-vdsm
* vdsm-internal
and keep each group sorted.
Line 29: import time
Line 30: from monkeypatch import Patch
Line 31: import cpopen
Line 32:
Line 469:
Line 470: def __init__(self, *args, **kwargs):
Line 471: TestCaseBase.__init__(self, *args, **kwargs)
Line 472:
Line 473: self.patch = Patch([(cpopen, 'CPopen', self.patchedCPopen),
I feel responsible, having suggested monkey-patching...
However I must say that this unit test goes too deep into replicating the implementation of execCmd. It would be much better to have a black-box test, verifying that (echo, hello, world) executes as it should, and that touching a root-owned directory works with sudo and fails without it.
Line 474: (utils, 'AsyncProc', self.patchedAsyncProc)])
Line 475:
Line 476: def setUp(self):
Line 477: self.patch.apply()
--
To view, visit http://gerrit.ovirt.org/26070
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d1269d9a26b1fef1552976ae626cf4596471283
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list