Actually check Rietveld for LGTMs ...

This change requires us to change the previous signature for the CheckOwners() hook to provide the server address and email regexp to use for parsing approvals from Rietveld.

Review URL: http://codereview.chromium.org/6657028

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

PRESUBMIT.py

@@ -20,6 +20,7 @@ UNIT_TESTS = [
   'tests.watchlists_unittest',
 ]
 
+
 def CommonChecks(input_api, output_api):
   output = []
   # Verify that LocalPath() is local, e.g.:
@@ -31,9 +32,11 @@ def CommonChecks(input_api, output_api):
       # Return right away because it needs to be fixed first.
       return output
 
-  output.extend(input_api.canned_checks.CheckOwners(
-      input_api,
-      output_api))
+  # TODO(dpranke): uncomment and enable :).
+  #
+  # output.extend(input_api.canned_checks.CheckOwners(
+  #     input_api,
+  #     output_api))
 
   output.extend(input_api.canned_checks.RunPythonUnitTests(
       input_api,

owners.py

@@ -44,8 +44,7 @@ class Database(object):
     self.fopen = fopen
     self.os_path = os_path
 
-    # TODO: Figure out how to share the owners email addr format w/
-    # tools/commit-queue/projects.py, especially for per-repo whitelists.
+    # Pick a default email regexp to use; callers can override as desired.
     self.email_regexp = re.compile(BASIC_EMAIL_REGEXP)
 
     # Mapping of owners to the paths they own.

presubmit_canned_checks.py

@@ -540,7 +540,7 @@ def RunPylint(input_api, output_api, white_list=None, black_list=None):
   finally:
     warnings.filterwarnings('default', category=DeprecationWarning)
 
-
+# TODO(dpranke): Get the host_url from the input_api instead
 def CheckRietveldTryJobExecution(input_api, output_api, host_url, platforms,
                                  owner):
   if not input_api.is_committing:
@@ -631,11 +631,14 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
       input_api.change.AffectedFiles(source_file_filter)])
   owners_db = input_api.owners_db
 
-  if input_api.is_committing:
-    missing_files = owners_db.files_not_covered_by(affected_files,
-                                                input_api.change.approvers)
+  if input_api.is_committing and input_api.tbr:
+    return [output_api.PresubmitNotifyResult(
+        '--tbr was specified, skipping OWNERS check')]
+  elif input_api.is_committing:
+    approvers = _Approvers(input_api, owners_db.email_regexp)
+    missing_files = owners_db.files_not_covered_by(affected_files, approvers)
     if missing_files:
-      return [output_api.PresubmitError('Missing owner LGTM for: %s' %
+      return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' %
               ','.join(missing_files))]
     return []
   elif input_api.change.tags.get('R'):
@@ -643,3 +646,29 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
 
   suggested_reviewers = owners_db.reviewers_for(affected_files)
   return [output_api.PresubmitAddText('R=%s' % ','.join(suggested_reviewers))]
+
+
+def _Approvers(input_api, email_regexp):
+  if not input_api.change.issue:
+    return []
+
+  path = '/api/%s?messages=true'
+  url = (input_api.host_url + path) % input_api.change.issue
+
+  f = input_api.urllib2.urlopen(url)
+  issue_props = input_api.json.load(f)
+  owner = input_api.re.escape(issue_props['owner'])
+
+  # TODO(dpranke): This mimics the logic in
+  # /tools/commit-queue/verifiers/reviewer_lgtm.py
+  # We should share the code and/or remove the check there where it is
+  # redundant (since the commit queue also enforces the presubmit checks).
+  def match_reviewer(r):
+    return email_regexp.match(r) and not input_api.re.match(owner, r)
+
+  approvers = []
+  for m in issue_props['messages']:
+    if 'lgtm' in m['text'].lower() and match_reviewer(m['sender']):
+      approvers.append(m['sender'])
+  return set(approvers)
+

presubmit_support.py

@@ -227,7 +227,10 @@ class InputApi(object):
       r"(|.*[\\\/])\.svn[\\\/].*",
   )
 
-  def __init__(self, change, presubmit_path, is_committing):
+  # TODO(dpranke): Update callers to pass in tbr, host_url, remove
+  # default arguments.
+  def __init__(self, change, presubmit_path, is_committing, tbr=False,
+    host_url='http://codereview.chromium.org'):
     """Builds an InputApi object.
 
     Args:
@@ -238,7 +241,9 @@ class InputApi(object):
     # Version number of the presubmit_support script.
     self.version = [int(x) for x in __version__.split('.')]
     self.change = change
+    self.host_url = host_url
     self.is_committing = is_committing
+    self.tbr = tbr
 
     # We expose various modules and functions as attributes of the input_api
     # so that presubmit scripts don't have to import them.
@@ -653,10 +658,6 @@ class Change(object):
     self._local_root = os.path.abspath(local_root)
     self.issue = issue
     self.patchset = patchset
-
-    # TODO(dpranke): implement - get from the patchset?
-    self.approvers = set()
-
     self.scm = ''
 
     # From the description text, build up a dictionary of key/value pairs

tests/presubmit_unittest.py

@@ -700,9 +700,10 @@ class InputApiUnittest(PresubmitTestsBase):
       'LocalToDepotPath',
       'PresubmitLocalPath', 'ReadFile', 'RightHandSideLines', 'ServerPaths',
       'basename', 'cPickle', 'cStringIO', 'canned_checks', 'change', 'environ',
-      'is_committing', 'json', 'marshal', 'os_path', 'owners_db', 'pickle',
-      'platform', 'python_executable', 're', 'subprocess', 'tempfile',
-      'traceback', 'unittest', 'urllib2', 'version',
+      'host_url', 'is_committing', 'json', 'marshal', 'os_path',
+      'owners_db', 'pickle', 'platform', 'python_executable', 're',
+      'subprocess', 'tbr', 'tempfile', 'traceback', 'unittest', 'urllib2',
+      'version',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(presubmit.InputApi(self.fake_change, './.', False),
@@ -1187,7 +1188,7 @@ class GclChangeUnittest(PresubmitTestsBase):
         'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTextFiles',
         'DescriptionText', 'FullDescriptionText', 'LocalPaths', 'Name',
         'RepositoryRoot', 'RightHandSideLines', 'ServerPaths',
-        'approvers', 'issue', 'patchset', 'scm', 'tags',
+        'issue', 'patchset', 'scm', 'tags',
     ]
     # If this test fails, you should add the relevant test.
     self.mox.ReplayAll()
@@ -1214,7 +1215,9 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.subprocess = self.mox.CreateMock(presubmit.subprocess)
 
     input_api.change = change
+    input_api.host_url = 'http://localhost'
     input_api.is_committing = committing
+    input_api.tbr = False
     input_api.python_executable = 'pyyyyython'
     return input_api
 
@@ -1857,7 +1860,7 @@ mac|success|blew
     self.assertEquals(results[0].__class__,
         presubmit.OutputApi.PresubmitNotifyResult)
 
-  def OwnersTest(self, is_committing, change_tags=None,
+  def OwnersTest(self, is_committing, tbr=False, change_tags=None,
       suggested_reviewers=None, approvers=None,
       uncovered_files=None, expected_results=None):
     affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
@@ -1867,21 +1870,31 @@ mac|success|blew
 
     input_api = self.MockInputApi(change, False)
     fake_db = self.mox.CreateMock(owners.Database)
+    fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP)
     input_api.owners_db = fake_db
     input_api.is_committing = is_committing
-
-    if is_committing:
-      change.approvers = approvers
+    input_api.tbr = tbr
+
+    if is_committing and not tbr:
+      change.issue = '1'
+      messages = list('{"sender": "' + a + '","text": "lgtm"}' for
+                      a in approvers)
+      rietveld_response = ('{"owner": "john@example.com",'
+                          '"messages": [' + ','.join(messages) + ']}')
+      input_api.urllib2.urlopen(
+          input_api.host_url + '/api/1?messages=true').AndReturn(
+          StringIO.StringIO(rietveld_response))
+      input_api.json = presubmit.json
       fake_db.files_not_covered_by(set(['foo.cc']), approvers).AndReturn(
           uncovered_files)
-    else:
+    elif not is_committing:
       change.tags = change_tags
       if not change_tags.get('R'):
         fake_db.reviewers_for(set(['foo.cc'])).AndReturn(suggested_reviewers)
 
     self.mox.ReplayAll()
     results = presubmit_canned_checks.CheckOwners(input_api,
-        presubmit.OutputApi, None)
+        presubmit.OutputApi)
     self.assertEquals(len(results), len(expected_results))
     if results and expected_results:
       output = StringIO.StringIO()
@@ -1892,24 +1905,34 @@ mac|success|blew
   def testCannedCheckOwners_WithReviewer(self):
     self.OwnersTest(is_committing=False, change_tags={'R': 'ben@example.com'},
         expected_results=[])
+    self.OwnersTest(is_committing=False, tbr=True,
+        change_tags={'R': 'ben@example.com'}, expected_results=[])
 
   def testCannedCheckOwners_NoReviewer(self):
     self.OwnersTest(is_committing=False, change_tags={},
         suggested_reviewers=['ben@example.com'],
         expected_results=['ADD: R=ben@example.com\n'])
+    self.OwnersTest(is_committing=False, tbr=True, change_tags={},
+        suggested_reviewers=['ben@example.com'],
+        expected_results=['ADD: R=ben@example.com\n'])
 
   def testCannedCheckOwners_CommittingWithoutOwnerLGTM(self):
     self.OwnersTest(is_committing=True,
         approvers=set(),
         uncovered_files=set(['foo.cc']),
-        expected_results=['Missing owner LGTM for: foo.cc\n'])
+        expected_results=['Missing LGTM from an OWNER for: foo.cc\n'])
 
   def testCannedCheckOwners_CommittingWithLGTMs(self):
     self.OwnersTest(is_committing=True,
-        approvers=set('ben@example.com'),
+        approvers=set(['ben@example.com']),
         uncovered_files=set(),
         expected_results=[])
 
+  def testCannedCheckOwners_TBR(self):
+    self.OwnersTest(is_committing=True, tbr=True,
+        approvers=set(),
+        uncovered_files=set(),
+        expected_results=['--tbr was specified, skipping OWNERS check\n'])
 
 if __name__ == '__main__':
   import unittest