Create CachingRietveld to automatically cache results for presubmit checks.

Multiple presubmit checks may call the same function multiple times, so it's
worth caching the results to speed up the presubmit check run.

Convert presubmit_support, git-cl and gcl to use it.

R=dpranke@chromium.org
BUG=


Review URL: https://chromiumcodereview.appspot.com/11280143

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@169726 0039d316-1c4b-4281-b951-d872f2087c98

gcl.py

@@ -363,7 +363,7 @@ class ChangeInfo(object):
     if not self._rpc_server:
       if not self.rietveld:
         ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND)
-      self._rpc_server = rietveld.Rietveld(self.rietveld, None, None)
+      self._rpc_server = rietveld.CachingRietveld(self.rietveld, None, None)
     return self._rpc_server
 
   def CloseIssue(self):

git_cl.py

@@ -742,8 +742,8 @@ or verify this branch is set up to track another (via the --track argument to
     """Returns an upload.RpcServer() to access this review's rietveld instance.
     """
     if not self._rpc_server:
-      self._rpc_server = rietveld.Rietveld(self.GetRietveldServer(),
-                                           None, None)
+      self._rpc_server = rietveld.CachingRietveld(
+          self.GetRietveldServer(), None, None)
     return self._rpc_server
 
   def _IssueSetting(self):

presubmit_support.py

@@ -1237,7 +1237,7 @@ def Main(argv):
   logging.info('Found %d file(s).' % len(files))
   rietveld_obj = None
   if options.rietveld_url:
-    rietveld_obj = rietveld.Rietveld(
+    rietveld_obj = rietveld.CachingRietveld(
         options.rietveld_url,
         options.rietveld_email,
         options.rietveld_password)

rietveld.py

@@ -14,6 +14,7 @@ The following hypothesis are made:
   - A patch set cannot be modified
 """
 
+import copy
 import json
 import logging
 import re
@@ -395,3 +396,53 @@ class Rietveld(object):
 
   # DEPRECATED.
   Send = get
+
+
+class CachingRietveld(Rietveld):
+  """Caches the common queries.
+
+  Not to be used in long-standing processes, like the commit queue.
+  """
+  def __init__(self, *args, **kwargs):
+    super(CachingRietveld, self).__init__(*args, **kwargs)
+    self._cache = {}
+
+  def _lookup(self, function_name, args, update):
+    """Caches the return values corresponding to the arguments.
+
+    It is important that the arguments are standardized, like None vs False.
+    """
+    function_cache = self._cache.setdefault(function_name, {})
+    if args not in function_cache:
+      function_cache[args] = update(*args)
+    return copy.deepcopy(function_cache[args])
+
+  def get_description(self, issue):
+    return self._lookup(
+        'get_description',
+        (issue,),
+        super(CachingRietveld, self).get_description)
+
+  def get_issue_properties(self, issue, messages):
+    """Returns the issue properties.
+
+    Because in practice the presubmit checks often ask without messages first
+    and then with messages, always ask with messages and strip off if not asked
+    for the messages.
+    """
+    # It's a tad slower to request with the message but it's better than
+    # requesting the properties twice.
+    data = self._lookup(
+        'get_issue_properties',
+        (issue, True),
+        super(CachingRietveld, self).get_issue_properties)
+    if not messages:
+      # Assumes self._lookup uses deepcopy.
+      del data['messages']
+    return data
+
+  def get_patchset_properties(self, issue, patchset):
+    return self._lookup(
+        'get_patchset_properties',
+        (issue, patchset),
+        super(CachingRietveld, self).get_patchset_properties)

tests/git_cl_test.py

@@ -69,6 +69,7 @@ class TestGitCl(TestCase):
     self.mock(git_cl.breakpad, 'SendStack', self._mocked_call)
     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)
     # It's important to reset settings to not have inter-tests interference.

tests/rietveld_test.py

@@ -35,32 +35,29 @@ def _file(
   }
 
 
-class RietveldTest(unittest.TestCase):
+class BaseFixture(unittest.TestCase):
+  # Override.
+  TESTED_CLASS = Exception
+
   def setUp(self):
-    super(RietveldTest, self).setUp()
+    super(BaseFixture, self).setUp()
     # Access to a protected member XX of a client class
     # pylint: disable=W0212
-    self.rietveld = rietveld.Rietveld('url', 'email', 'password')
+    self.rietveld = self.TESTED_CLASS('url', 'email', 'password')
     self.rietveld._send = self._rietveld_send
     self.requests = []
 
   def tearDown(self):
-    self.assertEquals([], self.requests)
-    super(RietveldTest, self).tearDown()
+    self.assertEqual([], self.requests)
+    super(BaseFixture, self).tearDown()
 
   def _rietveld_send(self, url, *args, **kwargs):
     self.assertTrue(self.requests, url)
     request = self.requests.pop(0)
-    self.assertEquals(2, len(request))
-    self.assertEquals(url, request[0])
+    self.assertEqual(2, len(request))
+    self.assertEqual(url, request[0])
     return request[1]
 
-  def test_get_patch_empty(self):
-    self.requests = [('/api/123/456', '{}')]
-    patches = self.rietveld.get_patch(123, 456)
-    self.assertTrue(isinstance(patches, patch.PatchSet))
-    self.assertEquals([], patches.patches)
-
   def _check_patch(self,
       p,
       filename,
@@ -73,19 +70,29 @@ class RietveldTest(unittest.TestCase):
       patchlevel=0,
       svn_properties=None):
     svn_properties = svn_properties or []
-    self.assertEquals(p.filename, filename)
-    self.assertEquals(p.source_filename, source_filename)
-    self.assertEquals(p.is_binary, is_binary)
-    self.assertEquals(p.is_delete, is_delete)
+    self.assertEqual(p.filename, filename)
+    self.assertEqual(p.source_filename, source_filename)
+    self.assertEqual(p.is_binary, is_binary)
+    self.assertEqual(p.is_delete, is_delete)
     if hasattr(p, 'is_git_diff'):
-      self.assertEquals(p.is_git_diff, is_git_diff)
-    self.assertEquals(p.is_new, is_new)
+      self.assertEqual(p.is_git_diff, is_git_diff)
+    self.assertEqual(p.is_new, is_new)
     if hasattr(p, 'patchlevel'):
-      self.assertEquals(p.patchlevel, patchlevel)
+      self.assertEqual(p.patchlevel, patchlevel)
     if diff:
-      self.assertEquals(p.get(True), diff)
+      self.assertEqual(p.get(True), diff)
     if hasattr(p, 'svn_properties'):
-      self.assertEquals(p.svn_properties, svn_properties)
+      self.assertEqual(p.svn_properties, svn_properties)
+
+
+class RietveldTest(BaseFixture):
+  TESTED_CLASS = rietveld.Rietveld
+
+  def test_get_patch_empty(self):
+    self.requests = [('/api/123/456', '{}')]
+    patches = self.rietveld.get_patch(123, 456)
+    self.assertTrue(isinstance(patches, patch.PatchSet))
+    self.assertEqual([], patches.patches)
 
   def test_get_patch_no_status(self):
     self.requests = [
@@ -99,7 +106,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', RAW.DELETE),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(
         patches.patches[0],
         'tools/clang_check/README.chromium',
@@ -114,7 +121,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_790.diff', RAW.NEW_NOT_NULL),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(2, len(patches.patches))
+    self.assertEqual(2, len(patches.patches))
     self._check_patch(
         patches.patches[0], 'file_a', RAW.NEW_NOT_NULL, is_new=True)
     self._check_patch(patches.patches[1], 'foo', RAW.NEW, is_new=True)
@@ -125,7 +132,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', RAW.NEW),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(patches.patches[0], 'foo', RAW.NEW, is_new=True)
 
   def test_invalid_status(self):
@@ -136,7 +143,7 @@ class RietveldTest(unittest.TestCase):
       self.rietveld.get_patch(123, 456)
       self.fail()
     except patch.UnsupportedPatchFormat, e:
-      self.assertEquals('file_a', e.filename)
+      self.assertEqual('file_a', e.filename)
 
   def test_add_plus_merge(self):
     # svn:mergeinfo is dropped.
@@ -149,7 +156,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', GIT.COPY),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(
         patches.patches[0],
         'pp',
@@ -167,7 +174,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', GIT.COPY),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(
         patches.patches[0],
         'pp',
@@ -184,7 +191,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', RAW.CRAP_ONLY),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(
         patches.patches[0],
         '__init__.py',
@@ -198,7 +205,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', RAW.DELETE),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(patches.patches[0], name, RAW.DELETE, is_delete=True)
 
   def test_delete_empty(self):
@@ -208,7 +215,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', GIT.DELETE_EMPTY),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(
         patches.patches[0],
         name,
@@ -225,7 +232,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', RAW.PATCH),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(
         patches.patches[0],
         'chrome/file.cc',
@@ -242,7 +249,7 @@ class RietveldTest(unittest.TestCase):
       self.rietveld.get_patch(123, 456)
       self.fail()
     except patch.UnsupportedPatchFormat, e:
-      self.assertEquals('file_a', e.filename)
+      self.assertEqual('file_a', e.filename)
 
   def test_get_patch_moved(self):
     self.requests = [
@@ -250,7 +257,7 @@ class RietveldTest(unittest.TestCase):
         ('/download/issue123_456_789.diff', RAW.MINIMAL_RENAME),
     ]
     patches = self.rietveld.get_patch(123, 456)
-    self.assertEquals(1, len(patches.patches))
+    self.assertEqual(1, len(patches.patches))
     self._check_patch(
         patches.patches[0],
         'file_b',
@@ -269,14 +276,14 @@ class RietveldTest(unittest.TestCase):
 
     # svn:mergeinfo across branches:
     # http://codereview.chromium.org/202046/diff/1/third_party/libxml/xmlcatalog_dummy.cc
-    self.assertEquals(
+    self.assertEqual(
         [('svn:eol-style', 'LF')],
         rietveld.Rietveld.parse_svn_properties(
           u'\nAdded: svn:eol-style\n   + LF\n', 'foo'))
 
     # svn:eol-style property that is lost in the diff
     # http://codereview.chromium.org/202046/diff/1/third_party/libxml/xmllint_dummy.cc
-    self.assertEquals(
+    self.assertEqual(
         [],
         rietveld.Rietveld.parse_svn_properties(
           u'\nAdded: svn:mergeinfo\n'
@@ -284,13 +291,13 @@ class RietveldTest(unittest.TestCase):
           'libxml/xmldummy_mac.cc:r69-2775\n',
           'foo'))
 
-    self.assertEquals(
+    self.assertEqual(
         [],
         rietveld.Rietveld.parse_svn_properties(u'', 'foo'))
 
 
     # http://codereview.chromium.org/api/7834045/15001
-    self.assertEquals(
+    self.assertEqual(
         [('svn:executable', '*'), ('svn:eol-style', 'LF')],
         rietveld.Rietveld.parse_svn_properties(
           '\n'
@@ -301,7 +308,7 @@ class RietveldTest(unittest.TestCase):
           'foo'))
 
     # http://codereview.chromium.org/api/9139006/7001
-    self.assertEquals(
+    self.assertEqual(
         [('svn:mime-type', 'image/png')],
         rietveld.Rietveld.parse_svn_properties(
           '\n'
@@ -314,7 +321,7 @@ class RietveldTest(unittest.TestCase):
       rietveld.Rietveld.parse_svn_properties(u'\n', 'foo')
       self.fail()
     except rietveld.patch.UnsupportedPatchFormat, e:
-      self.assertEquals('foo', e.filename)
+      self.assertEqual('foo', e.filename)
     # TODO(maruel): Change with no diff, only svn property change:
     # http://codereview.chromium.org/6462019/
 
@@ -352,7 +359,7 @@ class RietveldTest(unittest.TestCase):
       True,
       True,
       ))
-    self.assertEquals([], results)
+    self.assertEqual([], results)
 
   def test_results_cursor(self):
     # Verify cursor iteration is transparent.
@@ -379,8 +386,39 @@ class RietveldTest(unittest.TestCase):
         {'foo': 'prout'},
     ]
     for i in self.rietveld.search(base='base'):
-      self.assertEquals(expected.pop(0), i)
-    self.assertEquals([], expected)
+      self.assertEqual(expected.pop(0), i)
+    self.assertEqual([], expected)
+
+
+class CachingRietveldTest(BaseFixture):
+  # Tests only one request is done.
+  TESTED_CLASS = rietveld.CachingRietveld
+
+  def test_get_description(self):
+    self.requests = [
+      ('/1/description', 'Blah blah blah'),
+    ]
+    expected = 'Blah blah blah'
+    self.assertEqual(expected, self.rietveld.get_description(1))
+    self.assertEqual(expected, self.rietveld.get_description(1))
+
+  def test_get_issue_properties(self):
+    self.requests = [
+      ('/api/1?messages=true', rietveld.json.dumps({'messages': 'foo'})),
+    ]
+    expected = {}
+    expected_msg = {'messages': 'foo'}
+    self.assertEqual(expected, self.rietveld.get_issue_properties(1, False))
+    self.assertEqual(expected_msg, self.rietveld.get_issue_properties(1, True))
+
+  def test_get_patchset_properties(self):
+    self.requests = [
+      ('/api/1/2', '{}'),
+    ]
+    expected = {}
+    self.assertEqual(expected, self.rietveld.get_patchset_properties(1, 2))
+    self.assertEqual(expected, self.rietveld.get_patchset_properties(1, 2))
+
 
 
 if __name__ == '__main__':