Nir Soffer has posted comments on this change. Change subject: configurator doesn't load pyc files under configurators folder ......................................................................
Patch Set 5: (3 comments) https://gerrit.ovirt.org/#/c/45846/5/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 28: """ Line 29: Line 30: Line 31: from collections import deque Line 32: import re Sorting? Line 33: import argparse Line 34: import os Line 35: import sys Line 36: import traceback Line 57: return set( Line 58: os.path.splitext(name)[0] Line 59: for name in os.listdir(path) Line 60: if is_module(name) Line 61: ) Looks good Line 62: Line 63: Line 64: _CONFIGURATORS = {} Line 65: for module in _listmodules(configurators): https://gerrit.ovirt.org/#/c/45846/5/tests/toolTests.py File tests/toolTests.py: Line 89: Line 90: self.assertEquals( Line 91: configurator._listmodules(configurators), Line 92: expected Line 93: ) This does not test the inclusion of pyc files on node, and also will break when we add the next module. I think the best way to test this is to create a temporary directory with some files e.g. ("a.py", "a.pyc, "_underscore.py", "_underscore.pyc"), and test that we get the correct set (e.g. set(["a"])) I would add 4 tests: - including py files - including pyc files - removing duplicates (py, pyc) - excluding private modules Line 94: Line 95: def testPatch(self): Line 96: self.configurator = MockModuleConfigurator('a') Line 97: self.function_was_run = False -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
