git cl patch: handle missing/wrong Gerrit issue better.

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 119 matching lines @@
 
 class MockChangelistWithBranchAndIssue():
   def __init__(self, branch, issue):
     self.branch = branch
     self.issue = issue
   def GetBranch(self):
     return self.branch
   def GetIssue(self):
     return self.issue
 
+
+class SystemExitMock(Exception):
+  pass
+
+
 class TestGitClBasic(unittest.TestCase):
   def _test_ParseIssueUrl(self, func, url, issue, patchset, hostname, fail):
     parsed = urlparse.urlparse(url)
     result = func(parsed)
     if fail:
       self.assertIsNone(result)
       return None
     self.assertIsNotNone(result)
     self.assertEqual(result.issue, issue)
     self.assertEqual(result.patchset, patchset)
@@ skipping 114 matching lines @@
     self.mock(git_cl, 'write_json', lambda path, contents:
         self._mocked_call('write_json', path, contents))
     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))
+    self.mock(git_cl, 'DieWithError',
+              lambda msg: self._mocked_call(['DieWithError', msg]))
     # It's important to reset settings to not have inter-tests interference.
     git_cl.settings = None
 
 
   def tearDown(self):
     try:
       # Note: has_failed returns True if at least 1 test ran so far, current
       # included, has failed. That means current test may have actually ran
       # fine, and the check for no leftover calls would be skipped.
       if not self.has_failed():
@@ skipping 415 matching lines @@
     self._run_reviewer_test(
         ['--send-mail'],
         'desc\n\nBUG=',
         description.strip('\n'),
         description,
         ['--reviewers=reviewer@example.com', '--send_mail'])
 
   def test_reviewer_send_mail_no_rev(self):
     # Fails without a reviewer.
     stdout = StringIO.StringIO()
-    stderr = StringIO.StringIO()
-    try:
-      self.calls = self._upload_no_rev_calls(None, None)
-      def RunEditor(desc, _, **kwargs):
-        return desc
-      self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor)
-      self.mock(sys, 'stdout', stdout)
-      self.mock(sys, 'stderr', stderr)
+    self.calls = self._upload_no_rev_calls(None, None) + [
+        ((['DieWithError', 'Must specify reviewers to send email.'],),
+          SystemExitMock())
+    ]
+
+    def RunEditor(desc, _, **kwargs):
+      return desc
+    self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor)
+    self.mock(sys, 'stdout', stdout)
+    with self.assertRaises(SystemExitMock):
       git_cl.main(['upload', '--send-mail'])
-      self.fail()
-    except SystemExit:

[Sergiy Byelozyorov, 2016/10/10 11:41:12]

nice refactor

[tandrii(chromium), 2016/10/10 15:10:14]

:)

-      self.assertEqual(
-          'Using 50% similarity for rename/copy detection. Override with '
-          '--similarity.\n',
-          stdout.getvalue())
-      self.assertEqual(
-          'Must specify reviewers to send email.\n', stderr.getvalue())
+    self.assertEqual(
+        'Using 50% similarity for rename/copy detection. Override with '
+        '--similarity.\n',
+        stdout.getvalue())
 
   def test_bug_on_cmd(self):
     self._run_reviewer_test(
         ['--bug=500658,proj:123'],
         'desc\n\nBUG=500658\nBUG=proj:123',
         '# Blah blah comment.\ndesc\n\nBUG=500658\nBUG=proj:1234',
         'desc\n\nBUG=500658\nBUG=proj:1234',
         [])
 
   def test_dcommit(self):
@@ skipping 656 matching lines @@
        ''),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://chromium-review.googlesource.com'],), ''),
       ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
     ]
     self.assertEqual(git_cl.main(
       ['patch', 'https://chromium-review.googlesource.com/#/c/123456/1']), 0)
 
   def test_gerrit_patch_conflict(self):
     self._patch_common(is_gerrit=True)
-    self.mock(git_cl, 'DieWithError',
-              lambda msg: self._mocked_call(['DieWithError', msg]))
-    class SystemExitMock(Exception):
-      pass
     self.calls += [
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/1'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1),
       ((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],),
        SystemExitMock()),
     ]
     with self.assertRaises(SystemExitMock):
       git_cl.main(['patch',
                    'https://chromium-review.googlesource.com/#/c/123456/1'])
 
+  def test_gerrit_patch_not_exists(self):
+    url = 'https://chromium-review.googlesource.com'
+    self.mock(git_cl.gerrit_util, 'GetChangeDetail', lambda _, __, ___: None)
+    self.calls = [
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'config', 'branch.master.rietveldissue'],), CERR1),
+      ((['git', 'config', 'branch.master.gerritissue'],), CERR1),
+      ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
+      ((['git', 'config', 'gerrit.host'],), 'true'),
+      ((['DieWithError', 'issue 123456 at ' + url + ' does not exist '
+                         'or you have no access to it'],), SystemExitMock()),
+    ]
+    with self.assertRaises(SystemExitMock):
+      self.assertEqual(1, git_cl.main(['patch', url + '/#/c/123456/1']))
+
   def _checkout_calls(self):
     return [
         ((['git', 'config', '--local', '--get-regexp',
            'branch\\..*\\.rietveldissue'], ),
            ('branch.retrying.rietveldissue 1111111111\n'
             'branch.some-fix.rietveldissue 2222222222\n')),
         ((['git', 'config', '--local', '--get-regexp',
            'branch\\..*\\.gerritissue'], ),
            ('branch.ger-branch.gerritissue 123456\n'
             'branch.gbranch654.gerritissue 654321\n')),
@@ skipping 575 matching lines @@
     self.assertEqual(0, git_cl.main(['try-results']))
     self.assertRegexpMatches(sys.stdout.getvalue(), 'Failures:')
     self.assertRegexpMatches(sys.stdout.getvalue(), 'Started:')
     self.assertRegexpMatches(sys.stdout.getvalue(), '2 try jobs')
 
 
 if __name__ == '__main__':
   git_cl.logging.basicConfig(
       level=git_cl.logging.DEBUG if '-v' in sys.argv else git_cl.logging.ERROR)
   unittest.main()