Refactor repository_svn

Refactor repository_svn to match the have structure, conventions and
organization as repository_git. Update unit tests.

Testing:
  python2/3 - unit tests - pass
  python2 - clm checkout, status - ok
  python3 - cesm checkout, status - ok

manic/repository_svn.py

@@ -5,10 +5,8 @@
 from __future__ import unicode_literals
 from __future__ import print_function
 
-import logging
 import os
 import re
-import subprocess
 import xml.etree.ElementTree as ET
 
 from .repository import Repository
@@ -20,6 +18,20 @@
 class SvnRepository(Repository):
     """
     Class to represent and operate on a repository description.
+
+    For testing purpose, all system calls to svn should:
+
+    * be isolated in separate functions with no application logic
+      * of the form:
+         - cmd = ['svn', ...]
+         - value = check_output(cmd)
+         - return value
+      * be static methods (not rely on self)
+      * name as _svn_subcommand_args(user_args)
+
+    This convention allows easy unit testing of the repository logic
+    by mocking the specific calls to return predefined results.
+
     """
     RE_URLLINE = re.compile(r'^URL:')
 
@@ -36,22 +48,11 @@ def __init__(self, component_name, repo):
             msg = "DEV_ERROR in svn repository. Shouldn't be here!"
             fatal_error(msg)
 
-    def status(self, stat, repo_dir_path):
-        """
-        Check and report the status of the repository
-        """
-        self.svn_check_sync(stat, repo_dir_path)
-        if os.path.exists(repo_dir_path):
-            self.svn_status(stat, repo_dir_path)
-        return stat
-
-    def verbose_status(self, repo_dir_path):
-        """Display the raw repo status to the user.
-
-        """
-        if os.path.exists(repo_dir_path):
-            self.svn_status_verbose(repo_dir_path)
-
+    # ----------------------------------------------------------------
+    #
+    # Public API, defined by Repository
+    #
+    # ----------------------------------------------------------------
     def checkout(self, base_dir_path, repo_dir_name):
         """Checkout or update the working copy
 
@@ -64,42 +65,54 @@ def checkout(self, base_dir_path, repo_dir_name):
         """
         repo_dir_path = os.path.join(base_dir_path, repo_dir_name)
         if os.path.exists(repo_dir_path):
-            self._svn_switch(repo_dir_path)
+            cwd = os.getcwd()
+            os.chdir(repo_dir_path)
+            self._svn_switch(self._url)
+            os.chdir(cwd)
         else:
-            self._svn_checkout(repo_dir_path)
+            self._svn_checkout(self._url, repo_dir_path)
 
-    def _svn_checkout(self, repo_dir_path):
+    def status(self, stat, repo_dir_path):
         """
-        Checkout a subversion repository (repo_url) to checkout_dir.
+        Check and report the status of the repository
         """
-        cmd = ['svn', 'checkout', self._url, repo_dir_path]
-        execute_subprocess(cmd)
+        self._check_sync(stat, repo_dir_path)
+        if os.path.exists(repo_dir_path):
+            self._status_summary(stat, repo_dir_path)
+        return stat
+
+    def verbose_status(self, repo_dir_path):
+        """Display the raw repo status to the user.
 
-    def _svn_switch(self, repo_dir_path):
-        """
-        Switch branches for in an svn sandbox
         """
-        cwd = os.getcwd()
-        os.chdir(repo_dir_path)
-        cmd = ['svn', 'switch', self._url]
-        execute_subprocess(cmd)
-        os.chdir(cwd)
+        if os.path.exists(repo_dir_path):
+            self._status_verbose(repo_dir_path)
+
+    # ----------------------------------------------------------------
+    #
+    # Internal work functions
+    #
+    # ----------------------------------------------------------------
+    def _check_sync(self, stat, repo_dir_path):
+        """Check to see if repository directory exists and is at the expected
+        url.  Return: status object
 
-    @staticmethod
-    def svn_info(repo_dir_path):
-        """Return results of svn info command
         """
-        cmd = ['svn', 'info', repo_dir_path]
-        try:
-            output = check_output(cmd)
-            log_process_output(output)
-        except subprocess.CalledProcessError as error:
-            logging.info(error)
-            output = ''
-        return output
+        if not os.path.exists(repo_dir_path):
+            # NOTE(bja, 2017-10) this state should have been handled by
+            # the source object and we never get here!
+            stat.sync_state = ExternalStatus.STATUS_ERROR
+        else:
+            svn_output = self._svn_info(repo_dir_path)
+            if not svn_output:
+                # directory exists, but info returned nothing. .svn
+                # directory removed or incomplete checkout?
+                stat.sync_state = ExternalStatus.UNKNOWN
+            else:
+                stat.sync_state = self._check_url(svn_output, self._url)
 
     @staticmethod
-    def svn_check_url(svn_output, expected_url):
+    def _check_url(svn_output, expected_url):
         """Determine the svn url from svn info output and return whether it
         matches the expected value.
 
@@ -117,32 +130,25 @@ def svn_check_url(svn_output, expected_url):
             status = ExternalStatus.MODEL_MODIFIED
         return status
 
-    def svn_check_sync(self, stat, repo_dir_path):
-        """Check to see if repository directory exists and is at the expected
-        url.  Return: status object
+    def _status_summary(self, stat, repo_dir_path):
+        """Report whether the svn repository is in-sync with the model
+        description and whether the sandbox is clean or dirty.
 
         """
-        if not os.path.exists(repo_dir_path):
-            # NOTE(bja, 2017-10) this state should have been recorded by
-            # the source object and we never get here!
-            stat.sync_state = ExternalStatus.STATUS_ERROR
+        svn_output = self._svn_status_xml(repo_dir_path)
+        is_dirty = self.xml_status_is_dirty(svn_output)
+        if is_dirty:
+            stat.clean_state = ExternalStatus.DIRTY
         else:
-            svn_output = self.svn_info(repo_dir_path)
-            if not svn_output:
-                # directory exists, but info returned nothing. .svn
-                # directory removed or incomplete checkout?
-                stat.sync_state = ExternalStatus.UNKNOWN
-            else:
-                stat.sync_state = self.svn_check_url(svn_output, self._url)
+            stat.clean_state = ExternalStatus.STATUS_OK
+
+    def _status_verbose(self, repo_dir_path):
+        """Display the raw svn status output to the user.
 
-    @staticmethod
-    def _svn_status_xml(repo_dir_path):
-        """
-        Get status of the subversion sandbox in repo_dir
         """
-        cmd = ['svn', 'status', '--xml', repo_dir_path]
-        svn_output = check_output(cmd)
-        return svn_output
+        svn_output = self._svn_status_verbose(repo_dir_path)
+        log_process_output(svn_output)
+        print(svn_output)
 
     @staticmethod
     def xml_status_is_dirty(svn_output):
@@ -173,17 +179,18 @@ def xml_status_is_dirty(svn_output):
                 is_dirty = True
         return is_dirty
 
-    def svn_status(self, stat, repo_dir_path):
-        """Report whether the svn repository is in-sync with the model
-        description and whether the sandbox is clean or dirty.
-
+    # ----------------------------------------------------------------
+    #
+    # system call to svn for information gathering
+    #
+    # ----------------------------------------------------------------
+    @staticmethod
+    def _svn_info(repo_dir_path):
+        """Return results of svn info command
         """
-        svn_output = self._svn_status_xml(repo_dir_path)
-        is_dirty = self.xml_status_is_dirty(svn_output)
-        if is_dirty:
-            stat.clean_state = ExternalStatus.DIRTY
-        else:
-            stat.clean_state = ExternalStatus.STATUS_OK
+        cmd = ['svn', 'info', repo_dir_path]
+        output = check_output(cmd)
+        return output
 
     @staticmethod
     def _svn_status_verbose(repo_dir_path):
@@ -193,10 +200,32 @@ def _svn_status_verbose(repo_dir_path):
         svn_output = check_output(cmd)
         return svn_output
 
-    def svn_status_verbose(self, repo_dir_path):
-        """Display the raw svn status output to the user.
+    @staticmethod
+    def _svn_status_xml(repo_dir_path):
+        """
+        Get status of the subversion sandbox in repo_dir
+        """
+        cmd = ['svn', 'status', '--xml', repo_dir_path]
+        svn_output = check_output(cmd)
+        return svn_output
 
+    # ----------------------------------------------------------------
+    #
+    # system call to svn for sideffects modifying the working tree
+    #
+    # ----------------------------------------------------------------
+    @staticmethod
+    def _svn_checkout(url, repo_dir_path):
         """
-        svn_output = self._svn_status_verbose(repo_dir_path)
-        log_process_output(svn_output)
-        print(svn_output)
+        Checkout a subversion repository (repo_url) to checkout_dir.
+        """
+        cmd = ['svn', 'checkout', url, repo_dir_path]
+        execute_subprocess(cmd)
+
+    @staticmethod
+    def _svn_switch(url):
+        """
+        Switch branches for in an svn sandbox
+        """
+        cmd = ['svn', 'switch', url]
+        execute_subprocess(cmd)

test/test_unit_repository_svn.py

@@ -19,6 +19,8 @@
 from manic.externals_description import ExternalsDescriptionDict
 from manic.global_constants import EMPTY_STR
 
+# pylint: disable=W0212
+
 SVN_INFO_MOSART = """Path: components/mosart
 Working Copy Root Path: /Users/andreb/projects/ncar/git-conversion/clm-dev-experimental/components/mosart
 URL: https://svn-ccsm-models.cgd.ucar.edu/mosart/trunk_tags/mosart1_0_26
@@ -82,15 +84,15 @@ def test_check_url_same(self):
         """
         svn_output = SVN_INFO_MOSART
         expected_url = self._repo.url()
-        result = self._repo.svn_check_url(svn_output, expected_url)
+        result = self._repo._check_url(svn_output, expected_url)
         self.assertEqual(result, ExternalStatus.STATUS_OK)
 
     def test_check_url_different(self):
         """Test that we correctly reject an incorrect URL.
         """
         svn_output = SVN_INFO_CISM
         expected_url = self._repo.url()
-        result = self._repo.svn_check_url(svn_output, expected_url)
+        result = self._repo._check_url(svn_output, expected_url)
         self.assertEqual(result, ExternalStatus.MODEL_MODIFIED)
 
     def test_check_url_none(self):
@@ -100,7 +102,7 @@ def test_check_url_none(self):
         """
         svn_output = EMPTY_STR
         expected_url = self._repo.url()
-        result = self._repo.svn_check_url(svn_output, expected_url)
+        result = self._repo._check_url(svn_output, expected_url)
         self.assertEqual(result, ExternalStatus.UNKNOWN)
 
 
@@ -161,7 +163,7 @@ def test_repo_dir_not_exist(self):
 
         """
         stat = ExternalStatus()
-        self._repo.svn_check_sync(stat, 'junk')
+        self._repo._check_sync(stat, 'junk')
         self.assertEqual(stat.sync_state, ExternalStatus.STATUS_ERROR)
         # check_dir should only modify the sync_state, not clean_state
         self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT)
@@ -172,8 +174,8 @@ def test_repo_dir_exist_no_svn_info(self):
         stat = ExternalStatus()
         # Now we over-ride the _svn_info method on the repo to return
         # a known value without requiring access to svn.
-        self._repo.svn_info = self._svn_info_empty
-        self._repo.svn_check_sync(stat, '.')
+        self._repo._svn_info = self._svn_info_empty
+        self._repo._check_sync(stat, '.')
         self.assertEqual(stat.sync_state, ExternalStatus.UNKNOWN)
         # check_dir should only modify the sync_state, not clean_state
         self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT)
@@ -186,8 +188,8 @@ def test_repo_dir_synced(self):
         stat = ExternalStatus()
         # Now we over-ride the _svn_info method on the repo to return
         # a known value without requiring access to svn.
-        self._repo.svn_info = self._svn_info_synced
-        self._repo.svn_check_sync(stat, '.')
+        self._repo._svn_info = self._svn_info_synced
+        self._repo._check_sync(stat, '.')
         self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK)
         # check_dir should only modify the sync_state, not clean_state
         self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT)
@@ -200,8 +202,8 @@ def test_repo_dir_modified(self):
         stat = ExternalStatus()
         # Now we over-ride the _svn_info method on the repo to return
         # a known value without requiring access to svn.
-        self._repo.svn_info = self._svn_info_modified
-        self._repo.svn_check_sync(stat, '.')
+        self._repo._svn_info = self._svn_info_modified
+        self._repo._check_sync(stat, '.')
         self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED)
         # check_dir should only modify the sync_state, not clean_state
         self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT)