Gerrit git cl upload: warn and offer to remove Gerrit commit-msg hook.

tests/git_cl_test.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Unit tests for git_cl.py."""
 
 import os
 import StringIO
 import stat
@@ skipping 207 matching lines @@
     self.mock(subprocess2, 'check_call', self._mocked_call)
     self.mock(subprocess2, 'check_output', self._mocked_call)
     self.mock(subprocess2, 'communicate', self._mocked_call)
     self.mock(git_cl.gclient_utils, 'CheckCallAndFilter', self._mocked_call)
     self.mock(git_common, 'is_dirty_git_tree', lambda x: False)
     self.mock(git_common, 'get_or_create_merge_base',
               lambda *a: (
                   self._mocked_call(['get_or_create_merge_base']+list(a))))
     self.mock(git_cl, 'BranchExists', lambda _: True)
     self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: '')
     self.mock(git_cl, 'ask_for_data', self._mocked_call)

[tandrii(chromium), 2016/06/07 16:40:51]

see here - _mocked_call doesn't get the name of "ask_for_data", which in case of
ask for data is not bad, imo.

[Sergiy Byelozyorov, 2016/06/07 17:29:11]

Acknowledged.

     self.mock(git_cl.presubmit_support, 'DoPresubmitChecks', PresubmitMock)
     self.mock(git_cl.rietveld, 'Rietveld', RietveldMock)
     self.mock(git_cl.rietveld, 'CachingRietveld', RietveldMock)
     self.mock(git_cl.upload, 'RealMain', self.fail)
     self.mock(git_cl.watchlists, 'Watchlists', WatchlistsMock)
     self.mock(git_cl.auth, 'get_authenticator_for_host', AuthenticatorMock)
     self.mock(git_cl.gerrit_util.GceAuthenticator, 'is_gce',
               classmethod(lambda _: False))
     # It's important to reset settings to not have inter-tests interference.
     git_cl.settings = None
@@ skipping 632 matching lines @@
       reviewers=None,
       squash=False,
       expected_upstream_ref='origin/refs/heads/master',
       ref_suffix='',
       notify=False,
       post_amend_description=None,
       issue=None):
     """Generic gerrit upload test framework."""
     reviewers = reviewers or []
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
-    self.mock(git_cl.gerrit_util, "CookiesAuthenticator",
+    self.mock(git_cl.gerrit_util, 'CookiesAuthenticator',
               CookiesAuthenticatorMockFactory(same_cookie='same_cred'))
+    self.mock(git_cl._GerritChangelistImpl, '_GerritCommitMsgHookCheck',
+              lambda _, offer_removal: '')

[Sergiy Byelozyorov, 2016/06/07 16:33:21]

not that it matters, but why return '' and not None?

[tandrii(chromium), 2016/06/07 16:40:51]

Typo, though '' is shorter than None.

[Sergiy Byelozyorov, 2016/06/07 17:29:11]

Acknowledged.

     self.calls = self._gerrit_base_calls(issue=issue)
     self.calls += self._gerrit_upload_calls(
         description, reviewers, squash,
         expected_upstream_ref=expected_upstream_ref,
         ref_suffix=ref_suffix, notify=notify,
         post_amend_description=post_amend_description,
         issue=issue)
     # Uncomment when debugging.
     # print '\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls)))
     git_cl.main(['upload'] + upload_args)
@@ skipping 624 matching lines @@
         ((['git', 'config', '--unset', 'branch.feature.gerritpatchset'],), ''),
         # Let this command raise exception (retcode=1) - it should be ignored.
         ((['git', 'config', '--unset', 'branch.feature.last-upload-hash'],),
          '', subprocess2.CalledProcessError(1, '', '', '', '')),
         ((['git', 'config', '--unset', 'branch.feature.gerritserver'],), ''),
         ((['git', 'config', '--unset', 'branch.feature.gerritsquashhash'],),
          ''),
     ]
     self.assertEqual(0, git_cl.main(['issue', '0']))
 
+  def _common_GerritCommitMsgHookCheck(self):
+    self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
+    self.mock(git_cl.os.path, 'abspath',
+              lambda path: self._mocked_call(['abspath', path]))

[Sergiy Byelozyorov, 2016/06/07 16:33:21]

where is path defined? also didn't quite figure out how self._mocked_call works
here... but it's used like that in other places, so i guess it's fine

[tandrii(chromium), 2016/06/07 16:40:51]

it basically pops next item from self.calls, ensures that func args match what
has been popped, and returns whatever popped items contains as result.

[Sergiy Byelozyorov, 2016/06/07 17:29:11]

Oh. Nevermind about path. It's lambda argument.
Ack.

+    self.mock(git_cl.os.path, 'exists',
+              lambda path: self._mocked_call(['exists', path]))
+    self.mock(git_cl.gclient_utils, 'FileRead',
+              lambda path: self._mocked_call(['FileRead', path]))
+    self.mock(git_cl.gclient_utils, 'rm_file_or_tree',
+              lambda path: self._mocked_call(['rm_file_or_tree', path]))
+    self.calls = [
+        ((['git', 'rev-parse', '--show-cdup'],), '../'),
+        ((['abspath', '../'],), '/abs/git_repo_root'),
+    ]
+    return git_cl.Changelist(codereview='gerrit', issue=123)
+
+  def test_GerritCommitMsgHookCheck_custom_hook(self):
+    cl = self._common_GerritCommitMsgHookCheck()
+    self.calls += [
+        ((['exists', '/abs/git_repo_root/.git/hooks/commit-msg'],), True),
+        ((['FileRead', '/abs/git_repo_root/.git/hooks/commit-msg'],),
+         '#!/bin/sh\necho "custom hook"')
+    ]
+    cl._codereview_impl._GerritCommitMsgHookCheck(offer_removal=True)
+
+  def test_GerritCommitMsgHookCheck_not_exists(self):
+    cl = self._common_GerritCommitMsgHookCheck()
+    self.calls += [
+        ((['exists', '/abs/git_repo_root/.git/hooks/commit-msg'],), False),
+    ]
+    cl._codereview_impl._GerritCommitMsgHookCheck(offer_removal=True)
+
+  def test_GerritCommitMsgHookCheck(self):
+    cl = self._common_GerritCommitMsgHookCheck()
+    self.calls += [
+        ((['exists', '/abs/git_repo_root/.git/hooks/commit-msg'],), True),
+        ((['FileRead', '/abs/git_repo_root/.git/hooks/commit-msg'],),
+         '...\n# From Gerrit Code Review\n...\nadd_ChangeId()\n'),
+        (('Do you want to remove it now? [Yes/No]',), 'Yes'),

[Sergiy Byelozyorov, 2016/06/07 16:33:22]

is there a function name missing here? ask_for_data?

[tandrii(chromium), 2016/06/07 16:40:51]

yeah, that's how ask_for_data is mocked for this whole test class.

[Sergiy Byelozyorov, 2016/06/07 17:29:11]

Highly intuitive... :-)

+        ((['rm_file_or_tree', '/abs/git_repo_root/.git/hooks/commit-msg'],),
+         ''),
+    ]
+    cl._codereview_impl._GerritCommitMsgHookCheck(offer_removal=True)
+
 
 if __name__ == '__main__':
   git_cl.logging.basicConfig(
       level=git_cl.logging.DEBUG if '-v' in sys.argv else git_cl.logging.ERROR)
   unittest.main()