Improve code coverage of _install methods

COT/helpers/brew.py

@@ -38,8 +38,8 @@ def install_package(self, package):
         """Install the requested package if needed.
 
         Args:
-          package (str,list): Name of the package to install or list of
-            parameters needed to install the package.
+          package (str): Name of the package to install, or a list of
+            parameters used to install the package.
         """
         # Brew automatically updates when called so no need for us to do it.
         if isinstance(package, list):

COT/helpers/fatdisk.py

@@ -55,15 +55,14 @@ def installable(self):
 
     def _install(self):
         """Install ``fatdisk``."""
-        if helpers['port']:
-            helpers['port'].install_package('fatdisk')
+        try:
+            super(FatDisk, self)._install()
             return
-        elif helpers['brew']:
-            helpers['brew'].install_package('glennmatthews/fatdisk/fatdisk',
-                                            opts=['--devel'])
-            return
-        elif platform.system() != 'Linux':
-            raise self.unsure_how_to_install()
+        except NotImplementedError:
+            # We have an alternative install method available for Linux,
+            # below - but if not Linux, you're out of luck!
+            if platform.system() != 'Linux':
+                raise
 
         # Fatdisk installation requires make
         helpers['make'].install()

COT/helpers/helper.py

@@ -180,6 +180,7 @@ class Helper(object):
 
       call
       install
+      _install
       unsure_how_to_install
     """
 
@@ -370,13 +371,18 @@ def unsure_how_to_install(self):
             return NotImplementedError(msg)
 
     def _install(self):
-        """Subclass-specific implementation of installation logic."""
+        """Subclass-specific implementation of installation logic.
+
+        This method should only be called from :meth:`install`,
+        which does the appropriate pre-validation against the
+        :attr:`installed` and :attr:`installable` properties before
+        calling into this method if appropriate.
+        """
         # Default implementation
         for pm_name, package in self._provider_package.items():
             if helpers[pm_name]:
                 helpers[pm_name].install_package(package)
                 return
-        # We shouldn't get here under normal call flow and logic.
         raise self.unsure_how_to_install()
 
     @staticmethod
@@ -491,8 +497,8 @@ def install_package(self, package):
         """Install the requested package if needed.
 
         Args:
-          package (str,list): Name of the package to install or list of
-            parameters needed to install the package.
+          package (str): Name of the package to install, or a list of
+            parameters used to install the package.
         """
         raise NotImplementedError("install_package not implemented!")
 

COT/helpers/tests/test_fatdisk.py

@@ -203,4 +203,4 @@ def test_install_linux_need_compiler_no_package_manager(self,
     def test_install_helper_mac_no_package_manager(self, *_):
         """Mac installation requires port."""
         self.select_package_manager(None)
-        self.assertRaises(NotImplementedError, self.helper.install)
+        self.assertRaises(RuntimeError, self.helper.install)

COT/helpers/tests/test_helper.py

@@ -215,13 +215,25 @@ def tearDown(self):
 
     @mock.patch('distutils.spawn.find_executable', return_value=None)
     @mock.patch('platform.system', return_value='Windows')
-    def test_install_unsupported(self, *_):
-        """Unable to install without a package manager."""
+    def test_install_windows_unsupported(self, *_):
+        """No support for installation on Windows.
+
+        This is a somewhat artificial test of logic in ``_install``
+        that is normally unreachable when calling ``install()``.
+        """
+        if self.helper is None:
+            return
         self.select_package_manager(None)
-        if self.helper:
-            with mock.patch.object(self.helper, '_path', new=None):
-                self.assertRaises(NotImplementedError,
-                                  self.helper.install)
+        self.assertRaises(NotImplementedError, self.helper._install)
+
+    @mock.patch('distutils.spawn.find_executable', return_value=None)
+    @mock.patch('platform.system', return_value='Linux')
+    def test_install_linux_no_package_manager(self, *_):
+        """Unable to install on Linux without a package manager."""
+        if self.helper is None:
+            return
+        self.select_package_manager(None)
+        self.assertRaises(RuntimeError, self.helper._install)
 
 
 class HelperGenericTest(HelperUT):

COT/helpers/tests/test_vmdktool.py

@@ -174,4 +174,4 @@ def test_install_linux_need_compiler_no_package_manager(self,
     def test_install_helper_mac_no_package_manager(self, *_):
         """Mac installation requires port."""
         self.select_package_manager(None)
-        self.assertRaises(NotImplementedError, self.helper.install)
+        self.assertRaises(RuntimeError, self.helper.install)

COT/helpers/vmdktool.py

@@ -58,14 +58,14 @@ def installable(self):
 
     def _install(self):
         """Install ``vmdktool``."""
-        if helpers['port']:
-            helpers['port'].install_package('vmdktool')
+        try:
+            super(VMDKTool, self)._install()
             return
-        elif helpers['brew']:
-            helpers['brew'].install_package('vmdktool')
-            return
-        elif platform.system() != 'Linux':
-            raise self.unsure_how_to_install()
+        except NotImplementedError:
+            # We have an alternative install method available for Linux,
+            # below - but if not Linux, you're out of luck!
+            if platform.system() != 'Linux':
+                raise
 
         # We don't have vmdktool in apt or yum yet,
         # but we can build it manually:

docs/COT.helpers.helper.rst

@@ -2,5 +2,6 @@
 =============================
 
 .. automodule:: COT.helpers.helper
+  :private-members: _install
   :special-members: __init__
   :exclude-members: TemporaryDirectory