From c2405f5545288efe2cad1293ac106b189bbbed5f Mon Sep 17 00:00:00 2001 From: tandrii Date: Mon, 10 Oct 2016 08:13:15 -0700 Subject: [PATCH] git cl patch: handle missing/wrong Gerrit issue better. No stacktrace is printed, instead just a user-friendly string is shown. Add test + some tests refactoring. This CL also improve missing Gerrit issue exception elsewhere by raising user-friendly exception. BUG=654360 R=sergiyb@chromium.org TEST=manual Review-Url: https://codereview.chromium.org/2403793002 --- git_cl.py | 29 +++++++++++++++++----- tests/git_cl_test.py | 57 ++++++++++++++++++++++++++++---------------- 2 files changed, 60 insertions(+), 26 deletions(-) diff --git a/git_cl.py b/git_cl.py index ac9a17b66..381d8dbdc 100755 --- a/git_cl.py +++ b/git_cl.py @@ -977,6 +977,17 @@ def ParseIssueNumberArgument(arg): return fail_result +class GerritIssueNotExists(Exception): + def __init__(self, issue, url): + self.issue = issue + self.url = url + super(GerritIssueNotExists, self).__init__() + + def __str__(self): + return 'issue %s at %s does not exist or you have no access to it' % ( + self.issue, self.url) + + class Changelist(object): """Changelist works with one changelist in local branch. @@ -2258,8 +2269,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): * 'unsent' - no reviewers added * 'waiting' - waiting for review * 'reply' - waiting for owner to reply to review - * 'not lgtm' - Code-Review -2 from at least one approved reviewer - * 'lgtm' - Code-Review +2 from at least one approved reviewer + * 'not lgtm' - Code-Review disaproval from at least one valid reviewer + * 'lgtm' - Code-Review approval from at least one valid reviewer * 'commit' - in the commit queue * 'closed' - abandoned """ @@ -2268,7 +2279,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): try: data = self._GetChangeDetail(['DETAILED_LABELS', 'CURRENT_REVISION']) - except httplib.HTTPException: + except (httplib.HTTPException, GerritIssueNotExists): return 'error' if data['status'] in ('ABANDONED', 'MERGED'): @@ -2343,9 +2354,12 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): def _GetChangeDetail(self, options=None, issue=None): options = options or [] issue = issue or self.GetIssue() - assert issue, 'issue required to query Gerrit' - return gerrit_util.GetChangeDetail(self._GetGerritHost(), str(issue), + assert issue, 'issue is required to query Gerrit' + data = gerrit_util.GetChangeDetail(self._GetGerritHost(), str(issue), options) + if not data: + raise GerritIssueNotExists(issue, self.GetCodereviewServer()) + return data def CMDLand(self, force, bypass_hooks, verbose): if git_common.is_dirty_git_tree('land'): @@ -2400,7 +2414,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): self._gerrit_host = parsed_issue_arg.hostname self._gerrit_server = 'https://%s' % self._gerrit_host - detail = self._GetChangeDetail(['ALL_REVISIONS']) + try: + detail = self._GetChangeDetail(['ALL_REVISIONS']) + except GerritIssueNotExists as e: + DieWithError(str(e)) if not parsed_issue_arg.patchset: # Use current revision by default. diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 987b478a0..b4c149fb9 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -137,6 +137,11 @@ class MockChangelistWithBranchAndIssue(): 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) @@ -271,6 +276,8 @@ class TestGitCl(TestCase): 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 @@ -706,23 +713,21 @@ class TestGitCl(TestCase): 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: - 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( @@ -1399,10 +1404,6 @@ class TestGitCl(TestCase): 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'],), ''), @@ -1414,6 +1415,22 @@ class TestGitCl(TestCase): 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',