From 21813e9d514ee7379102e1cd800455b46c8cc0b2 Mon Sep 17 00:00:00 2001 From: Ben Andre Date: Wed, 4 Apr 2018 17:38:44 -0600 Subject: [PATCH 1/8] Add system test demonstrating failure to detect out of sync status. Add a system test demonstrating a bug that checkout_externals fails to detect out-of-sync status for git when there are commits on a detached head state. Testing: make test - python2/3 - 1 new test fails, all existing tests pass. --- test/test_sys_checkout.py | 94 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 88 insertions(+), 6 deletions(-) diff --git a/test/test_sys_checkout.py b/test/test_sys_checkout.py index 80feea8..443dd70 100644 --- a/test/test_sys_checkout.py +++ b/test/test_sys_checkout.py @@ -83,6 +83,7 @@ CFG_NAME = 'externals.cfg' CFG_SUB_NAME = 'sub-externals.cfg' README_NAME = 'readme.txt' +REMOTE_BRANCH_FEATURE2 = 'feature2' SVN_TEST_REPO = 'https://github.com/escomp/cesm' @@ -130,7 +131,7 @@ def container_full(self, dest_dir): tag='tag1') self.create_section(SIMPLE_REPO_NAME, 'simp_branch', - branch='feature2') + branch=REMOTE_BRANCH_FEATURE2) self.create_section(SIMPLE_REPO_NAME, 'simp_opt', tag='tag1', required=False) @@ -149,7 +150,7 @@ def container_simple_required(self, dest_dir): tag='tag1') self.create_section(SIMPLE_REPO_NAME, 'simp_branch', - branch='feature2') + branch=REMOTE_BRANCH_FEATURE2) self.create_section(SIMPLE_REPO_NAME, 'simp_hash', ref_hash='60b1cc1a38d63') @@ -191,7 +192,7 @@ def mixed_simple_base(self, dest_dir): tag='tag1') self.create_section(SIMPLE_REPO_NAME, 'simp_branch', - branch='feature2') + branch=REMOTE_BRANCH_FEATURE2) self.create_section(SIMPLE_REPO_NAME, 'simp_hash', ref_hash='60b1cc1a38d63') @@ -207,7 +208,8 @@ def mixed_simple_sub(self, dest_dir): tag='tag1', path=SUB_EXTERNALS_PATH) self.create_section(SIMPLE_REPO_NAME, 'simp_branch', - branch='feature2', path=SUB_EXTERNALS_PATH) + branch=REMOTE_BRANCH_FEATURE2, + path=SUB_EXTERNALS_PATH) self._write_config(dest_dir, filename=CFG_SUB_NAME) @@ -329,6 +331,31 @@ def create_branch(dest_dir, repo_name, branch, with_commit=False): execute_subprocess(cmd) os.chdir(cwd) + @staticmethod + def create_commit(dest_dir, repo_name, local_tracking_branch=None): + """Make a commit on whatever is currently checked out. + + This is used to test sync state changes from local commits on + detached heads and tracking branches. + + """ + cwd = os.getcwd() + repo_root = os.path.join(dest_dir, EXTERNALS_NAME) + repo_root = os.path.join(repo_root, repo_name) + os.chdir(repo_root) + if local_tracking_branch: + cmd = ['git', 'checkout', '-b', local_tracking_branch, ] + execute_subprocess(cmd) + + msg = 'work on great new feature!' + with open(README_NAME, 'a') as handle: + handle.write(msg) + cmd = ['git', 'add', README_NAME, ] + execute_subprocess(cmd) + cmd = ['git', 'commit', '-m', msg, ] + execute_subprocess(cmd) + os.chdir(cwd) + def update_branch(self, dest_dir, name, branch, repo_type=None, filename=CFG_NAME): """Update a repository branch, and potentially the remote. @@ -601,6 +628,10 @@ def _check_simple_tag_dirty(self, tree, directory=EXTERNALS_NAME): name = './{0}/simp_tag'.format(directory) self._check_generic_ok_dirty_required(tree, name) + def _check_simple_tag_modified(self, tree, directory=EXTERNALS_NAME): + name = './{0}/simp_tag'.format(directory) + self._check_generic_modified_ok_required(tree, name) + def _check_simple_branch_empty(self, tree, directory=EXTERNALS_NAME): name = './{0}/simp_branch'.format(directory) self._check_generic_empty_default_required(tree, name) @@ -621,6 +652,10 @@ def _check_simple_hash_ok(self, tree, directory=EXTERNALS_NAME): name = './{0}/simp_hash'.format(directory) self._check_generic_ok_clean_required(tree, name) + def _check_simple_hash_modified(self, tree, directory=EXTERNALS_NAME): + name = './{0}/simp_hash'.format(directory) + self._check_generic_modified_ok_required(tree, name) + def _check_simple_req_empty(self, tree, directory=EXTERNALS_NAME): name = './{0}/simp_req'.format(directory) self._check_generic_empty_default_required(tree, name) @@ -673,6 +708,12 @@ def _check_container_simple_required_post_checkout(self, overall, tree): self._check_simple_branch_ok(tree) self._check_simple_hash_ok(tree) + def _check_container_simple_required_out_of_sync(self, overall, tree): + self.assertEqual(overall, 0) + self._check_simple_tag_modified(tree) + self._check_simple_branch_modified(tree) + self._check_simple_hash_modified(tree) + def _check_container_simple_optional_pre_checkout(self, overall, tree): self.assertEqual(overall, 0) self._check_simple_req_empty(tree) @@ -941,6 +982,47 @@ def test_container_simple_untracked(self): self.status_args) self._check_container_simple_required_post_checkout(overall, tree) + def test_container_simple_detached_sync(self): + """Verify that a container with simple subrepos generates the correct + out of sync status when making commits from a detached head + state. + + """ + # create repo + under_test_dir = self.setup_test_repo(CONTAINER_REPO_NAME) + self._generator.container_simple_required(under_test_dir) + + # status of empty repo + overall, tree = self.execute_cmd_in_dir(under_test_dir, + self.status_args) + self._check_container_simple_required_pre_checkout(overall, tree) + + # checkout + overall, tree = self.execute_cmd_in_dir(under_test_dir, + self.checkout_args) + self._check_container_simple_required_checkout(overall, tree) + + # make a commit on the detached head of the tag and hash externals + self._generator.create_commit(under_test_dir, 'simp_tag') + self._generator.create_commit(under_test_dir, 'simp_hash') + self._generator.create_commit(under_test_dir, 'simp_branch') + + # status of repo, branch, tag and hash should all be out of sync! + overall, tree = self.execute_cmd_in_dir(under_test_dir, + self.status_args) + self._check_container_simple_required_out_of_sync(overall, tree) + + # checkout + overall, tree = self.execute_cmd_in_dir(under_test_dir, + self.checkout_args) + # same pre-checkout out of sync status + self._check_container_simple_required_out_of_sync(overall, tree) + + # now status should be in-sync + overall, tree = self.execute_cmd_in_dir(under_test_dir, + self.status_args) + self._check_container_simple_required_post_checkout(overall, tree) + def test_container_remote_branch(self): """Verify that a container with remote branch change works @@ -957,7 +1039,7 @@ def test_container_remote_branch(self): # update the config file to point to a different remote with # the same branch self._generator.update_branch(under_test_dir, 'simp_branch', - 'feature2', SIMPLE_FORK_NAME) + REMOTE_BRANCH_FEATURE2, SIMPLE_FORK_NAME) # status of simp_branch should be out of sync overall, tree = self.execute_cmd_in_dir(under_test_dir, @@ -1066,7 +1148,7 @@ def test_container_preserve_dot(self): # update the config file to point to a different remote with # the same branch self._generator.update_branch(under_test_dir, 'simp_branch', - 'feature2', SIMPLE_FORK_NAME) + REMOTE_BRANCH_FEATURE2, SIMPLE_FORK_NAME) # checkout overall, tree = self.execute_cmd_in_dir(under_test_dir, self.checkout_args) From 376c780225fe5af2f33b8b72bdd942823b7e4924 Mon Sep 17 00:00:00 2001 From: Ben Andre Date: Wed, 4 Apr 2018 19:15:28 -0600 Subject: [PATCH 2/8] Bugfix: detect and report 'detached from' correctly Bugfix to detect and report when a git repo is detached from a hash, tag, or branch. This changes the git _check_sync_logic function to compare the underlying hashes instead of the names of branches and tags. Testing: make stest - python2/3 - all system tests pass, including new test make utest - python2/3 - unit tests have 17 failures because current_ref_from_branch_command and check_sync_logic is now operating on the underlying hash rather than the tag and branch names. --- manic/repository_git.py | 71 ++++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 22 deletions(-) diff --git a/manic/repository_git.py b/manic/repository_git.py index f9e88df..8faa650 100644 --- a/manic/repository_git.py +++ b/manic/repository_git.py @@ -94,7 +94,7 @@ def _clone_repo(self, base_dir_path, repo_dir_name, verbosity): os.chdir(cwd) def _current_ref_from_branch_command(self, git_output): - """Parse output of the 'git branch -vv' command to determine the current + """Parse output of the 'git branch -vv' command to determine the *name* of current branch. The line starting with '*' is the current branch. It can be one of the following head states: @@ -104,24 +104,39 @@ def _current_ref_from_branch_command(self, git_output): * feature3 36418b4 Work on feature2 master 9b75494 [origin/master] Initialize repository. - 2. Detached from sha + 2. Detached at sha * (HEAD detached at 36418b4) 36418b4 Work on feature2 feature2 36418b4 [origin/feature2] Work on feature2 master 9b75494 [origin/master] Initialize repository. - 3. Detached from remote branch + 3. Detached at remote branch * (HEAD detached at origin/feature2) 36418b4 Work on feature2 feature2 36418b4 [origin/feature2] Work on feature2 feature3 36418b4 Work on feature2 master 9b75494 [origin/master] Initialize repository. - 4. Detached from tag + 4. Detached at tag, still at exactly the tag * (HEAD detached at clm4_5_18_r272) b837fc36 clm4_5_18_r272 - 5. On tracking branch. Note, may be may be ahead or behind remote. + 5. Detached from a sha, development beyond the sha + + * (HEAD detached from 60b1cc1) 046eeac work on great new feature! + master 9b75494 [origin/master] Initialize repository. + + 6. Detached from a branch, development beyond the branch + + * (HEAD detached from origin/feature2) 1c455f6 work on great new feature! + master 9b75494 [origin/master] Initialize repository. + + 7. Detached from a tag, development beyond the tag + + * (HEAD detached from tag1) 3bcf79f work on great new feature! + master 9b75494 [origin/master] Initialize repository. + + 8. On tracking branch. Note, may be may be ahead or behind remote. * master 562bac9a [origin/master] more test junk @@ -194,12 +209,11 @@ def _check_sync(self, stat, repo_dir_path): self._check_sync_logic(stat, repo_dir_path) def _check_sync_logic(self, stat, repo_dir_path): - """Isolate the complicated synce logic so it is not so deeply nested - and a bit easier to understand. - - Sync logic - only reporting on whether we are on the ref - (branch, tag, hash) specified in the externals description. + """Compare the underlying hashes of the currently checkout ref and the + expected ref. + Output: sets the sync_state as well as the current and + expected ref in the input status object. """ def compare_refs(current_ref, expected_ref): @@ -215,8 +229,9 @@ def compare_refs(current_ref, expected_ref): cwd = os.getcwd() os.chdir(repo_dir_path) - git_output = self._git_branch_vv() - current_ref = self._current_ref_from_branch_command(git_output) + # get the full hash of the current commit + current_ref = self._git_log_hash() + current_ref = current_ref.strip('"') if self._branch: if self._url == LOCAL_PATH_INDICATOR: @@ -230,23 +245,24 @@ def compare_refs(current_ref, expected_ref): else: expected_ref = "{0}/{1}".format(remote_name, self._branch) elif self._hash: - # NOTE(bja, 2018-03) For comparison purposes, we could - # determine which is longer and check that the short ref - # is a substring of the long ref. But it is simpler to - # just expand both to the full sha and do an exact - # comparison. - _, expected_ref = self._git_revparse_commit(self._hash) - _, current_ref = self._git_revparse_commit(current_ref) + expected_ref = self._hash else: expected_ref = self._tag + # record the *names* of the current and expected branches + git_output = self._git_branch_vv() + stat.current_version = self._current_ref_from_branch_command( + git_output) + stat.expected_version = copy.deepcopy(expected_ref) + + # get the underlying hash of the expected ref + _, expected_ref = self._git_revparse_commit(expected_ref) + # import pdb; pdb.set_trace() + # compare the underlying hashes stat.sync_state = compare_refs(current_ref, expected_ref) if current_ref == EMPTY_STR: stat.sync_state = ExternalStatus.UNKNOWN - stat.current_version = current_ref - stat.expected_version = expected_ref - os.chdir(cwd) def _determine_remote_name(self): @@ -594,6 +610,16 @@ def _status_v1z_is_dirty(git_output): # system call to git for information gathering # # ---------------------------------------------------------------- + @staticmethod + def _git_log_hash(): + """Run git log -1 --format='%H' to return the full hash of the + currently checkedout version. + + """ + cmd = ['git', 'log', '-1', '--format="%H"'] + git_output = execute_subprocess(cmd, output_to_caller=True) + return git_output.strip() + @staticmethod def _git_branch_vv(): """Run git branch -vv to obtain verbose branch information, including @@ -647,6 +673,7 @@ def _git_revparse_commit(ref): '{0}^{1}'.format(ref, '{commit}'), ] status, git_output = execute_subprocess(cmd, status_to_caller=True, output_to_caller=True) + git_output = git_output.strip() return status, git_output @staticmethod From 719383e9fc2dcfd79fec0860b19e3e687defca5d Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Fri, 6 Apr 2018 13:26:25 -0600 Subject: [PATCH 3/8] Remove commented-out pdb.set_trace() call --- manic/repository_git.py | 1 - 1 file changed, 1 deletion(-) diff --git a/manic/repository_git.py b/manic/repository_git.py index 8faa650..376401a 100644 --- a/manic/repository_git.py +++ b/manic/repository_git.py @@ -257,7 +257,6 @@ def compare_refs(current_ref, expected_ref): # get the underlying hash of the expected ref _, expected_ref = self._git_revparse_commit(expected_ref) - # import pdb; pdb.set_trace() # compare the underlying hashes stat.sync_state = compare_refs(current_ref, expected_ref) if current_ref == EMPTY_STR: From ca0a5d34d69d0587b20281017a11a7a8b2bed188 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Sun, 8 Apr 2018 10:04:00 -0600 Subject: [PATCH 4/8] Rework some git repository functions, and major rework of unit tests - Rename _current_ref_from_branch_command to _current_ref. This now calls _git_branch_vv rather than expecting it to be called in. This simplifies work for the caller of this function, and is a step towards the next rework I want to do. - Fix _current_ref to give the correct reference if we are detached FROM something: in this case, give the hash that we're on rather than the name of the branch/tag from which we are detached. - Change _check_sync_logic to handle the case where git revparse fails - Major overhaul of many of the repository_git unit tests. Get tests passing, remove tests that are now redundant due to earlier work on this branch, add some new tests needed with the rework on this branch, and generally rework some of the tests. --- manic/repository_git.py | 72 +++-- test/test_unit_repository_git.py | 440 +++++++++++++------------------ 2 files changed, 246 insertions(+), 266 deletions(-) diff --git a/manic/repository_git.py b/manic/repository_git.py index 376401a..98f94b1 100644 --- a/manic/repository_git.py +++ b/manic/repository_git.py @@ -37,10 +37,25 @@ class GitRepository(Repository): """ + # Note the distinction between "detached at" vs. "detached from": At + # least with recent versions of git, "detached at" means your HEAD + # is still at the same hash as the given reference; "detached from" + # means that your HEAD has moved past the hash corresponding to the + # given reference (branch or tag). (Ben Andre says that earlier + # versions of git, such as 1.8, did not distinguish between these + # situations, instead calling everything "detached from".) + # match XYZ of '* (HEAD detached at {XYZ}): # e.g. * (HEAD detached at origin/feature-2) - RE_DETACHED = re.compile( - r'\* \((?:[\w]+[\s]+)?detached (?:at|from) ([\w\-./]+)\)') + RE_DETACHED_AT = re.compile( + r'\* \((?:[\w]+[\s]+)?detached at ([\w\-./]+)\)') + + # match abc123 of '* (HEAD detached from XYZ) abc123': + # e.g. * (HEAD detached from origin/feature-2) abc123 + # (where abc123 is the current hash) + RE_DETACHED_FROM = re.compile( + r'\* \((?:[\w]+[\s]+)?detached from [\w\-./]+\)[\s]+([\w]+)') + # match tracking reference info, return XYZ from [XYZ] # e.g. [origin/master] @@ -93,9 +108,9 @@ def _clone_repo(self, base_dir_path, repo_dir_name, verbosity): self._git_clone(self._url, repo_dir_name, verbosity) os.chdir(cwd) - def _current_ref_from_branch_command(self, git_output): + def _current_ref(self): """Parse output of the 'git branch -vv' command to determine the *name* of current - branch. The line starting with '*' is the current branch. It + branch, tag or hash. The line starting with '*' is the current branch. It can be one of the following head states: 1. On local branch @@ -121,7 +136,8 @@ def _current_ref_from_branch_command(self, git_output): * (HEAD detached at clm4_5_18_r272) b837fc36 clm4_5_18_r272 - 5. Detached from a sha, development beyond the sha + 5. Detached from a sha, development beyond the sha (not sure if + this will ever occur) * (HEAD detached from 60b1cc1) 046eeac work on great new feature! master 9b75494 [origin/master] Initialize repository. @@ -151,6 +167,7 @@ def _current_ref_from_branch_command(self, git_output): command that will return the same info. """ + git_output = self._git_branch_vv() lines = git_output.splitlines() ref = '' for line in lines: @@ -162,12 +179,25 @@ def _current_ref_from_branch_command(self, git_output): # not a git repo? some other error? we return so the # caller can handle. pass - elif 'detached' in ref: - match = self.RE_DETACHED.search(ref) + elif 'detached at' in ref: + match = self.RE_DETACHED_AT.search(ref) try: + # In this case, match group 1 gives the reference we are + # detached at (e.g., the tag name) current_ref = match.group(1) except BaseException: - msg = 'DEV_ERROR: regex to detect detached head state failed!' + msg = 'DEV_ERROR: regex to detect "detached at" head state failed!' + msg += '\nref:\n{0}\ngit_output\n{1}\n'.format(ref, git_output) + fatal_error(msg) + elif 'detached from' in ref: + match = self.RE_DETACHED_FROM.search(ref) + try: + # In this case, match group 1 gives the current hash, + # because (at least with git v. 2) we are not actually + # at the given tag, but are beyond it + current_ref = match.group(1) + except BaseException: + msg = 'DEV_ERROR: regex to detect "detached from" head state failed!' msg += '\nref:\n{0}\ngit_output\n{1}\n'.format(ref, git_output) fatal_error(msg) elif '[' in ref: @@ -246,21 +276,31 @@ def compare_refs(current_ref, expected_ref): expected_ref = "{0}/{1}".format(remote_name, self._branch) elif self._hash: expected_ref = self._hash - else: + elif self._tag: expected_ref = self._tag + else: + msg = 'In repo "{0}": none of branch, hash or tag are set'.format( + self._name) + fatal_error(msg) # record the *names* of the current and expected branches - git_output = self._git_branch_vv() - stat.current_version = self._current_ref_from_branch_command( - git_output) + stat.current_version = self._current_ref() stat.expected_version = copy.deepcopy(expected_ref) - # get the underlying hash of the expected ref - _, expected_ref = self._git_revparse_commit(expected_ref) - # compare the underlying hashes - stat.sync_state = compare_refs(current_ref, expected_ref) if current_ref == EMPTY_STR: stat.sync_state = ExternalStatus.UNKNOWN + else: + # get the underlying hash of the expected ref + revparse_status, expected_ref_hash = self._git_revparse_commit(expected_ref) + if revparse_status: + # We failed to get the hash associated with + # expected_ref. Maybe we should assign this to some special + # status, but for now we're just calling this out-of-sync to + # remain consistent with how this worked before. + stat.sync_state = ExternalStatus.MODEL_MODIFIED + else: + # compare the underlying hashes + stat.sync_state = compare_refs(current_ref, expected_ref_hash) os.chdir(cwd) diff --git a/test/test_unit_repository_git.py b/test/test_unit_repository_git.py index d481995..0565af9 100644 --- a/test/test_unit_repository_git.py +++ b/test/test_unit_repository_git.py @@ -23,13 +23,14 @@ from manic.externals_description import ExternalsDescriptionDict from manic.global_constants import EMPTY_STR -# pylint: disable=C0103 -GIT_BRANCH_OUTPUT_DETACHED_BRANCH_v1_8 = ''' -* (detached from origin/feature2) 36418b4 Work on feature2 - master 9b75494 [origin/master] Initialize repository. -''' -# pylint: enable=C0103 - +# In the following git branch output, note the distinction between +# "detached at" vs. "detached from": At least with recent versions of +# git, "detached at" means your HEAD is still at the same hash as the +# given reference; "detached from" means that your HEAD has moved past +# the hash corresponding to the given reference (branch or tag). (Ben +# Andre says that earlier versions of git, such as 1.8, did not +# distinguish between these situations, instead calling everything +# "detached from".) GIT_BRANCH_OUTPUT_DETACHED_BRANCH = ''' * (HEAD detached at origin/feature-2) 36418b4 Work on feature-2 @@ -38,6 +39,11 @@ master 9b75494 [origin/master] Initialize repository. ''' +GIT_BRANCH_OUTPUT_DETACHED_FROM_BRANCH = ''' +* (HEAD detached from origin/feature2) 36418b4 Work on feature2 + master 9b75494 [origin/master] Initialize repository. +''' + GIT_BRANCH_OUTPUT_DETACHED_HASH = ''' * (HEAD detached at 36418b4) 36418b4 Work on feature-2 feature-2 36418b4 [origin/feature-2] Work on feature-2 @@ -52,6 +58,13 @@ master 9b75494 [origin/master] Initialize repository. ''' +GIT_BRANCH_OUTPUT_DETACHED_FROM_TAG = ''' +* (HEAD detached from tag1) 9b75494 Initialize repository. + feature-2 36418b4 [origin/feature-2] Work on feature-2 + feature3 36418b4 Work on feature-2 + master 9b75494 [origin/master] Initialize repository. +''' + GIT_BRANCH_OUTPUT_UNTRACKED_BRANCH = ''' feature-2 36418b4 [origin/feature-2] Work on feature-2 * feature3 36418b4 Work on feature-2 @@ -112,52 +125,82 @@ def setUp(self): repo = model[self._name][ExternalsDescription.REPO] self._repo = GitRepository('test', repo) - def test_ref_detached_from_tag(self): - """Test that we correctly identify that the ref is detached from a tag + # + # mock methods replacing git system calls + # + @staticmethod + def _git_branch_vv(output): + """Return a function that takes the place of + repo._git_branch_vv, which returns the given output. """ - git_output = GIT_BRANCH_OUTPUT_DETACHED_TAG + def my_git_branch_vv(): + return output + return my_git_branch_vv + + # ------------------------------------------------------------------------ + # Begin tests + # + # See above for distinction between "detached at" vs. "detached from" + # ------------------------------------------------------------------------ + + def test_ref_detached_tag(self): + """Test that we correctly identify that the ref is detached at a tag + """ + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_DETACHED_TAG) expected = self._repo.tag() - result = self._repo._current_ref_from_branch_command( - git_output) + result = self._repo._current_ref() + self.assertEqual(result, expected) + + def test_ref_detached_from_tag(self): + """Test that we correctly identify that the ref is detached FROM a tag + + In this case, the best we can do is to list the hash we're on + """ + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_DETACHED_FROM_TAG) + expected = '9b75494' + result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_detached_hash(self): - """Test that we can identify ref is detached from a hash + """Test that we can identify ref is detached at a hash """ - git_output = GIT_BRANCH_OUTPUT_DETACHED_HASH + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_DETACHED_HASH) expected = '36418b4' - result = self._repo._current_ref_from_branch_command( - git_output) + result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_detached_branch(self): - """Test that we can identify ref is detached from a remote branch + """Test that we can identify ref is detached at a remote branch """ - git_output = GIT_BRANCH_OUTPUT_DETACHED_BRANCH + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_DETACHED_BRANCH) expected = 'origin/feature-2' - result = self._repo._current_ref_from_branch_command( - git_output) + result = self._repo._current_ref() self.assertEqual(result, expected) - def test_ref_detached_branch_v1_8(self): - """Test that we can identify ref is detached from a remote branch + def test_ref_detached_from_branch(self): + """Test that we can identify ref is detached FROM a remote branch + In this case, the best we can do is to list the hash we're on """ - git_output = GIT_BRANCH_OUTPUT_DETACHED_BRANCH_v1_8 - expected = 'origin/feature2' - result = self._repo._current_ref_from_branch_command( - git_output) + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_DETACHED_FROM_BRANCH) + expected = '36418b4' + result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_tracking_branch(self): """Test that we correctly identify we are on a tracking branch """ - git_output = GIT_BRANCH_OUTPUT_TRACKING_BRANCH + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_TRACKING_BRANCH) expected = 'origin/feature-2' - result = self._repo._current_ref_from_branch_command( - git_output) + result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_tracking_branch_ahead(self): @@ -165,10 +208,10 @@ def test_ref_tracking_branch_ahead(self): ahead or behind the remote branch. """ - git_output = GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD) expected = 'origin/master' - result = self._repo._current_ref_from_branch_command( - git_output) + result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_tracking_branch_ahead_behind(self): # pylint: disable=C0103 @@ -176,19 +219,19 @@ def test_ref_tracking_branch_ahead_behind(self): # pylint: disable=C0103 ahead or behind the remote branch. """ - git_output = GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD_BEHIND + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD_BEHIND) expected = 'origin/master' - result = self._repo._current_ref_from_branch_command( - git_output) + result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_untracked_branch(self): """Test that we correctly identify we are on an untracked branch """ - git_output = GIT_BRANCH_OUTPUT_UNTRACKED_BRANCH + self._repo._git_branch_vv = self._git_branch_vv( + GIT_BRANCH_OUTPUT_UNTRACKED_BRANCH) expected = 'feature3' - result = self._repo._current_ref_from_branch_command( - git_output) + result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_none(self): @@ -196,9 +239,9 @@ def test_ref_none(self): repo. """ - git_output = EMPTY_STR - received = self._repo._current_ref_from_branch_command( - git_output) + self._repo._git_branch_vv = self._git_branch_vv( + EMPTY_STR) + received = self._repo._current_ref() self.assertEqual(received, EMPTY_STR) @@ -264,6 +307,11 @@ def setUp(self): model = ExternalsDescriptionDict(data) repo = model[self._name][ExternalsDescription.REPO] self._repo = GitRepository('test', repo) + # The unit tests here don't care about the result of + # _git_branch_vv, but we replace it here so that we don't need + # to worry about calling a possibly slow and possibly + # error-producing command: + self._repo._git_branch_vv = self._git_branch_empty self._create_tmp_git_dir() def tearDown(self): @@ -293,46 +341,39 @@ def _git_branch_empty(): return EMPTY_STR @staticmethod - def _git_branch_detached_tag(): - """Return an info sting that is a checkouted tag - """ - return GIT_BRANCH_OUTPUT_DETACHED_TAG - - @staticmethod - def _git_branch_detached_hash(): + def _git_remote_origin_upstream(): """Return an info string that is a checkout hash """ - return GIT_BRANCH_OUTPUT_DETACHED_HASH + return GIT_REMOTE_OUTPUT_ORIGIN_UPSTREAM @staticmethod - def _git_branch_detached_branch(): + def _git_remote_none(): """Return an info string that is a checkout hash """ - return GIT_BRANCH_OUTPUT_DETACHED_BRANCH + return EMPTY_STR @staticmethod - def _git_branch_untracked_branch(): - """Return an info string that is a checkout branch + def _git_log_hash(myhash): + """Return a function that takes the place of repo._git_log_hash, + which returns the given hash """ - return GIT_BRANCH_OUTPUT_UNTRACKED_BRANCH + def my_git_log_hash(): + return myhash + return my_git_log_hash - @staticmethod - def _git_branch_tracked_branch(): - """Return an info string that is a checkout branch - """ - return GIT_BRANCH_OUTPUT_TRACKING_BRANCH + def _git_revparse_commit(self, expected_ref, mystatus, myhash): + """Return a function that takes the place of + repo._git_revparse_commit, which returns a tuple: + (mystatus, myhash). - @staticmethod - def _git_remote_origin_upstream(): - """Return an info string that is a checkout hash - """ - return GIT_REMOTE_OUTPUT_ORIGIN_UPSTREAM + Expects the passed-in ref to equal expected_ref - @staticmethod - def _git_remote_none(): - """Return an info string that is a checkout hash + status = 0 implies success, non-zero implies failure """ - return EMPTY_STR + def my_git_revparse_commit(ref): + self.assertEqual(expected_ref, ref) + return mystatus, myhash + return my_git_revparse_commit # ---------------------------------------------------------------- # @@ -353,105 +394,67 @@ def test_sync_dir_not_exist(self): self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) def test_sync_dir_exist_no_git_info(self): - """Test that an empty info string returns an unknown status + """Test that a non-existent git repo returns an unknown status """ stat = ExternalStatus() - # Now we over-ride the _git_branch method on the repo to return + # Now we over-ride the _git_remote_verbose method on the repo to return # a known value without requiring access to git. self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._git_branch_vv = self._git_branch_empty + self._repo._tag = 'tag1' + self._repo._git_log_hash = self._git_log_hash('') + self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 1, '') self._repo._check_sync(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.UNKNOWN) # check_sync should only modify the sync_state, not clean_state self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - # ---------------------------------------------------------------- - # - # Tests where external description specifies a tag + # ------------------------------------------------------------------------ # - # Perturbations of working dir state: on detached - # {tag|branch|hash}, tracking branch, untracked branch. + # Tests where version in configuration file is not a valid reference # - # ---------------------------------------------------------------- - def test_sync_tag_on_detached_tag(self): - """Test expect tag on detached tag --> status ok - - """ - stat = ExternalStatus() - self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = '' - self._repo._tag = 'tag1' - self._repo._git_branch_vv = self._git_branch_detached_tag - self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - - def test_sync_tag_on_diff_tag(self): - """Test expect tag on diff tag --> status modified - - """ - stat = ExternalStatus() - self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = '' - self._repo._tag = 'tag2' - self._repo._git_branch_vv = self._git_branch_detached_tag - self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - - def test_sync_tag_on_detached_hash(self): - """Test expect tag on detached hash --> status modified - - """ - stat = ExternalStatus() - self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = '' - self._repo._tag = 'tag1' - self._repo._git_branch_vv = self._git_branch_detached_hash - self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - - def test_sync_tag_on_detached_branch(self): - """Test expect tag on detached branch --> status modified + # ------------------------------------------------------------------------ + def test_sync_invalid_reference(self): + """Test that an invalid reference returns out-of-sync """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = '' self._repo._tag = 'tag1' - self._repo._git_branch_vv = self._git_branch_detached_branch + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 1, '') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - def test_sync_tag_on_tracking_branch(self): - """Test expect tag on tracking branch --> status modified + # ---------------------------------------------------------------- + # + # Tests where external description specifies a tag + # + # ---------------------------------------------------------------- + def test_sync_tag_on_same_hash(self): + """Test expect tag on same hash --> status ok """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = '' self._repo._tag = 'tag1' - self._repo._git_branch_vv = self._git_branch_tracked_branch + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) + self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) # check_sync should only modify the sync_state, not clean_state self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - def test_sync_tag_on_untracked_branch(self): - """Test expect tag on untracked branch --> status modified + def test_sync_tag_on_different_hash(self): + """Test expect tag on a different hash --> status modified """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = '' self._repo._tag = 'tag1' - self._repo._git_branch_vv = self._git_branch_untracked_branch + self._repo._git_log_hash = self._git_log_hash('def456') + self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state @@ -459,115 +462,78 @@ def test_sync_tag_on_untracked_branch(self): # ---------------------------------------------------------------- # - # Tests where external description specifies a branch - # - # Perturbations of working dir state: on detached - # {tag|branch|hash}, tracking branch, untracked branch. + # Tests where external description specifies a hash # # ---------------------------------------------------------------- - def test_sync_branch_on_detached_branch_same_remote(self): - """Test expect branch on detached branch with same remote --> status ok + def test_sync_hash_on_same_hash(self): + """Test expect hash on same hash --> status ok """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = 'feature-2' self._repo._tag = '' - self._repo._git_branch_vv = self._git_branch_detached_branch + self._repo._hash = 'abc' + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = self._git_revparse_commit('abc', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) # check_sync should only modify the sync_state, not clean_state self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - def test_sync_branch_on_detached_branch_diff_remote(self): - """Test expect branch on detached branch, different remote --> status modified - - """ - stat = ExternalStatus() - self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = 'feature-2' - self._repo._tag = '' - self._repo._url = '/path/to/other/repo' - self._repo._git_branch_vv = self._git_branch_detached_branch - self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - - def test_sync_branch_on_detached_branch_diff_remote2(self): - """Test expect branch on detached branch, different remote --> status modified + def test_sync_hash_on_different_hash(self): + """Test expect hash on a different hash --> status modified """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = 'feature-2' self._repo._tag = '' - self._repo._url = '/path/to/local/repo2' - self._repo._git_branch_vv = self._git_branch_detached_branch + self._repo._hash = 'abc' + self._repo._git_log_hash = self._git_log_hash('def456') + self._repo._git_revparse_commit = self._git_revparse_commit('abc', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - def test_sync_branch_on_diff_branch(self): - """Test expect branch on diff branch --> status modified - - """ - stat = ExternalStatus() - self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = 'nice_new_feature' - self._repo._tag = '' - self._repo._git_branch_vv = self._git_branch_detached_branch - self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - - def test_sync_branch_on_detached_hash(self): - """Test expect branch on detached hash --> status modified + # ---------------------------------------------------------------- + # + # Tests where external description specifies a branch + # + # ---------------------------------------------------------------- + def test_sync_branch_on_same_hash(self): + """Test expect branch on same hash --> status ok """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._branch = 'feature-2' self._repo._tag = '' - self._repo._git_branch_vv = self._git_branch_detached_hash + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = ( + self._git_revparse_commit('origin/feature-2', 0, 'abc123')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) + self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) # check_sync should only modify the sync_state, not clean_state self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - def test_sync_branch_on_detached_tag(self): - """Test expect branch on detached tag --> status modified + def test_sync_branch_on_diff_hash(self): + """Test expect branch on diff hash --> status modified """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._branch = 'feature-2' self._repo._tag = '' - self._repo._git_branch_vv = self._git_branch_detached_tag + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = ( + self._git_revparse_commit('origin/feature-2', 0, 'def456')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - def test_sync_branch_on_tracking_branch_same_remote(self): - """Test expect branch on tracking branch with same remote --> status ok - - """ - stat = ExternalStatus() - self._repo._git_remote_verbose = self._git_remote_origin_upstream - self._repo._branch = 'feature-2' - self._repo._tag = '' - self._repo._git_branch_vv = self._git_branch_tracked_branch - self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - - def test_sync_branch_on_tracking_branch_diff_remote(self): - """Test expect branch on tracking branch with different remote--> - status modified + def test_sync_branch_diff_remote(self): + """Test _determine_remote_name with a different remote """ stat = ExternalStatus() @@ -575,30 +541,28 @@ def test_sync_branch_on_tracking_branch_diff_remote(self): self._repo._branch = 'feature-2' self._repo._tag = '' self._repo._url = '/path/to/other/repo' - self._repo._git_branch_vv = self._git_branch_tracked_branch + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = ( + self._git_revparse_commit('upstream/feature-2', 0, 'def456')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) - - def test_sync_branch_on_untracked_branch(self): - """Test expect branch on untracked branch --> status modified + # The test passes if _git_revparse_commit is called with the + # expected argument - NOTE(bja, 2017-11) the externals description url is always a - remote repository. A local untracked branch only exists - locally, therefore it is always a modified state, even if this - is what the user wants. + def test_sync_branch_diff_remote2(self): + """Test _determine_remote_name with a different remote """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._branch = 'feature-2' self._repo._tag = '' - self._repo._git_branch_vv = self._git_branch_untracked_branch + self._repo._url = '/path/to/local/repo2' + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = ( + self._git_revparse_commit('other/feature-2', 0, 'def789')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) - self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) - # check_sync should only modify the sync_state, not clean_state - self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) + # The test passes if _git_revparse_commit is called with the + # expected argument def test_sync_branch_on_unknown_remote(self): """Test expect branch, but remote is unknown --> status modified @@ -609,7 +573,9 @@ def test_sync_branch_on_unknown_remote(self): self._repo._branch = 'feature-2' self._repo._tag = '' self._repo._url = '/path/to/unknown/repo' - self._repo._git_branch_vv = self._git_branch_untracked_branch + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = ( + self._git_revparse_commit('unknown_remote/feature-2', 1, '')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state @@ -619,22 +585,19 @@ def test_sync_branch_on_untracked_local(self): """Test expect branch, on untracked branch in local repo --> status ok Setting the externals description to '.' indicates that the - user only want's to consider the current local repo state + user only wants to consider the current local repo state without fetching from remotes. This is required to preserve the current branch of a repository during an update. - NOTE(bja, 2017-11) the externals description is always a - remote repository. A local untracked branch only exists - locally, therefore it is always a modified state, even if this - is what the user wants. - """ stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._branch = 'feature3' self._repo._tag = '' - self._repo._git_branch_vv = self._git_branch_untracked_branch self._repo._url = '.' + self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_revparse_commit = ( + self._git_revparse_commit('feature3', 0, 'abc123')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) # check_sync should only modify the sync_state, not clean_state @@ -653,25 +616,18 @@ def setUp(self): self._detached_git_v2_tmpl = string.Template( '* (HEAD detached at $ref) 36418b4 Work on feature-2') - self._detached_git_v1_tmpl = string.Template( - '* (detached from $ref) 36418b4 Work on feature-2') - self._tracking_tmpl = string.Template( '* feature-2 36418b4 [$ref] Work on feature-2') # - # RE_DETACHED + # RE_DETACHED_AT # def test_re_detached_alphnum(self): """Test re correctly matches alphnumeric (basic debugging) """ value = 'feature2' input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - input_str = self._detached_git_v1_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) + match = GitRepository.RE_DETACHED_AT.search(input_str) self.assertIsNotNone(match) self.assertEqual(match.group(1), value) @@ -680,11 +636,7 @@ def test_re_detached_underscore(self): """ value = 'feature_2' input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - input_str = self._detached_git_v1_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) + match = GitRepository.RE_DETACHED_AT.search(input_str) self.assertIsNotNone(match) self.assertEqual(match.group(1), value) @@ -693,11 +645,7 @@ def test_re_detached_hyphen(self): """ value = 'feature-2' input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - input_str = self._detached_git_v1_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) + match = GitRepository.RE_DETACHED_AT.search(input_str) self.assertIsNotNone(match) self.assertEqual(match.group(1), value) @@ -706,11 +654,7 @@ def test_re_detached_period(self): """ value = 'feature.2' input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - input_str = self._detached_git_v1_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) + match = GitRepository.RE_DETACHED_AT.search(input_str) self.assertIsNotNone(match) self.assertEqual(match.group(1), value) @@ -719,11 +663,7 @@ def test_re_detached_slash(self): """ value = 'feature/2' input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - input_str = self._detached_git_v1_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED.search(input_str) + match = GitRepository.RE_DETACHED_AT.search(input_str) self.assertIsNotNone(match) self.assertEqual(match.group(1), value) From 92d342c9287cafaaec23f600c62765ffe3eeac79 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Mon, 9 Apr 2018 09:01:10 -0600 Subject: [PATCH 5/8] Rewrite _current_ref to use plumbing rather than parsing porcelain Previously, _current_ref attempted to do everything with the output from git branch -vv. This required some complex regex matches that were fragile and hard to maintain. In addition, differences between git v. 1 and git v. 2 meant that the result was incorrect in some cases - particularly, in the case where we have "detached from foo" (which is the text that always appeared for a detached head in v 1, but in v 2 means we are no longer at foo). I have rewritten _current_ref to use various plumbing commands. This also resulted in being able to simplify some of the unit tests. --- manic/repository_git.py | 224 +++++++----------- test/test_sys_repository_git.py | 188 +++++++++++++++ test/test_unit_repository_git.py | 391 ++++++------------------------- 3 files changed, 339 insertions(+), 464 deletions(-) create mode 100644 test/test_sys_repository_git.py diff --git a/manic/repository_git.py b/manic/repository_git.py index 98f94b1..ab70802 100644 --- a/manic/repository_git.py +++ b/manic/repository_git.py @@ -37,30 +37,6 @@ class GitRepository(Repository): """ - # Note the distinction between "detached at" vs. "detached from": At - # least with recent versions of git, "detached at" means your HEAD - # is still at the same hash as the given reference; "detached from" - # means that your HEAD has moved past the hash corresponding to the - # given reference (branch or tag). (Ben Andre says that earlier - # versions of git, such as 1.8, did not distinguish between these - # situations, instead calling everything "detached from".) - - # match XYZ of '* (HEAD detached at {XYZ}): - # e.g. * (HEAD detached at origin/feature-2) - RE_DETACHED_AT = re.compile( - r'\* \((?:[\w]+[\s]+)?detached at ([\w\-./]+)\)') - - # match abc123 of '* (HEAD detached from XYZ) abc123': - # e.g. * (HEAD detached from origin/feature-2) abc123 - # (where abc123 is the current hash) - RE_DETACHED_FROM = re.compile( - r'\* \((?:[\w]+[\s]+)?detached from [\w\-./]+\)[\s]+([\w]+)') - - - # match tracking reference info, return XYZ from [XYZ] - # e.g. [origin/master] - RE_TRACKING = re.compile(r'\[([\w\-./]+)(?::[\s]+[\w\s,]+)?\]') - def __init__(self, component_name, repo): """ Parse repo (a XML element). @@ -109,110 +85,41 @@ def _clone_repo(self, base_dir_path, repo_dir_name, verbosity): os.chdir(cwd) def _current_ref(self): - """Parse output of the 'git branch -vv' command to determine the *name* of current - branch, tag or hash. The line starting with '*' is the current branch. It - can be one of the following head states: - - 1. On local branch - - feature2 36418b4 [origin/feature2] Work on feature2 - * feature3 36418b4 Work on feature2 - master 9b75494 [origin/master] Initialize repository. - - 2. Detached at sha - - * (HEAD detached at 36418b4) 36418b4 Work on feature2 - feature2 36418b4 [origin/feature2] Work on feature2 - master 9b75494 [origin/master] Initialize repository. - - 3. Detached at remote branch - - * (HEAD detached at origin/feature2) 36418b4 Work on feature2 - feature2 36418b4 [origin/feature2] Work on feature2 - feature3 36418b4 Work on feature2 - master 9b75494 [origin/master] Initialize repository. - - 4. Detached at tag, still at exactly the tag - - * (HEAD detached at clm4_5_18_r272) b837fc36 clm4_5_18_r272 - - 5. Detached from a sha, development beyond the sha (not sure if - this will ever occur) - - * (HEAD detached from 60b1cc1) 046eeac work on great new feature! - master 9b75494 [origin/master] Initialize repository. - - 6. Detached from a branch, development beyond the branch - - * (HEAD detached from origin/feature2) 1c455f6 work on great new feature! - master 9b75494 [origin/master] Initialize repository. - - 7. Detached from a tag, development beyond the tag - - * (HEAD detached from tag1) 3bcf79f work on great new feature! - master 9b75494 [origin/master] Initialize repository. - - 8. On tracking branch. Note, may be may be ahead or behind remote. - - * master 562bac9a [origin/master] more test junk - - * master 408a8920 [origin/master: ahead 3] more junk + """Determine the *name* associated with HEAD. + + If we're on a branch, then returns the branch name; otherwise, + if we're on a tag, then returns the tag name; otherwise, returns + the current hash. Returns an empty string if no reference can be + determined (e.g., if we're not actually in a git repository). + """ + ref_found = False + + # If we're on a branch, then use that as the current ref + branch_found, branch_name = self._git_current_branch() + if branch_found: + current_ref = branch_name + ref_found = True + + if not ref_found: + # Otherwise, if we're exactly at a tag, use that as the + # current ref + tag_found, tag_name = self._git_current_tag() + if tag_found: + current_ref = tag_name + ref_found = True + + if not ref_found: + # Otherwise, use current hash as the current ref + hash_found, hash_name = self._git_current_hash() + if hash_found: + current_ref = hash_name + ref_found = True + + if not ref_found: + # If we still can't find a ref, return empty string. This + # can happen if we're not actually in a git repo + current_ref = '' - * master 408a8920 [origin/master: ahead 3, behind 2] more junk - - * master 822d687d [origin/master: behind 3] more junk - - NOTE: Parsing the output of the porcelain is probably not a - great idea, but there doesn't appear to be a single plumbing - command that will return the same info. - - """ - git_output = self._git_branch_vv() - lines = git_output.splitlines() - ref = '' - for line in lines: - if line.startswith('*'): - ref = line - break - current_ref = EMPTY_STR - if not ref: - # not a git repo? some other error? we return so the - # caller can handle. - pass - elif 'detached at' in ref: - match = self.RE_DETACHED_AT.search(ref) - try: - # In this case, match group 1 gives the reference we are - # detached at (e.g., the tag name) - current_ref = match.group(1) - except BaseException: - msg = 'DEV_ERROR: regex to detect "detached at" head state failed!' - msg += '\nref:\n{0}\ngit_output\n{1}\n'.format(ref, git_output) - fatal_error(msg) - elif 'detached from' in ref: - match = self.RE_DETACHED_FROM.search(ref) - try: - # In this case, match group 1 gives the current hash, - # because (at least with git v. 2) we are not actually - # at the given tag, but are beyond it - current_ref = match.group(1) - except BaseException: - msg = 'DEV_ERROR: regex to detect "detached from" head state failed!' - msg += '\nref:\n{0}\ngit_output\n{1}\n'.format(ref, git_output) - fatal_error(msg) - elif '[' in ref: - match = self.RE_TRACKING.search(ref) - try: - current_ref = match.group(1) - except BaseException: - msg = 'DEV_ERROR: regex to detect tracking branch failed.' - msg += '\nref:\n{0}\ngit_output\n{1}\n'.format(ref, git_output) - fatal_error(msg) - else: - # assumed local branch - current_ref = ref.split()[1] - - current_ref = current_ref.strip() return current_ref def _check_sync(self, stat, repo_dir_path): @@ -260,8 +167,7 @@ def compare_refs(current_ref, expected_ref): os.chdir(repo_dir_path) # get the full hash of the current commit - current_ref = self._git_log_hash() - current_ref = current_ref.strip('"') + _, current_ref = self._git_current_hash() if self._branch: if self._url == LOCAL_PATH_INDICATOR: @@ -650,24 +556,58 @@ def _status_v1z_is_dirty(git_output): # # ---------------------------------------------------------------- @staticmethod - def _git_log_hash(): - """Run git log -1 --format='%H' to return the full hash of the - currently checkedout version. + def _git_current_hash(): + """Return the full hash of the currently checked-out version. + Returns a tuple, (hash_found, hash), where hash_found is a + logical specifying whether a hash was found for HEAD (False + could mean we're not in a git repository at all). (If hash_found + is False, then hash is ''.) """ - cmd = ['git', 'log', '-1', '--format="%H"'] - git_output = execute_subprocess(cmd, output_to_caller=True) - return git_output.strip() + status, git_output = GitRepository._git_revparse_commit("HEAD") + hash_found = not status + if not hash_found: + git_output = '' + return hash_found, git_output @staticmethod - def _git_branch_vv(): - """Run git branch -vv to obtain verbose branch information, including - upstream tracking and hash. + def _git_current_branch(): + """Determines the name of the current branch. + + Returns a tuple, (branch_found, branch_name), where branch_found + is a logical specifying whether a branch name was found for + HEAD. (If branch_found is False, then branch_name is ''.) + """ + cmd = ['git', 'symbolic-ref', '--short', '-q', 'HEAD'] + status, git_output = execute_subprocess(cmd, + output_to_caller=True, + status_to_caller=True) + branch_found = not status + if branch_found: + git_output = git_output.strip() + else: + git_output = '' + return branch_found, git_output - """ - cmd = ['git', 'branch', '--verbose', '--verbose'] - git_output = execute_subprocess(cmd, output_to_caller=True) - return git_output + @staticmethod + def _git_current_tag(): + """Determines the name tag corresponding to HEAD (if any). + + Returns a tuple, (tag_found, tag_name), where tag_found is a + logical specifying whether we found a tag name corresponding to + HEAD. (If tag_found is False, then tag_name is ''.) + """ + # git describe --exact-match --tags HEAD + cmd = ['git', 'describe', '--exact-match', '--tags', 'HEAD'] + status, git_output = execute_subprocess(cmd, + output_to_caller=True, + status_to_caller=True) + tag_found = not status + if tag_found: + git_output = git_output.strip() + else: + git_output = '' + return tag_found, git_output @staticmethod def _git_showref_tag(ref): diff --git a/test/test_sys_repository_git.py b/test/test_sys_repository_git.py new file mode 100644 index 0000000..395e5cb --- /dev/null +++ b/test/test_sys_repository_git.py @@ -0,0 +1,188 @@ +#!/usr/bin/env python + +"""Tests of some of the functionality in repository_git.py that actually +interacts with git repositories. + +We're calling these "system" tests because we expect them to be a lot +slower than most of the unit tests. + +""" + +from __future__ import absolute_import +from __future__ import unicode_literals +from __future__ import print_function + +import os +import shutil +import tempfile +import unittest + +from manic.repository_git import GitRepository +from manic.externals_description import ExternalsDescription +from manic.externals_description import ExternalsDescriptionDict +from manic.utils import execute_subprocess + +class GitTestCase(unittest.TestCase): + """Adds some git-specific unit test functionality on top of TestCase""" + + def assertIsHash(self, maybe_hash): + """Assert that the string given by maybe_hash really does look + like a git hash. + """ + + # Ensure it is non-empty + self.assertTrue(maybe_hash, msg="maybe_hash is empty") + + # Ensure it has a single string + self.assertEqual(1, len(maybe_hash.split()), + msg="maybe_hash has multiple strings: {}".format(maybe_hash)) + + # Ensure that the only characters in the string are ones allowed + # in hashes + allowed_chars_set = set('0123456789abcdef') + self.assertTrue(set(maybe_hash) <= allowed_chars_set, + msg="maybe_hash has non-hash characters: {}".format(maybe_hash)) + +class TestGitTestCase(GitTestCase): + """Tests GitTestCase""" + + def test_assertIsHash_true(self): + self.assertIsHash('abc123') + + def test_assertIsHash_empty(self): + with self.assertRaises(AssertionError): + self.assertIsHash('') + + def test_assertIsHash_multipleStrings(self): + with self.assertRaises(AssertionError): + self.assertIsHash('abc123 def456') + + def test_assertIsHash_badChar(self): + with self.assertRaises(AssertionError): + self.assertIsHash('abc123g') + +class TestGitRepositoryGitCommands(GitTestCase): + + # ======================================================================== + # Test helper functions + # ======================================================================== + + def setUp(self): + self._tmpdir = tempfile.mkdtemp() + os.chdir(self._tmpdir) + + # It's silly that we need to create a repository in order to + # test these git commands. Much or all of the git functionality + # that is currently in repository_git.py should eventually be + # moved to a separate module that is solely responsible for + # wrapping git commands; that would allow us to test it + # independently of this repository class. + self._name = 'component' + rdata = {ExternalsDescription.PROTOCOL: 'git', + ExternalsDescription.REPO_URL: + '/path/to/local/repo', + ExternalsDescription.TAG: + 'tag1', + } + + data = {self._name: + { + ExternalsDescription.REQUIRED: False, + ExternalsDescription.PATH: 'junk', + ExternalsDescription.EXTERNALS: '', + ExternalsDescription.REPO: rdata, + }, + } + model = ExternalsDescriptionDict(data) + repo = model[self._name][ExternalsDescription.REPO] + self._repo = GitRepository('test', repo) + + + def tearDown(self): + shutil.rmtree(self._tmpdir, ignore_errors=True) + + def make_git_repo(self): + """Turn the current directory into an empty git repository""" + execute_subprocess(['git', 'init']) + + def add_git_commit(self): + """Add a git commit in the current directory""" + with open('README', 'a') as myfile: + myfile.write('more info') + execute_subprocess(['git', 'add', 'README']) + execute_subprocess(['git', 'commit', '-m', 'my commit message']) + + def checkout_git_branch(self, branchname): + """Checkout a new branch in the current directory""" + execute_subprocess(['git', 'checkout', '-b', branchname]) + + def make_git_tag(self, tagname): + """Make a lightweight tag at the current commit""" + execute_subprocess(['git', 'tag', '-m', 'making a tag', tagname]) + + def checkout_ref(self, refname): + """Checkout the given refname in the current directory""" + execute_subprocess(['git', 'checkout', refname]) + + # ======================================================================== + # Begin actual tests + # ======================================================================== + + def test_currentHash_returnsHash(self): + self.make_git_repo() + self.add_git_commit() + hash_found, myhash = self._repo._git_current_hash() + self.assertTrue(hash_found) + self.assertIsHash(myhash) + + def test_currentHash_outsideGitRepo(self): + hash_found, myhash = self._repo._git_current_hash() + self.assertFalse(hash_found) + self.assertEqual('', myhash) + + def test_currentBranch_onBranch(self): + self.make_git_repo() + self.add_git_commit() + self.checkout_git_branch('foo') + branch_found, mybranch = self._repo._git_current_branch() + self.assertTrue(branch_found) + self.assertEqual('foo', mybranch) + + def test_currentBranch_notOnBranch(self): + self.make_git_repo() + self.add_git_commit() + self.make_git_tag('mytag') + self.checkout_ref('mytag') + branch_found, mybranch = self._repo._git_current_branch() + self.assertFalse(branch_found) + self.assertEqual('', mybranch) + + def test_currentBranch_outsideGitRepo(self): + branch_found, mybranch = self._repo._git_current_branch() + self.assertFalse(branch_found) + self.assertEqual('', mybranch) + + def test_currentTag_onTag(self): + self.make_git_repo() + self.add_git_commit() + self.make_git_tag('some_tag') + tag_found, mytag = self._repo._git_current_tag() + self.assertTrue(tag_found) + self.assertEqual('some_tag', mytag) + + def test_currentTag_notOnTag(self): + self.make_git_repo() + self.add_git_commit() + self.make_git_tag('some_tag') + self.add_git_commit() + tag_found, mytag = self._repo._git_current_tag() + self.assertFalse(tag_found) + self.assertEqual('', mytag) + + def test_currentTag_outsideGitRepo(self): + tag_found, mytag = self._repo._git_current_tag() + self.assertFalse(tag_found) + self.assertEqual('', mytag) + +if __name__ == '__main__': + unittest.main() diff --git a/test/test_unit_repository_git.py b/test/test_unit_repository_git.py index 0565af9..525a315 100644 --- a/test/test_unit_repository_git.py +++ b/test/test_unit_repository_git.py @@ -23,70 +23,6 @@ from manic.externals_description import ExternalsDescriptionDict from manic.global_constants import EMPTY_STR -# In the following git branch output, note the distinction between -# "detached at" vs. "detached from": At least with recent versions of -# git, "detached at" means your HEAD is still at the same hash as the -# given reference; "detached from" means that your HEAD has moved past -# the hash corresponding to the given reference (branch or tag). (Ben -# Andre says that earlier versions of git, such as 1.8, did not -# distinguish between these situations, instead calling everything -# "detached from".) - -GIT_BRANCH_OUTPUT_DETACHED_BRANCH = ''' -* (HEAD detached at origin/feature-2) 36418b4 Work on feature-2 - feature-2 36418b4 [origin/feature-2] Work on feature-2 - feature3 36418b4 Work on feature-2 - master 9b75494 [origin/master] Initialize repository. -''' - -GIT_BRANCH_OUTPUT_DETACHED_FROM_BRANCH = ''' -* (HEAD detached from origin/feature2) 36418b4 Work on feature2 - master 9b75494 [origin/master] Initialize repository. -''' - -GIT_BRANCH_OUTPUT_DETACHED_HASH = ''' -* (HEAD detached at 36418b4) 36418b4 Work on feature-2 - feature-2 36418b4 [origin/feature-2] Work on feature-2 - feature3 36418b4 Work on feature-2 - master 9b75494 [origin/master] Initialize repository. -''' - -GIT_BRANCH_OUTPUT_DETACHED_TAG = ''' -* (HEAD detached at tag1) 9b75494 Initialize repository. - feature-2 36418b4 [origin/feature-2] Work on feature-2 - feature3 36418b4 Work on feature-2 - master 9b75494 [origin/master] Initialize repository. -''' - -GIT_BRANCH_OUTPUT_DETACHED_FROM_TAG = ''' -* (HEAD detached from tag1) 9b75494 Initialize repository. - feature-2 36418b4 [origin/feature-2] Work on feature-2 - feature3 36418b4 Work on feature-2 - master 9b75494 [origin/master] Initialize repository. -''' - -GIT_BRANCH_OUTPUT_UNTRACKED_BRANCH = ''' - feature-2 36418b4 [origin/feature-2] Work on feature-2 -* feature3 36418b4 Work on feature-2 - master 9b75494 [origin/master] Initialize repository. -''' - -GIT_BRANCH_OUTPUT_TRACKING_BRANCH = ''' -* feature-2 36418b4 [origin/feature-2] Work on feature-2 - feature3 36418b4 Work on feature-2 - master 9b75494 [origin/master] Initialize repository. -''' - -GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD_BEHIND = ''' -* master 408a8920 [origin/master: ahead 3, behind 2] more junk - feature3 36418b4 Work on feature-2 -''' - -GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD = ''' -* master 408a8920 [origin/master: ahead 3] more junk - feature3 36418b4 Work on feature-2 -''' - # NOTE(bja, 2017-11) order is important here. origin should be a # subset of other to trap errors on processing remotes! GIT_REMOTE_OUTPUT_ORIGIN_UPSTREAM = ''' @@ -99,8 +35,8 @@ ''' -class TestGitRepositoryCurrentRefBranch(unittest.TestCase): - """test the current_ref_from_branch_command on a git repository +class TestGitRepositoryCurrentRef(unittest.TestCase): + """test the current_ref command on a git repository """ def setUp(self): @@ -129,37 +65,50 @@ def setUp(self): # mock methods replacing git system calls # @staticmethod - def _git_branch_vv(output): + def _git_current_branch(branch_found, branch_name): """Return a function that takes the place of - repo._git_branch_vv, which returns the given output. - """ - def my_git_branch_vv(): - return output - return my_git_branch_vv + repo._git_current_branch, which returns the given output.""" + def my_git_current_branch(): + return branch_found, branch_name + return my_git_current_branch + + @staticmethod + def _git_current_tag(tag_found, tag_name): + """Return a function that takes the place of + repo._git_current_tag, which returns the given output.""" + def my_git_current_tag(): + return tag_found, tag_name + return my_git_current_tag + + @staticmethod + def _git_current_hash(hash_found, hash_name): + """Return a function that takes the place of + repo._git_current_hash, which returns the given output.""" + def my_git_current_hash(): + return hash_found, hash_name + return my_git_current_hash # ------------------------------------------------------------------------ # Begin tests - # - # See above for distinction between "detached at" vs. "detached from" # ------------------------------------------------------------------------ - def test_ref_detached_tag(self): - """Test that we correctly identify that the ref is detached at a tag + def test_ref_branch(self): + """Test that we correctly identify we are on a branch """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_DETACHED_TAG) - expected = self._repo.tag() + self._repo._git_current_branch = self._git_current_branch(True, 'feature3') + self._repo._git_current_tag = self._git_current_tag(True, 'foo_tag') + self._repo._git_current_hash = self._git_current_hash(True, 'abc123') + expected = 'feature3' result = self._repo._current_ref() self.assertEqual(result, expected) - def test_ref_detached_from_tag(self): - """Test that we correctly identify that the ref is detached FROM a tag - - In this case, the best we can do is to list the hash we're on + def test_ref_detached_tag(self): + """Test that we correctly identify that the ref is detached at a tag """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_DETACHED_FROM_TAG) - expected = '9b75494' + self._repo._git_current_branch = self._git_current_branch(False, '') + self._repo._git_current_tag = self._git_current_tag(True, 'foo_tag') + self._repo._git_current_hash = self._git_current_hash(True, 'abc123') + expected = 'foo_tag' result = self._repo._current_ref() self.assertEqual(result, expected) @@ -167,82 +116,21 @@ def test_ref_detached_hash(self): """Test that we can identify ref is detached at a hash """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_DETACHED_HASH) - expected = '36418b4' - result = self._repo._current_ref() - self.assertEqual(result, expected) - - def test_ref_detached_branch(self): - """Test that we can identify ref is detached at a remote branch - - """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_DETACHED_BRANCH) - expected = 'origin/feature-2' - result = self._repo._current_ref() - self.assertEqual(result, expected) - - def test_ref_detached_from_branch(self): - """Test that we can identify ref is detached FROM a remote branch - - In this case, the best we can do is to list the hash we're on - """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_DETACHED_FROM_BRANCH) - expected = '36418b4' - result = self._repo._current_ref() - self.assertEqual(result, expected) - - def test_ref_tracking_branch(self): - """Test that we correctly identify we are on a tracking branch - """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_TRACKING_BRANCH) - expected = 'origin/feature-2' - result = self._repo._current_ref() - self.assertEqual(result, expected) - - def test_ref_tracking_branch_ahead(self): - """Test that we correctly identify we are on a tracking branch that is - ahead or behind the remote branch. - - """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD) - expected = 'origin/master' - result = self._repo._current_ref() - self.assertEqual(result, expected) - - def test_ref_tracking_branch_ahead_behind(self): # pylint: disable=C0103 - """Test that we correctly identify we are on a tracking branch that is - ahead or behind the remote branch. - - """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_TRACKING_BRANCH_AHEAD_BEHIND) - expected = 'origin/master' - result = self._repo._current_ref() - self.assertEqual(result, expected) - - def test_ref_untracked_branch(self): - """Test that we correctly identify we are on an untracked branch - """ - self._repo._git_branch_vv = self._git_branch_vv( - GIT_BRANCH_OUTPUT_UNTRACKED_BRANCH) - expected = 'feature3' + self._repo._git_current_branch = self._git_current_branch(False, '') + self._repo._git_current_tag = self._git_current_tag(False, '') + self._repo._git_current_hash = self._git_current_hash(True, 'abc123') + expected = 'abc123' result = self._repo._current_ref() self.assertEqual(result, expected) def test_ref_none(self): - """Test that we can handle an empty string for output, e.g. not an git - repo. - + """Test that we correctly identify that we're not in a git repo. """ - self._repo._git_branch_vv = self._git_branch_vv( - EMPTY_STR) - received = self._repo._current_ref() - self.assertEqual(received, EMPTY_STR) + self._repo._git_current_branch = self._git_current_branch(False, '') + self._repo._git_current_tag = self._git_current_tag(False, '') + self._repo._git_current_hash = self._git_current_hash(False, '') + result = self._repo._current_ref() + self.assertEqual(result, EMPTY_STR) class TestGitRepositoryCheckSync(unittest.TestCase): @@ -308,10 +196,11 @@ def setUp(self): repo = model[self._name][ExternalsDescription.REPO] self._repo = GitRepository('test', repo) # The unit tests here don't care about the result of - # _git_branch_vv, but we replace it here so that we don't need - # to worry about calling a possibly slow and possibly - # error-producing command: - self._repo._git_branch_vv = self._git_branch_empty + # _current_ref, but we replace it here so that we don't need to + # worry about calling a possibly slow and possibly + # error-producing command (since _current_ref calls various git + # functions): + self._repo._current_ref = self._current_ref_empty self._create_tmp_git_dir() def tearDown(self): @@ -335,8 +224,8 @@ def _remove_tmp_git_dir(self): # mock methods replacing git system calls # @staticmethod - def _git_branch_empty(): - """Return an empty info string. Simulates git info failing. + def _current_ref_empty(): + """Return an empty string. """ return EMPTY_STR @@ -353,13 +242,13 @@ def _git_remote_none(): return EMPTY_STR @staticmethod - def _git_log_hash(myhash): - """Return a function that takes the place of repo._git_log_hash, + def _git_current_hash(myhash): + """Return a function that takes the place of repo._git_current_hash, which returns the given hash """ - def my_git_log_hash(): - return myhash - return my_git_log_hash + def my_git_current_hash(): + return 0, myhash + return my_git_current_hash def _git_revparse_commit(self, expected_ref, mystatus, myhash): """Return a function that takes the place of @@ -401,7 +290,7 @@ def test_sync_dir_exist_no_git_info(self): # a known value without requiring access to git. self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' - self._repo._git_log_hash = self._git_log_hash('') + self._repo._git_current_hash = self._git_current_hash('') self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 1, '') self._repo._check_sync(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.UNKNOWN) @@ -420,7 +309,7 @@ def test_sync_invalid_reference(self): stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 1, '') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) @@ -439,7 +328,7 @@ def test_sync_tag_on_same_hash(self): stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) @@ -453,7 +342,7 @@ def test_sync_tag_on_different_hash(self): stat = ExternalStatus() self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' - self._repo._git_log_hash = self._git_log_hash('def456') + self._repo._git_current_hash = self._git_current_hash('def456') self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) @@ -473,7 +362,7 @@ def test_sync_hash_on_same_hash(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = '' self._repo._hash = 'abc' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = self._git_revparse_commit('abc', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) @@ -488,7 +377,7 @@ def test_sync_hash_on_different_hash(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = '' self._repo._hash = 'abc' - self._repo._git_log_hash = self._git_log_hash('def456') + self._repo._git_current_hash = self._git_current_hash('def456') self._repo._git_revparse_commit = self._git_revparse_commit('abc', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) @@ -508,7 +397,7 @@ def test_sync_branch_on_same_hash(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._branch = 'feature-2' self._repo._tag = '' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = ( self._git_revparse_commit('origin/feature-2', 0, 'abc123')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) @@ -524,7 +413,7 @@ def test_sync_branch_on_diff_hash(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._branch = 'feature-2' self._repo._tag = '' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = ( self._git_revparse_commit('origin/feature-2', 0, 'def456')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) @@ -541,7 +430,7 @@ def test_sync_branch_diff_remote(self): self._repo._branch = 'feature-2' self._repo._tag = '' self._repo._url = '/path/to/other/repo' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = ( self._git_revparse_commit('upstream/feature-2', 0, 'def456')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) @@ -557,7 +446,7 @@ def test_sync_branch_diff_remote2(self): self._repo._branch = 'feature-2' self._repo._tag = '' self._repo._url = '/path/to/local/repo2' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = ( self._git_revparse_commit('other/feature-2', 0, 'def789')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) @@ -573,7 +462,7 @@ def test_sync_branch_on_unknown_remote(self): self._repo._branch = 'feature-2' self._repo._tag = '' self._repo._url = '/path/to/unknown/repo' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = ( self._git_revparse_commit('unknown_remote/feature-2', 1, '')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) @@ -595,7 +484,7 @@ def test_sync_branch_on_untracked_local(self): self._repo._branch = 'feature3' self._repo._tag = '' self._repo._url = '.' - self._repo._git_log_hash = self._git_log_hash('abc123') + self._repo._git_current_hash = self._git_current_hash('abc123') self._repo._git_revparse_commit = ( self._git_revparse_commit('feature3', 0, 'abc123')) self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) @@ -604,148 +493,6 @@ def test_sync_branch_on_untracked_local(self): self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT) -class TestGitRegExp(unittest.TestCase): - """Test that the regular expressions in the GitRepository class - capture intended strings - - """ - - def setUp(self): - """Common constans - """ - self._detached_git_v2_tmpl = string.Template( - '* (HEAD detached at $ref) 36418b4 Work on feature-2') - - self._tracking_tmpl = string.Template( - '* feature-2 36418b4 [$ref] Work on feature-2') - - # - # RE_DETACHED_AT - # - def test_re_detached_alphnum(self): - """Test re correctly matches alphnumeric (basic debugging) - """ - value = 'feature2' - input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED_AT.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_detached_underscore(self): - """Test re matches with underscore - """ - value = 'feature_2' - input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED_AT.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_detached_hyphen(self): - """Test re matches - - """ - value = 'feature-2' - input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED_AT.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_detached_period(self): - """Test re matches . - """ - value = 'feature.2' - input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED_AT.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_detached_slash(self): - """Test re matches / - """ - value = 'feature/2' - input_str = self._detached_git_v2_tmpl.substitute(ref=value) - match = GitRepository.RE_DETACHED_AT.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - # - # RE_TRACKING - # - def test_re_tracking_alphnum(self): - """Test re matches alphanumeric for basic debugging - """ - value = 'feature2' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_tracking_underscore(self): - """Test re matches _ - """ - value = 'feature_2' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_tracking_hyphen(self): - """Test re matches - - """ - value = 'feature-2' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_tracking_period(self): - """Test re match . - """ - value = 'feature.2' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_tracking_slash(self): - """Test re matches / - """ - value = 'feature/2' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), value) - - def test_re_tracking_colon(self): - """Test re rejects names with colons because they are invalid for git - tag and branch names - - """ - value = 'feature:2' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNone(match) - - def test_re_tracking_ahead(self): - """Test re matches correctly with the ': ahead' syntax from git - """ - value = 'feature-2: ahead 3' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), 'feature-2') - - def test_re_tracking_ahead_behind(self): - """Test re matches correctly with the ': ahead 3, behind 2' syntax - from git - - """ - value = 'feature-2: ahead 3, behind 2' - input_str = self._tracking_tmpl.substitute(ref=value) - match = GitRepository.RE_TRACKING.search(input_str) - self.assertIsNotNone(match) - self.assertEqual(match.group(1), 'feature-2') - - class TestGitStatusPorcelain(unittest.TestCase): """Test parsing of output from git status --porcelain=v1 -z """ From dcf17b60fdcb5a17e947d455510e166a3b7b55f4 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Mon, 9 Apr 2018 09:15:19 -0600 Subject: [PATCH 6/8] make style cleanup --- manic/repository_git.py | 3 ++- test/test_sys_repository_git.py | 5 ++++- test/test_unit_repository_git.py | 21 ++++++++++++++------- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/manic/repository_git.py b/manic/repository_git.py index ab70802..5941356 100644 --- a/manic/repository_git.py +++ b/manic/repository_git.py @@ -197,7 +197,8 @@ def compare_refs(current_ref, expected_ref): stat.sync_state = ExternalStatus.UNKNOWN else: # get the underlying hash of the expected ref - revparse_status, expected_ref_hash = self._git_revparse_commit(expected_ref) + revparse_status, expected_ref_hash = self._git_revparse_commit( + expected_ref) if revparse_status: # We failed to get the hash associated with # expected_ref. Maybe we should assign this to some special diff --git a/test/test_sys_repository_git.py b/test/test_sys_repository_git.py index 395e5cb..baf2dca 100644 --- a/test/test_sys_repository_git.py +++ b/test/test_sys_repository_git.py @@ -22,6 +22,7 @@ from manic.externals_description import ExternalsDescriptionDict from manic.utils import execute_subprocess + class GitTestCase(unittest.TestCase): """Adds some git-specific unit test functionality on top of TestCase""" @@ -43,6 +44,7 @@ def assertIsHash(self, maybe_hash): self.assertTrue(set(maybe_hash) <= allowed_chars_set, msg="maybe_hash has non-hash characters: {}".format(maybe_hash)) + class TestGitTestCase(GitTestCase): """Tests GitTestCase""" @@ -61,6 +63,7 @@ def test_assertIsHash_badChar(self): with self.assertRaises(AssertionError): self.assertIsHash('abc123g') + class TestGitRepositoryGitCommands(GitTestCase): # ======================================================================== @@ -97,7 +100,6 @@ def setUp(self): repo = model[self._name][ExternalsDescription.REPO] self._repo = GitRepository('test', repo) - def tearDown(self): shutil.rmtree(self._tmpdir, ignore_errors=True) @@ -184,5 +186,6 @@ def test_currentTag_outsideGitRepo(self): self.assertFalse(tag_found) self.assertEqual('', mytag) + if __name__ == '__main__': unittest.main() diff --git a/test/test_unit_repository_git.py b/test/test_unit_repository_git.py index 525a315..78937e7 100644 --- a/test/test_unit_repository_git.py +++ b/test/test_unit_repository_git.py @@ -95,7 +95,8 @@ def my_git_current_hash(): def test_ref_branch(self): """Test that we correctly identify we are on a branch """ - self._repo._git_current_branch = self._git_current_branch(True, 'feature3') + self._repo._git_current_branch = self._git_current_branch( + True, 'feature3') self._repo._git_current_tag = self._git_current_tag(True, 'foo_tag') self._repo._git_current_hash = self._git_current_hash(True, 'abc123') expected = 'feature3' @@ -291,7 +292,8 @@ def test_sync_dir_exist_no_git_info(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' self._repo._git_current_hash = self._git_current_hash('') - self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 1, '') + self._repo._git_revparse_commit = self._git_revparse_commit( + 'tag1', 1, '') self._repo._check_sync(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.UNKNOWN) # check_sync should only modify the sync_state, not clean_state @@ -310,7 +312,8 @@ def test_sync_invalid_reference(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' self._repo._git_current_hash = self._git_current_hash('abc123') - self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 1, '') + self._repo._git_revparse_commit = self._git_revparse_commit( + 'tag1', 1, '') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state @@ -329,7 +332,8 @@ def test_sync_tag_on_same_hash(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' self._repo._git_current_hash = self._git_current_hash('abc123') - self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 0, 'abc123') + self._repo._git_revparse_commit = self._git_revparse_commit( + 'tag1', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) # check_sync should only modify the sync_state, not clean_state @@ -343,7 +347,8 @@ def test_sync_tag_on_different_hash(self): self._repo._git_remote_verbose = self._git_remote_origin_upstream self._repo._tag = 'tag1' self._repo._git_current_hash = self._git_current_hash('def456') - self._repo._git_revparse_commit = self._git_revparse_commit('tag1', 0, 'abc123') + self._repo._git_revparse_commit = self._git_revparse_commit( + 'tag1', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state @@ -363,7 +368,8 @@ def test_sync_hash_on_same_hash(self): self._repo._tag = '' self._repo._hash = 'abc' self._repo._git_current_hash = self._git_current_hash('abc123') - self._repo._git_revparse_commit = self._git_revparse_commit('abc', 0, 'abc123') + self._repo._git_revparse_commit = self._git_revparse_commit( + 'abc', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.STATUS_OK) # check_sync should only modify the sync_state, not clean_state @@ -378,7 +384,8 @@ def test_sync_hash_on_different_hash(self): self._repo._tag = '' self._repo._hash = 'abc' self._repo._git_current_hash = self._git_current_hash('def456') - self._repo._git_revparse_commit = self._git_revparse_commit('abc', 0, 'abc123') + self._repo._git_revparse_commit = self._git_revparse_commit( + 'abc', 0, 'abc123') self._repo._check_sync_logic(stat, self.TMP_FAKE_DIR) self.assertEqual(stat.sync_state, ExternalStatus.MODEL_MODIFIED) # check_sync should only modify the sync_state, not clean_state From a385070bbb8e2400f58239d5a058bff644323598 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Mon, 9 Apr 2018 10:51:09 -0600 Subject: [PATCH 7/8] fix pylint problems --- manic/repository_git.py | 1 - test/test_sys_repository_git.py | 62 ++++++++++++++++++++++++++------ test/test_unit_repository_git.py | 6 +++- 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/manic/repository_git.py b/manic/repository_git.py index 5941356..7e1c8b2 100644 --- a/manic/repository_git.py +++ b/manic/repository_git.py @@ -7,7 +7,6 @@ import copy import os -import re from .global_constants import EMPTY_STR, LOCAL_PATH_INDICATOR from .global_constants import VERBOSITY_VERBOSE diff --git a/test/test_sys_repository_git.py b/test/test_sys_repository_git.py index baf2dca..37039b8 100644 --- a/test/test_sys_repository_git.py +++ b/test/test_sys_repository_git.py @@ -22,6 +22,14 @@ from manic.externals_description import ExternalsDescriptionDict from manic.utils import execute_subprocess +# NOTE(wjs, 2018-04-09) I find a mix of camel case and underscores to be +# more readable for unit test names, so I'm disabling pylint's naming +# convention check +# pylint: disable=C0103 + +# Allow access to protected members +# pylint: disable=W0212 + class GitTestCase(unittest.TestCase): """Adds some git-specific unit test functionality on top of TestCase""" @@ -49,22 +57,40 @@ class TestGitTestCase(GitTestCase): """Tests GitTestCase""" def test_assertIsHash_true(self): + """Ensure that assertIsHash passes for something that looks + like a hash""" self.assertIsHash('abc123') def test_assertIsHash_empty(self): + """Ensure that assertIsHash raises an AssertionError for an + empty string""" with self.assertRaises(AssertionError): self.assertIsHash('') def test_assertIsHash_multipleStrings(self): + """Ensure that assertIsHash raises an AssertionError when + given multiple strings""" with self.assertRaises(AssertionError): self.assertIsHash('abc123 def456') def test_assertIsHash_badChar(self): + """Ensure that assertIsHash raises an AssertionError when given a + string that has a character that doesn't belong in a hash + """ with self.assertRaises(AssertionError): self.assertIsHash('abc123g') class TestGitRepositoryGitCommands(GitTestCase): + """Test some git commands in RepositoryGit + + It's silly that we need to create a repository in order to test + these git commands. Much or all of the git functionality that is + currently in repository_git.py should eventually be moved to a + separate module that is solely responsible for wrapping git + commands; that would allow us to test it independently of this + repository class. + """ # ======================================================================== # Test helper functions @@ -74,12 +100,6 @@ def setUp(self): self._tmpdir = tempfile.mkdtemp() os.chdir(self._tmpdir) - # It's silly that we need to create a repository in order to - # test these git commands. Much or all of the git functionality - # that is currently in repository_git.py should eventually be - # moved to a separate module that is solely responsible for - # wrapping git commands; that would allow us to test it - # independently of this repository class. self._name = 'component' rdata = {ExternalsDescription.PROTOCOL: 'git', ExternalsDescription.REPO_URL: @@ -103,26 +123,31 @@ def setUp(self): def tearDown(self): shutil.rmtree(self._tmpdir, ignore_errors=True) - def make_git_repo(self): + @staticmethod + def make_git_repo(): """Turn the current directory into an empty git repository""" execute_subprocess(['git', 'init']) - def add_git_commit(self): + @staticmethod + def add_git_commit(): """Add a git commit in the current directory""" with open('README', 'a') as myfile: myfile.write('more info') execute_subprocess(['git', 'add', 'README']) execute_subprocess(['git', 'commit', '-m', 'my commit message']) - def checkout_git_branch(self, branchname): + @staticmethod + def checkout_git_branch(branchname): """Checkout a new branch in the current directory""" execute_subprocess(['git', 'checkout', '-b', branchname]) - def make_git_tag(self, tagname): + @staticmethod + def make_git_tag(tagname): """Make a lightweight tag at the current commit""" execute_subprocess(['git', 'tag', '-m', 'making a tag', tagname]) - def checkout_ref(self, refname): + @staticmethod + def checkout_ref(refname): """Checkout the given refname in the current directory""" execute_subprocess(['git', 'checkout', refname]) @@ -131,6 +156,7 @@ def checkout_ref(self, refname): # ======================================================================== def test_currentHash_returnsHash(self): + """Ensure that the _git_current_hash function returns a hash""" self.make_git_repo() self.add_git_commit() hash_found, myhash = self._repo._git_current_hash() @@ -138,11 +164,15 @@ def test_currentHash_returnsHash(self): self.assertIsHash(myhash) def test_currentHash_outsideGitRepo(self): + """Ensure that the _git_current_hash function returns False when + outside a git repository""" hash_found, myhash = self._repo._git_current_hash() self.assertFalse(hash_found) self.assertEqual('', myhash) def test_currentBranch_onBranch(self): + """Ensure that the _git_current_branch function returns the name + of the branch""" self.make_git_repo() self.add_git_commit() self.checkout_git_branch('foo') @@ -151,6 +181,8 @@ def test_currentBranch_onBranch(self): self.assertEqual('foo', mybranch) def test_currentBranch_notOnBranch(self): + """Ensure that the _git_current_branch function returns False + when not on a branch""" self.make_git_repo() self.add_git_commit() self.make_git_tag('mytag') @@ -160,11 +192,15 @@ def test_currentBranch_notOnBranch(self): self.assertEqual('', mybranch) def test_currentBranch_outsideGitRepo(self): + """Ensure that the _git_current_branch function returns False + when outside a git repository""" branch_found, mybranch = self._repo._git_current_branch() self.assertFalse(branch_found) self.assertEqual('', mybranch) def test_currentTag_onTag(self): + """Ensure that the _git_current_tag function returns the name of + the tag""" self.make_git_repo() self.add_git_commit() self.make_git_tag('some_tag') @@ -173,6 +209,8 @@ def test_currentTag_onTag(self): self.assertEqual('some_tag', mytag) def test_currentTag_notOnTag(self): + """Ensure tha the _git_current_tag function returns False when + not on a tag""" self.make_git_repo() self.add_git_commit() self.make_git_tag('some_tag') @@ -182,6 +220,8 @@ def test_currentTag_notOnTag(self): self.assertEqual('', mytag) def test_currentTag_outsideGitRepo(self): + """Ensure that the _git_current_tag function returns False when + outside a git repository""" tag_found, mytag = self._repo._git_current_tag() self.assertFalse(tag_found) self.assertEqual('', mytag) diff --git a/test/test_unit_repository_git.py b/test/test_unit_repository_git.py index 78937e7..b025fbd 100644 --- a/test/test_unit_repository_git.py +++ b/test/test_unit_repository_git.py @@ -14,7 +14,6 @@ import os import shutil -import string import unittest from manic.repository_git import GitRepository @@ -69,6 +68,7 @@ def _git_current_branch(branch_found, branch_name): """Return a function that takes the place of repo._git_current_branch, which returns the given output.""" def my_git_current_branch(): + """mock function that can take the place of repo._git_current_branch""" return branch_found, branch_name return my_git_current_branch @@ -77,6 +77,7 @@ def _git_current_tag(tag_found, tag_name): """Return a function that takes the place of repo._git_current_tag, which returns the given output.""" def my_git_current_tag(): + """mock function that can take the place of repo._git_current_tag""" return tag_found, tag_name return my_git_current_tag @@ -85,6 +86,7 @@ def _git_current_hash(hash_found, hash_name): """Return a function that takes the place of repo._git_current_hash, which returns the given output.""" def my_git_current_hash(): + """mock function that can take the place of repo._git_current_hash""" return hash_found, hash_name return my_git_current_hash @@ -248,6 +250,7 @@ def _git_current_hash(myhash): which returns the given hash """ def my_git_current_hash(): + """mock function that can take the place of repo._git_current_hash""" return 0, myhash return my_git_current_hash @@ -261,6 +264,7 @@ def _git_revparse_commit(self, expected_ref, mystatus, myhash): status = 0 implies success, non-zero implies failure """ def my_git_revparse_commit(ref): + """mock function that can take the place of repo._git_revparse_commit""" self.assertEqual(expected_ref, ref) return mystatus, myhash return my_git_revparse_commit From d1de5f8f5e0a836b0c923fab48f1107057724d36 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Tue, 10 Apr 2018 08:32:21 -0600 Subject: [PATCH 8/8] Return to starting directory after each test Without this, future tests can fail --- test/test_sys_repository_git.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/test_sys_repository_git.py b/test/test_sys_repository_git.py index 37039b8..f6dbf84 100644 --- a/test/test_sys_repository_git.py +++ b/test/test_sys_repository_git.py @@ -97,6 +97,10 @@ class TestGitRepositoryGitCommands(GitTestCase): # ======================================================================== def setUp(self): + # directory we want to return to after the test system and + # checkout_externals are done cd'ing all over the place. + self._return_dir = os.getcwd() + self._tmpdir = tempfile.mkdtemp() os.chdir(self._tmpdir) @@ -121,6 +125,9 @@ def setUp(self): self._repo = GitRepository('test', repo) def tearDown(self): + # return to our common starting point + os.chdir(self._return_dir) + shutil.rmtree(self._tmpdir, ignore_errors=True) @staticmethod