Lint cleanup and testing

Cleanup remaining lint errors. Enable lint testing on travis ci. Add more unit
tests. update clean makerule to remove everything but virtual envs.

Testing:
  coverage - 82%
  python2/3 - unit tests - pass
  python2/3 - manual checkout status - ok

.travis.yml

@@ -25,7 +25,7 @@ before_script:
   - git --version
 script:
   - cd test; make test
-#  - cd test; make lint
+  - cd test; make lint
 after_success:
   - cd test; make coverage
   - cd test; coveralls

manic/__init__.py

@@ -5,5 +5,5 @@
 from manic.utils import printlog
 
 __all__ = [
-    checkout, printlog,
+    'checkout', 'printlog',
 ]

manic/checkout.py

@@ -16,9 +16,9 @@
 import os.path
 import sys
 import textwrap
-import traceback
 
-from manic.externals_description import read_externals_description_file, create_externals_description
+from manic.externals_description import create_externals_description
+from manic.externals_description import read_externals_description_file
 from manic.externals_status import check_safe_to_update_repos
 from manic.sourcetree import SourceTree
 from manic.utils import printlog
@@ -295,4 +295,3 @@ def main(args):
 
     logging.info('checkout_externals completed without exceptions.')
     return 0
-

manic/externals_description.py

@@ -135,9 +135,15 @@ def get_cfg_schema_version(model_cfg):
     version_list = re.split(r'[-+]', semver_str)
     version_str = version_list[0]
     version = version_str.split('.')
-    major = int(version[0].strip())
-    minor = int(version[1].strip())
-    patch = int(version[2].strip())
+    try:
+        major = int(version[0].strip())
+        minor = int(version[1].strip())
+        patch = int(version[2].strip())
+    except ValueError:
+        msg = ('Config file schema version must have integer digits for '
+               'major, minor and patch versions. '
+               'Received "{0}"'.format(version_str))
+        fatal_error(msg)
     return major, minor, patch
 
 

manic/repository_git.py

@@ -8,8 +8,6 @@
 import copy
 import os
 import re
-import string
-import sys
 
 from .global_constants import EMPTY_STR
 from .repository import Repository
@@ -26,11 +24,11 @@ class GitRepository(Repository):
 
     * be isolated in separate functions with no application logic
       * of the form:
-         - cmd = []
+         - cmd = ['git', ...]
          - value = check_output(cmd)
          - return value
       * be static methods (not rely on self)
-      * name as git_subcommand_args(user_args)
+      * name as _git_subcommand_args(user_args)
 
     This convention allows easy unit testing of the repository logic
     by mocking the specific calls to return predefined results.
@@ -232,9 +230,9 @@ def _create_remote_name(self):
         # repo name should nominally already be something that git can
         # deal with. We need to remove other possibly troublesome
         # punctuation, e.g. /, $, from the base name.
-        unsafe = '!@#$%^&*()[]{}\\/,;~'
-        for c in unsafe:
-            base_name = base_name.replace(c, '')
+        unsafe_characters = '!@#$%^&*()[]{}\\/,;~'
+        for unsafe in unsafe_characters:
+            base_name = base_name.replace(unsafe, '')
         remote_name = "{0}_{1}".format(base_name, repo_name)
         return remote_name
 

manic/sourcetree.py

@@ -22,6 +22,8 @@ class _Source(object):
     _Source represents a <source> object in a <config_sourcetree>
     """
 
+    # pylint: disable=R0902
+
     def __init__(self, root_dir, name, source):
         """Parse an XML node for a <source> tag
 

test/Makefile

@@ -27,7 +27,7 @@ COVERAGE_ARGS=--rcfile=.coveragerc --append
 
 # source files
 SRC = \
-	../checkout_externals.py \
+	../checkout_externals \
 	../manic/*.py
 
 CHECKOUT_EXE = ../checkout_externals
@@ -91,11 +91,11 @@ env : FORCE
 #
 .PHONY : clean
 clean : FORCE
-	-rm -rf *~ *.pyc
+	-rm -rf *~ *.pyc tmp fake htmlcov
 
 .PHONY : clobber
 clobber : clean
-	-rm -rf env_* htmlcov tmp fake
+	-rm -rf env_*
 
 FORCE : 
 

test/test_sys_checkout.py

@@ -180,6 +180,7 @@ def create_section(self, repo_type, name, tag='', branch='',
         optional items.
 
         """
+        # pylint: disable=R0913
         self._config.add_section(name)
         self._config.set(name, ExternalsDescription.PATH,
                          os.path.join(EXTERNALS_NAME, name))
@@ -205,6 +206,7 @@ def update_branch(self, dest_dir, name, branch, repo_type=None,
                       filename=CFG_NAME):
         """Update a repository branch, and potentially the remote.
         """
+        # pylint: disable=R0913
         self._config.set(name, ExternalsDescription.BRANCH, branch)
 
         if repo_type:
@@ -214,7 +216,7 @@ def update_branch(self, dest_dir, name, branch, repo_type=None,
         try:
             # remove the tag if it existed
             self._config.remove_option(name, ExternalsDescription.TAG)
-        except:
+        except BaseException:
             pass
 
         self._write_config(dest_dir, filename)
@@ -223,6 +225,7 @@ def update_tag(self, dest_dir, name, tag, repo_type=None,
                    filename=CFG_NAME):
         """Update a repository tag, and potentially the remote
         """
+        # pylint: disable=R0913
         self._config.set(name, ExternalsDescription.TAG, tag)
 
         if repo_type:
@@ -232,7 +235,7 @@ def update_tag(self, dest_dir, name, tag, repo_type=None,
         try:
             # remove the branch if it existed
             self._config.remove_option(name, ExternalsDescription.BRANCH)
-        except:
+        except BaseException:
             pass
 
         self._write_config(dest_dir, filename)

test/test_unit_externals_description.py

@@ -13,6 +13,7 @@
 
 import os
 import os.path
+import shutil
 import unittest
 
 try:
@@ -36,8 +37,11 @@ def config_string_cleaner(text):
 
 from manic.externals_description import DESCRIPTION_SECTION, VERSION_ITEM
 from manic.externals_description import ExternalsDescription
+from manic.externals_description import ExternalsDescriptionDict
 from manic.externals_description import ExternalsDescriptionConfigV1
 from manic.externals_description import get_cfg_schema_version
+from manic.externals_description import read_externals_description_file
+from manic.externals_description import create_externals_description
 
 from manic.global_constants import EMPTY_STR
 
@@ -92,12 +96,23 @@ def test_schema_version_missing(self):
         with self.assertRaises(RuntimeError):
             get_cfg_schema_version(self._config)
 
+    def test_schema_version_not_int(self):
+        """Test that a externals description file a version that doesn't
+        decompose to integer major, minor and patch versions raises
+        runtime error.
+
+        """
+        self._config.set(DESCRIPTION_SECTION, VERSION_ITEM, 'unknown')
+        with self.assertRaises(RuntimeError):
+            get_cfg_schema_version(self._config)
+
 
 class TestModelDescritionConfigV1(unittest.TestCase):
     """Test that parsing config/ini fileproduces a correct dictionary
     for the externals description.
 
     """
+    # pylint: disable=R0902
 
     def setUp(self):
         """Boiler plate construction of string containing xml for multiple components.
@@ -200,5 +215,129 @@ def test_two_sources(self):
         self._check_comp2(model)
 
 
+class TestReadExternalsDescription(unittest.TestCase):
+    """Test the application logic of read_externals_description_file
+    """
+    TMP_FAKE_DIR = 'fake'
+
+    def setUp(self):
+        """Setup directory for tests
+        """
+        if not os.path.exists(self.TMP_FAKE_DIR):
+            os.makedirs(self.TMP_FAKE_DIR)
+
+    def tearDown(self):
+        """Cleanup tmp stuff on the file system
+        """
+        if os.path.exists(self.TMP_FAKE_DIR):
+            shutil.rmtree(self.TMP_FAKE_DIR)
+
+    def test_no_file_error(self):
+        """Test that a runtime error is raised when the file does not exist
+
+        """
+        root_dir = os.getcwd()
+        filename = 'this-file-should-not-exist'
+        with self.assertRaises(RuntimeError):
+            read_externals_description_file(root_dir, filename)
+
+    def test_no_dir_error(self):
+        """Test that a runtime error is raised when the file does not exist
+
+        """
+        root_dir = '/path/to/some/repo'
+        filename = 'externals.cfg'
+        with self.assertRaises(RuntimeError):
+            read_externals_description_file(root_dir, filename)
+
+    def test_no_invalid_error(self):
+        """Test that a runtime error is raised when the file format is invalid
+
+        """
+        root_dir = os.getcwd()
+        filename = 'externals.cfg'
+        file_path = os.path.join(root_dir, filename)
+        file_path = os.path.abspath(file_path)
+        contents = """
+<source_tree version='1.0.0'>
+invalid file format
+</sourc_tree>"""
+        with open(file_path, 'w') as fhandle:
+            fhandle.write(contents)
+        with self.assertRaises(RuntimeError):
+            read_externals_description_file(root_dir, filename)
+        os.remove(file_path)
+
+
+class TestCreateExternalsDescription(unittest.TestCase):
+    """Test the application logic of creat_externals_description
+    """
+
+    def setUp(self):
+        """Create config object used as basis for all tests
+        """
+        self._config = config_parser()
+        self.setup_config()
+
+    def setup_config(self):
+        """Boiler plate construction of xml string for componet 1
+        """
+        name = 'test'
+        self._config.add_section(name)
+        self._config.set(name, ExternalsDescription.PATH, 'externals')
+        self._config.set(name, ExternalsDescription.PROTOCOL, 'git')
+        self._config.set(name, ExternalsDescription.REPO_URL, '/path/to/repo')
+        self._config.set(name, ExternalsDescription.TAG, 'test_tag')
+        self._config.set(name, ExternalsDescription.REQUIRED, 'True')
+
+        self._config.add_section(DESCRIPTION_SECTION)
+        self._config.set(DESCRIPTION_SECTION, VERSION_ITEM, '1.0.0')
+
+    def test_cfg_v1(self):
+        """Test that a correct cfg v1 object is created by create_externals_description
+
+        """
+        self._config.set(DESCRIPTION_SECTION, VERSION_ITEM, '1.2.3')
+        ext = create_externals_description(self._config, model_format='cfg')
+        self.assertIsInstance(ext, ExternalsDescriptionConfigV1)
+
+    def test_dict(self):
+        """Test that a correct cfg v1 object is created by create_externals_description
+
+        """
+        rdata = {ExternalsDescription.PROTOCOL: 'git',
+                 ExternalsDescription.REPO_URL: '/path/to/repo',
+                 ExternalsDescription.TAG: 'tagv1',
+                 ExternalsDescription.BRANCH: EMPTY_STR, }
+
+        desc = {
+            'test': {
+                ExternalsDescription.REQUIRED: False,
+                ExternalsDescription.PATH: '../fake',
+                ExternalsDescription.EXTERNALS: EMPTY_STR,
+                ExternalsDescription.REPO: rdata, },
+        }
+
+        ext = create_externals_description(desc, model_format='dict')
+        self.assertIsInstance(ext, ExternalsDescriptionDict)
+
+    def test_cfg_unknown_version(self):
+        """Test that a runtime error is raised when an unknown file version is
+        received
+
+        """
+        self._config.set(DESCRIPTION_SECTION, VERSION_ITEM, '123.456.789')
+        with self.assertRaises(RuntimeError):
+            create_externals_description(self._config, model_format='cfg')
+
+    def test_cfg_unknown_format(self):
+        """Test that a runtime error is raised when an unknown format string is
+        received
+
+        """
+        with self.assertRaises(RuntimeError):
+            create_externals_description(self._config, model_format='unknown')
+
+
 if __name__ == '__main__':
     unittest.main()

test/test_unit_repository_git.py

@@ -131,7 +131,7 @@ def setUp(self):
                 {
                     ExternalsDescription.REQUIRED: False,
                     ExternalsDescription.PATH: self.TMP_FAKE_DIR,
-                    ExternalsDescription.EXTERNALS: '',
+                    ExternalsDescription.EXTERNALS: EMPTY_STR,
                     ExternalsDescription.REPO: rdata,
                 },
                 }
@@ -274,7 +274,7 @@ class TestGitCreateRemoteName(unittest.TestCase):
     """
 
     def setUp(self):
-        """
+        """Common infrastructure for testing _create_remote_name
         """
         self._rdata = {ExternalsDescription.PROTOCOL: 'git',
                        ExternalsDescription.REPO_URL:

test/test_unit_utils.py

@@ -14,8 +14,40 @@
 import os
 import unittest
 
-from manic.utils import str_to_bool, is_remote_url, split_remote_url
-from manic.utils import expand_local_url
+from manic.utils import str_to_bool, execute_subprocess
+from manic.utils import is_remote_url, split_remote_url, expand_local_url
+
+
+class TestExecuteSubprocess(unittest.TestCase):
+    """Test the application logic of execute_subprocess wrapper
+    """
+
+    def test_exesub_return_stat_err(self):
+        """Test that execute_subprocess returns a status code when caller
+        requests and the executed subprocess fails.
+
+        """
+        cmd = ['false']
+        status = execute_subprocess(cmd, status_to_caller=True)
+        self.assertEqual(status, 1)
+
+    def test_exesub_return_stat_ok(self):
+        """Test that execute_subprocess returns a status code when caller
+        requests and the executed subprocess succeeds.
+
+        """
+        cmd = ['true']
+        status = execute_subprocess(cmd, status_to_caller=True)
+        self.assertEqual(status, 0)
+
+    def test_exesub_except_stat_err(self):
+        """Test that execute_subprocess raises an exception on error when
+        caller doesn't request return code
+
+        """
+        cmd = ['false']
+        with self.assertRaises(RuntimeError):
+            execute_subprocess(cmd, status_to_caller=False)
 
 
 class TestStrToBool(unittest.TestCase):