Change in vdsm[master]: configurator doesn't load pyc files under configurators folder

nsoffer at redhat.com nsoffer at redhat.com
Thu Sep 10 12:04:05 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: configurator doesn't load pyc files under configurators folder
......................................................................


Patch Set 7:

(4 comments)

https://gerrit.ovirt.org/#/c/45846/7/tests/toolTests.py
File tests/toolTests.py:

Line 80: 
Line 81: class PatchConfiguratorsTests(VdsmTestCase):
Line 82: 
Line 83:     def tempModuleFolder(self, files):
Line 84:         path = tempfile.mkdtemp()
path should be an argument, so you create and delete the directory in the same place.

Creating a temporary in one place and deleting in another place is a bad pattern.
Line 85:         for f in files:
Line 86:             utils.touchFile('%s/%s' % (path, f))
Line 87:         return path
Line 88: 


Line 82: 
Line 83:     def tempModuleFolder(self, files):
Line 84:         path = tempfile.mkdtemp()
Line 85:         for f in files:
Line 86:             utils.touchFile('%s/%s' % (path, f))
Use os.path.join
Line 87:         return path
Line 88: 
Line 89:     def listModuleFiles(self, files, expected_modules):
Line 90:         path = self.tempModuleFolder(files)


Line 89:     def listModuleFiles(self, files, expected_modules):
Line 90:         path = self.tempModuleFolder(files)
Line 91:         expected = set(expected_modules)
Line 92:         result = configurator._listmodules(path)
Line 93:         utils.rmTree(path)
Please use testlib.temporaryDir:

    with testlib.temporaryDir() as tmp:
        create files
Line 94:         self.assertEquals(
Line 95:             result,
Line 96:             expected,
Line 97:         )


Line 96:             expected,
Line 97:         )
Line 98: 
Line 99:     def testOnlyPyModules(self):
Line 100:         self.listModuleFiles(('a.py', 'b.py'), ('a', 'b'))
This is nice, but we better use one test with permuttions:

    @permutaions([
        # files, modules
        (("a.py", "b.py"), ("a", "b")),
        ...
    ])
    def test_list_modules(self, files, modules):
        with temp dir:
            create files...
            list modules...
            assert...
Line 101: 
Line 102:     def testAdditionalPyPerfixModule(self):
Line 103:         self.listModuleFiles(('a.py', 'b.py', 'a.pyc', 'a.pyioas'), ('a', 'b'))
Line 104: 


-- 
To view, visit https://gerrit.ovirt.org/45846
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia529de0069e2f4ec168a4b9df82ba62c56d66730
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list