Change in vdsm[ovirt-3.6]: tests: fixed mangled names in crossImportTests

Hello Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/47132

to review the following change.

Change subject: tests: fixed mangled names in crossImportTests
......................................................................

tests: fixed mangled names in crossImportTests

crossImportTests needs to extract the module name given
a python file. To do this, it uses a naive approach stripping the
extension of a filename, being the extension either ".py" or ".pyc".
This approach seems good enough for the test purposes, but it was
done incorrectly using the strip() method of string objects,
like

filename.strip(".pyc")

Unfortunately, as per strip() documentation:

strip(...)
    S.strip([chars]) -> string or unicode

[...]
    If chars is given and not None, remove characters in chars instead.
    [...]

This means that *any* occurrence of *any* char provided will be removed,
not just the substring as a whole. This leads to incorrect output.
For example

"cmdutils.pyc".strip(".pyc") -> "mdutils"

This patch fixes that using the more suitable os.path.splitext()
function. We take the chance to add an unit test for the get_mods()
helper, to make sure this won't happen again (or at least, not so
easily).

Change-Id: I53ba5fbc2f43fbafe9f6f291fcf2402986a5948c
Signed-off-by: Francesco Romani 
Bug-Url: https://bugzilla.redhat.com/1268229
---
M tests/crossImportsTests.py.in
1 file changed, 30 insertions(+), 4 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/32/47132/1

diff --git a/tests/crossImportsTests.py.in b/tests/crossImportsTests.py.in
index a3e1d81..f342ef1 100644
--- a/tests/crossImportsTests.py.in
+++ b/tests/crossImportsTests.py.in
@@ -17,12 +17,13 @@
 # Refer to the README and COPYING files for full details of the license
 #

+import collections
 import os
 import sys
 from distutils.sysconfig import get_python_lib

from testlib import VdsmTestCase as TestCaseBase
-from monkeypatch import MonkeyPatch
+from monkeypatch import MonkeyPatch, MonkeyPatchScope

def path_without_vdsm_ext_mod():
@@ -35,9 +36,34 @@

def get_mods(path):
-    return [(file.strip('.py')).strip('.pyc') for file in
-            os.listdir(os.path.abspath(path))
-            if (file.endswith(".py") or file.endswith(".pyc"))]
+    return [os.path.splitext(entry)[0]
+            for entry in os.listdir(os.path.abspath(path))
+            if (entry.endswith(".py") or entry.endswith(".pyc"))]
+
+
+Mod = collections.namedtuple("Mod", ["filename", "modulename"])
+
+
+class CrossImportsHelpersTests(TestCaseBase):
+
+    def test_get_mods(self):
+        mods = (
+            # some random samples. Some names must include 'p', 'y', 'c'
+            Mod("response.py", "response"),
+            Mod("cmdutils.py", "cmdutils"),
+            Mod("virtsparsify.py", "virtsparsify"),
+            Mod("pthread.py", "pthread"),
+            Mod("constants.py", "constants"),
+            Mod("compat.py", "compat"),
+            Mod("udevadm.py", "udevadm")
+        )
+
+        def _listdir(unused):
+            return [mod.filename for mod in mods]
+
+        with MonkeyPatchScope([(os, "listdir", _listdir)]):
+            self.assertEqual(get_mods("/bogus/unused/path"),
+                             [mod.modulename for mod in mods])

class CrossImportsTestCaseShould(TestCaseBase):

[e-mail] has posted comments on this change.

Change subject: tests: fixed mangled names in crossImportTests
......................................................................

Patch Set 1:

* Update tracker::#1268229::OK
* Check Bug-Url::OK
* Check Public Bug::#1268229::OK, public bug
* Check Product::#1268229::OK, Correct classification oVirt
* Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Petr Horáček has posted comments on this change.

Change subject: tests: fixed mangled names in crossImportTests
......................................................................

Patch Set 1: Verified+1

Test passed inside a mock runner.

Ido Barkan has posted comments on this change.

Change subject: tests: fixed mangled names in crossImportTests
......................................................................

Patch Set 1: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: fixed mangled names in crossImportTests
......................................................................

Patch Set 1: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: tests: fixed mangled names in crossImportTests
......................................................................

tests: fixed mangled names in crossImportTests

crossImportTests needs to extract the module name given
a python file. To do this, it uses a naive approach stripping the
extension of a filename, being the extension either ".py" or ".pyc".
This approach seems good enough for the test purposes, but it was
done incorrectly using the strip() method of string objects,
like

filename.strip(".pyc")

Unfortunately, as per strip() documentation:

strip(...)
    S.strip([chars]) -> string or unicode

[...]
    If chars is given and not None, remove characters in chars instead.
    [...]

This means that *any* occurrence of *any* char provided will be removed,
not just the substring as a whole. This leads to incorrect output.
For example

"cmdutils.pyc".strip(".pyc") -> "mdutils"

This patch fixes that using the more suitable os.path.splitext()
function. We take the chance to add an unit test for the get_mods()
helper, to make sure this won't happen again (or at least, not so
easily).

Change-Id: I53ba5fbc2f43fbafe9f6f291fcf2402986a5948c
Signed-off-by: Francesco Romani 
Bug-Url: https://bugzilla.redhat.com/1268229
Reviewed-on: https://gerrit.ovirt.org/47132
Tested-by: Petr Horáček 
Continuous-Integration: Jenkins CI
Reviewed-by: Ido Barkan 
Reviewed-by: Dan Kenigsberg 
---
M tests/crossImportsTests.py.in
1 file changed, 30 insertions(+), 4 deletions(-)

Approvals:
  Ido Barkan: Looks good to me, but someone else must approve
  Jenkins CI: Passed CI tests
  Petr Horáček: Verified
  Dan Kenigsberg: Looks good to me, approved

[e-mail] has posted comments on this change.

Change subject: tests: fixed mangled names in crossImportTests
......................................................................

Patch Set 2:

* Update tracker::#1268229::OK
* Set MODIFIED::bug 1268229::::#1268229::::IGNORE, not oVirt prod but vdsm