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