Change in vdsm[master]: tests: fix wrong use of assertions

Federico Simoncelli fsimonce at redhat.com
Mon Jun 9 22:13:48 UTC 2014


Federico Simoncelli has posted comments on this change.

Change subject: tests: fix wrong use of assertions
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/28124/4/tests/testrunner.py
File tests/testrunner.py:

Line 392:     raise AssertionError(msg)
Line 393: 
Line 394: 
Line 395: def runCommand(*args):
Line 396:     process = subprocess.Popen(args, stderr=subprocess.PIPE,
> I also think that it is time to improve the code instead of keeping the old
I think that the whole problem here comes from the fact that AsyncProc is misplaced.

I'd like to have a lightweight function to run commands. It's handy in many places (e.g. vdsm-tool, hooks, network tools, etc). In those area you don't need support for: AsyncProc, nice, ionice, setsid, etc.

It seems to me that AsyncProc (when really needed) should be something that wraps the lightweight command runner (instead of being an execCmd argument).
And then we can compose the other options, e.g.:

     cmd = [constants.EXT_DD, ...]
     return AsyncProc(runCommand(
         niceCommand(cmd, utils.IOCLASS.IDLE, utils.NICENESS.HIGH),
         deathSignal=signal.SIGKILL))

For code reuse and more extensive testing I think here we should use the same lightweight command runner.

Bottom line: let's use execCmd here now (because we don't have a lightweight command runner) but we should also work toward improving execCmd and move the AsyncProc outside.

Probably only the storage subsystem needs to wrap/extend the lightweight command runner with AsyncProc, nice, ionice and setsid, and in that case we can have an utility function to include all of those.
Line 397:                                stdout=subprocess.PIPE)
Line 398:     output, error = process.communicate()
Line 399:     if process.returncode != 0:
Line 400:         raise TestRunnerError("subprocess failed: returncode=%s: stderr=%r" %


-- 
To view, visit http://gerrit.ovirt.org/28124
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I693923e6dcaa05bc1479db814c0b7696b3536c9c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe 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