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 <fromani(a)redhat.com&gt; 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): -- To view, visit https://gerrit.ovirt.org/47132 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I53ba5fbc2f43fbafe9f6f291fcf2402986a5948c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com&gt; Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;