git-cl: Mock GetChangeDetail on tests.

Mock GetChangeDetail instead of using self.calls to make the code
easier to change.

Bug: 1051631
Change-Id: I77361caccaf694644839825d890343864267688f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2062716
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Anthony Polito <apolito@google.com>

git_cl.py

@@ -1420,8 +1420,6 @@ class Changelist(object):
     self.description = description
     self.has_description = True
 
-
-
   def RunHook(self, committing, may_prompt, verbose, change, parallel):
     """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
     try:
@@ -1693,11 +1691,6 @@ class Changelist(object):
     if not self.GetIssue():
       return
 
-    # Warm change details cache now to avoid RPCs later, reducing latency for
-    # developers.
-    self._GetChangeDetail(
-        ['DETAILED_ACCOUNTS', 'CURRENT_REVISION', 'CURRENT_COMMIT', 'LABELS'])
-
     status = self._GetChangeDetail()['status']
     if status in ('MERGED', 'ABANDONED'):
       DieWithError('Change %s has been %s, new uploads are not allowed' %
@@ -1912,29 +1905,19 @@ class Changelist(object):
         self._GetGerritHost(), self._GerritChangeIdentifier(),
         wait_for_merge=wait_for_merge)
 
-  def _GetChangeDetail(self, options=None, no_cache=False):
-    """Returns details of associated Gerrit change and caching results.
-
-    If fresh data is needed, set no_cache=True which will clear cache and
-    thus new data will be fetched from Gerrit.
-    """
+  def _GetChangeDetail(self, options=None):
+    """Returns details of associated Gerrit change and caching results."""
     options = options or []
     assert self.GetIssue(), 'issue is required to query Gerrit'
 
     # Optimization to avoid multiple RPCs:
-    if (('CURRENT_REVISION' in options or 'ALL_REVISIONS' in options) and
-        'CURRENT_COMMIT' not in options):
+    if 'CURRENT_REVISION' in options or 'ALL_REVISIONS' in options:
       options.append('CURRENT_COMMIT')
 
     # Normalize issue and options for consistent keys in cache.
     cache_key = str(self.GetIssue())
-    options = [o.upper() for o in options]
+    options_set = frozenset(o.upper() for o in options)
 
-    # Check in cache first unless no_cache is True.
-    if no_cache:
-      self._detail_cache.pop(cache_key, None)
-    else:
-      options_set = frozenset(options)
     for cached_options_set, data in self._detail_cache.get(cache_key, []):
       # Assumption: data fetched before with extra options is suitable
       # for return for a smaller set of options.
@@ -1947,14 +1930,13 @@ class Changelist(object):
 
     try:
       data = gerrit_util.GetChangeDetail(
-          self._GetGerritHost(), self._GerritChangeIdentifier(), options)
+          self._GetGerritHost(), self._GerritChangeIdentifier(), options_set)
     except gerrit_util.GerritError as e:
       if e.http_status == 404:
         raise GerritChangeNotExists(self.GetIssue(), self.GetCodereviewServer())
       raise
 
-    self._detail_cache.setdefault(cache_key, []).append(
-        (frozenset(options), data))
+    self._detail_cache.setdefault(cache_key, []).append((options_set, data))
     return data
 
   def _GetChangeCommit(self):
@@ -4363,9 +4345,16 @@ def CMDupload(parser, args):
   settings.GetIsGerrit()
 
   cl = Changelist()
+  # Warm change details cache now to avoid RPCs later, reducing latency for
+  # developers.
+  if cl.GetIssue():
+    cl._GetChangeDetail(
+        ['DETAILED_ACCOUNTS', 'CURRENT_REVISION', 'CURRENT_COMMIT', 'LABELS'])
+
   if options.retry_failed and not cl.GetIssue():
     print('No previous patchsets, so --retry-failed has no effect.')
     options.retry_failed = False
+
   # cl.GetMostRecentPatchset uses cached information, and can return the last
   # patchset before upload. Calling it here makes it clear that it's the
   # last patchset before upload. Note that GetMostRecentPatchset will fail

tests/git_cl_test.py

@@ -629,9 +629,7 @@ class TestGitCl(unittest.TestCase):
         'git_cl.presubmit_support.DoPresubmitChecks', PresubmitMock).start()
     mock.patch('git_cl.watchlists.Watchlists', WatchlistsMock).start()
     mock.patch('git_cl.auth.Authenticator', AuthenticatorMock).start()
-    mock.patch(
-        'git_cl.gerrit_util.GetChangeDetail',
-        lambda *a: self._mocked_call('GetChangeDetail', *a)).start()
+    mock.patch('gerrit_util.GetChangeDetail').start()
     mock.patch(
         'git_cl.gerrit_util.GetChangeComments',
         lambda *a: self._mocked_call('GetChangeComments', *a)).start()
@@ -796,12 +794,7 @@ class TestGitCl(unittest.TestCase):
       ]
 
     if issue:
-      calls += [
-        (('GetChangeDetail', '%s-review.googlesource.com' % short_hostname,
-          'my%2Frepo~123456',
-          ['DETAILED_ACCOUNTS', 'CURRENT_REVISION', 'CURRENT_COMMIT', 'LABELS']
-         ),
-         {
+      gerrit_util.GetChangeDetail.return_value = {
         'owner': {'email': (other_cl_owner or 'owner@example.com')},
         'change_id': (change_id or '123456789'),
         'current_revision': 'sha1_of_current_revision',
@@ -809,8 +802,7 @@ class TestGitCl(unittest.TestCase):
           'commit': {'message': fetched_description},
         }},
         'status': fetched_status or 'NEW',
-         }),
-      ]
+      }
       if fetched_status == 'ABANDONED':
         calls += [
           (('DieWithError', 'Change https://%s-review.googlesource.com/'
@@ -1714,11 +1706,7 @@ class TestGitCl(unittest.TestCase):
     mock.patch('git_cl.IsGitVersionAtLeast', return_value=True).start()
     self.mockGit.config['remote.origin.url'] = (
         'https://%s.googlesource.com/my/repo' % git_short_host)
-
-    self.calls += [
-      (('GetChangeDetail', git_short_host + '-review.googlesource.com',
-        'my%2Frepo~123456', ['ALL_REVISIONS', 'CURRENT_COMMIT']),
-       {
+    gerrit_util.GetChangeDetail.return_value = {
       'current_revision': '7777777777',
       'revisions': {
         '1111111111': {
@@ -1736,8 +1724,7 @@ class TestGitCl(unittest.TestCase):
           }},
         },
       },
-       }),
-    ]
+    }
 
   def test_patch_gerrit_default(self):
     self._patch_common()
@@ -1810,7 +1797,7 @@ class TestGitCl(unittest.TestCase):
       git_cl.main(['patch', '123456'])
 
   @mock.patch(
-      'git_cl.gerrit_util.GetChangeDetail',
+      'gerrit_util.GetChangeDetail',
       side_effect=gerrit_util.GerritError(404, ''))
   def test_patch_gerrit_not_exists(self, *_mocks):
     self.mockGit.config['remote.origin.url'] = (
@@ -2018,16 +2005,12 @@ class TestGitCl(unittest.TestCase):
   def test_description(self):
     self.mockGit.config['remote.origin.url'] = (
         'https://chromium.googlesource.com/my/repo')
-    self.calls = [
-        (('GetChangeDetail', 'chromium-review.googlesource.com',
-          'my%2Frepo~123123', ['CURRENT_REVISION', 'CURRENT_COMMIT']),
-         {
+    gerrit_util.GetChangeDetail.return_value = {
       'current_revision': 'sha1',
       'revisions': {'sha1': {
         'commit': {'message': 'foobar'},
       }},
-         }),
-    ]
+    }
     self.assertEqual(0, git_cl.main([
         'description',
         'https://chromium-review.googlesource.com/c/my/repo/+/123123',
@@ -2311,11 +2294,7 @@ class TestGitCl(unittest.TestCase):
 
   def test_gerrit_change_detail_cache_simple(self):
     self._mock_gerrit_changes_for_detail_cache()
-    self.calls = [
-        (('GetChangeDetail', 'host', 'my%2Frepo~1', []), 'a'),
-        (('GetChangeDetail', 'host', 'ab%2Frepo~2', []), 'b'),
-        (('GetChangeDetail', 'host', 'ab%2Frepo~2', []), 'b2'),
-    ]
+    gerrit_util.GetChangeDetail.side_effect = ['a', 'b']
     cl1 = git_cl.Changelist(issue=1)
     cl1._cached_remote_url = (
         True, 'https://chromium.googlesource.com/a/my/repo.git/')
@@ -2325,19 +2304,10 @@ class TestGitCl(unittest.TestCase):
     self.assertEqual(cl1._GetChangeDetail(), 'a')  # Miss.
     self.assertEqual(cl1._GetChangeDetail(), 'a')
     self.assertEqual(cl2._GetChangeDetail(), 'b')  # Miss.
-    self.assertEqual(cl2._GetChangeDetail(no_cache=True), 'b2')  # Miss.
-    self.assertEqual(cl1._GetChangeDetail(), 'a')
-    self.assertEqual(cl2._GetChangeDetail(), 'b2')
 
   def test_gerrit_change_detail_cache_options(self):
     self._mock_gerrit_changes_for_detail_cache()
-    self.calls = [
-        (('GetChangeDetail', 'host', 'repo~1', ['C', 'A', 'B']), 'cab'),
-        (('GetChangeDetail', 'host', 'repo~1', ['A', 'D']), 'ad'),
-        (('GetChangeDetail', 'host', 'repo~1', ['A']), 'a'),  # no_cache=True
-        # no longer in cache.
-        (('GetChangeDetail', 'host', 'repo~1', ['B']), 'b'),
-    ]
+    gerrit_util.GetChangeDetail.side_effect = ['cab', 'ad']
     cl = git_cl.Changelist(issue=1)
     cl._cached_remote_url = (True, 'https://chromium.googlesource.com/repo/')
     self.assertEqual(cl._GetChangeDetail(options=['C', 'A', 'B']), 'cab')
@@ -2352,21 +2322,13 @@ class TestGitCl(unittest.TestCase):
     self.assertEqual(cl._GetChangeDetail(options=['D']), 'ad')
     self.assertEqual(cl._GetChangeDetail(), 'cab')
 
-    # Finally, no_cache should invalidate all caches for given change.
-    self.assertEqual(cl._GetChangeDetail(options=['A'], no_cache=True), 'a')
-    self.assertEqual(cl._GetChangeDetail(options=['B']), 'b')
-
   def test_gerrit_description_caching(self):
-    def gen_detail(rev, desc):
-      return {
-        'current_revision': rev,
-        'revisions': {rev: {'commit': {'message': desc}}}
+    gerrit_util.GetChangeDetail.return_value = {
+      'current_revision': 'rev1',
+      'revisions': {
+        'rev1': {'commit': {'message': 'desc1'}},
+      },
     }
-    self.calls = [
-        (('GetChangeDetail', 'host', 'my%2Frepo~1',
-          ['CURRENT_REVISION', 'CURRENT_COMMIT']),
-         gen_detail('rev1', 'desc1')),
-    ]
 
     self._mock_gerrit_changes_for_detail_cache()
     cl = git_cl.Changelist(issue=1)
@@ -2477,11 +2439,7 @@ class TestGitCl(unittest.TestCase):
   def test_git_cl_comments_fetch_gerrit(self, *_mocks):
     self.mockGit.config['remote.origin.url'] = (
         'https://chromium.googlesource.com/infra/infra')
-    self.calls = [
-      (('GetChangeDetail', 'chromium-review.googlesource.com',
-        'infra%2Finfra~1',
-        ['MESSAGES', 'DETAILED_ACCOUNTS', 'CURRENT_REVISION',
-         'CURRENT_COMMIT']), {
+    gerrit_util.GetChangeDetail.return_value = {
         'owner': {'email': 'owner@example.com'},
         'current_revision': 'ba5eba11',
         'revisions': {
@@ -2528,7 +2486,8 @@ class TestGitCl(unittest.TestCase):
              u'message': u'Patch Set 2: Code-Review+1',
           },
         ]
-      }),
+      }
+    self.calls = [
       (('GetChangeComments', 'chromium-review.googlesource.com',
         'infra%2Finfra~1'), {
         '/COMMIT_MSG': [
@@ -2620,11 +2579,7 @@ class TestGitCl(unittest.TestCase):
     # comments from the latest patchset are shown.
     self.mockGit.config['remote.origin.url'] = (
         'https://chromium.googlesource.com/infra/infra')
-    self.calls = [
-      (('GetChangeDetail', 'chromium-review.googlesource.com',
-        'infra%2Finfra~1',
-        ['MESSAGES', 'DETAILED_ACCOUNTS', 'CURRENT_REVISION',
-         'CURRENT_COMMIT']), {
+    gerrit_util.GetChangeDetail.return_value = {
       'owner': {'email': 'owner@example.com'},
       'current_revision': 'ba5eba11',
       'revisions': {
@@ -2685,7 +2640,8 @@ class TestGitCl(unittest.TestCase):
         u'message': u'(1 comment)',
       },
       ]
-      }),
+    }
+    self.calls = [
       (('GetChangeComments', 'chromium-review.googlesource.com',
         'infra%2Finfra~1'), {}),
       (('GetChangeRobotComments', 'chromium-review.googlesource.com',
@@ -2862,15 +2818,26 @@ class CMDTestCaseBase(unittest.TestCase):
     mock.patch('git_cl.sys.stdout', StringIO()).start()
     mock.patch('git_cl.uuid.uuid4', return_value='uuid4').start()
     mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start()
-    mock.patch('git_cl.Changelist.GetCodereviewServer',
+    mock.patch(
+        'git_cl.Changelist.GetCodereviewServer',
         return_value='https://chromium-review.googlesource.com').start()
-    mock.patch('git_cl.Changelist.GetMostRecentPatchset',
+    mock.patch(
+        'git_cl.Changelist._GetGerritHost',
+        return_value='chromium-review.googlesource.com').start()
+    mock.patch(
+        'git_cl.Changelist.GetMostRecentPatchset',
         return_value=7).start()
-    mock.patch('git_cl.auth.Authenticator',
+    mock.patch(
+        'git_cl.Changelist.GetRemoteUrl',
+        return_value='https://chromium.googlesource.com/depot_tools').start()
+    mock.patch(
+        'auth.Authenticator',
         return_value=AuthenticatorMock()).start()
-    mock.patch('git_cl.Changelist._GetChangeDetail',
+    mock.patch(
+        'gerrit_util.GetChangeDetail',
         return_value=self._CHANGE_DETAIL).start()
-    mock.patch('git_cl._call_buildbucket',
+    mock.patch(
+        'git_cl._call_buildbucket',
         return_value = self._DEFAULT_RESPONSE).start()
     mock.patch('git_common.is_dirty_git_tree', return_value=False).start()
     self.addCleanup(mock.patch.stopall)
@@ -3240,6 +3207,14 @@ class CMDUploadTestCase(CMDTestCaseBase):
     mock.patch('git_cl.Settings.GetIsGerrit', return_value=True).start()
     self.addCleanup(mock.patch.stopall)
 
+  def testWarmUpChangeDetailCache(self):
+    self.assertEqual(0, git_cl.main(['upload']))
+    gerrit_util.GetChangeDetail.assert_called_once_with(
+        'chromium-review.googlesource.com', 'depot_tools~123456',
+        frozenset([
+            'LABELS', 'CURRENT_REVISION', 'DETAILED_ACCOUNTS',
+            'CURRENT_COMMIT']))
+
   def testUploadRetryFailed(self):
     # This test mocks out the actual upload part, and just asserts that after
     # upload, if --retry-failed is added, then the tool will fetch try jobs