Revert "presubmit: Use new API to check for owners approval"

This reverts commit 64d94dea.

Reason for revert: tentative revert for https://crbug.com/1166467

Original change's description:
> presubmit: Use new API to check for owners approval
>
> It also allows us to improve unit tests.
>
> Change-Id: I356a2fddcbcc5af0e628f79ede1ba277008f5cde
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2612222
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
> Reviewed-by: Josip Sokcevic <sokcevic@google.com>
> Reviewed-by: Gavin Mak <gavinmak@google.com>
> Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>

TBR=ehmaldonado@chromium.org,gavinmak@google.com,infra-scoped@luci-project-accounts.iam.gserviceaccount.com,sokcevic@google.com

Bug: 1166467
Change-Id: I1df97f8fdbc56942fdcc7bafffed517e24a9481d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2628574Reviewed-by: Ben Pastene <bpastene@chromium.org>
Commit-Queue: Ben Pastene <bpastene@chromium.org>

presubmit_canned_checks.py

@@ -1167,17 +1167,17 @@ def CheckOwners(
 
   affected_files = set([f.LocalPath() for f in
       input_api.change.AffectedFiles(file_filter=source_file_filter)])
+  owners_db = input_api.owners_db
+  owners_db.override_files = input_api.change.OriginalOwnersFiles()
   owner_email, reviewers = GetCodereviewOwnerAndReviewers(
       input_api,
+      owners_db.email_regexp,
       approval_needed=input_api.is_committing)
 
   owner_email = owner_email or input_api.change.author_email
 
-  approval_status = input_api.owners_client.GetFilesApprovalStatus(
-      affected_files, reviewers.union([owner_email]), [])
-  missing_files = [
-      f for f in affected_files
-      if approval_status[f] != input_api.owners_client.APPROVED]
+  missing_files = owners_db.files_not_covered_by(
+      affected_files, reviewers.union([owner_email]))
   affects_owners = any('OWNERS' in name for name in missing_files)
 
   if input_api.is_committing:
@@ -1206,7 +1206,7 @@ def CheckOwners(
     if tbr and affects_owners:
       output_list.append(output_fn('TBR for OWNERS files are ignored.'))
     if not input_api.is_committing:
-      suggested_owners = input_api.owners_client.SuggestOwners(missing_files)
+      suggested_owners = owners_db.reviewers_for(missing_files, owner_email)
       output_list.append(output_fn('Suggested OWNERS: ' +
           '(Use "git-cl owners" to interactively select owners.)\n    %s' %
           ('\n    '.join(suggested_owners))))
@@ -1217,7 +1217,7 @@ def CheckOwners(
   return []
 
 
-def GetCodereviewOwnerAndReviewers(input_api, approval_needed):
+def GetCodereviewOwnerAndReviewers(input_api, email_regexp, approval_needed):
   """Return the owner and reviewers of a change, if any.
 
   If approval_needed is True, only reviewers who have approved the change
@@ -1228,8 +1228,6 @@ def GetCodereviewOwnerAndReviewers(input_api, approval_needed):
     return None, (set() if approval_needed else
                   _ReviewersFromChange(input_api.change))
 
-  # Recognizes 'X@Y' email addresses. Very simplistic.
-  email_regexp = input_api.re.compile(r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$')
   owner_email = input_api.gerrit.GetChangeOwner(issue)
   reviewers = set(
       r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed)

tests/presubmit_unittest.py

@@ -44,7 +44,6 @@ import git_cl
 import git_common as git
 import json
 import owners
-import owners_client
 import owners_finder
 import presubmit_support as presubmit
 import rdb_wrapper
@@ -2694,67 +2693,90 @@ the current line as well!
     self.assertEqual(1, len(results))
     self.assertIsInstance(results[0], presubmit.OutputApi.PresubmitError)
 
-  def AssertOwnersWorks(
-      self, tbr=False, issue='1', approvers=None, modified_files=None,
-      owners_by_path=None, is_committing=True, response=None,
-      expected_output='', manually_specified_reviewers=None, dry_run=None,
-      allow_tbr=True):
-    # The set of people who lgtm'ed a change.
-    approvers = approvers or set()
-    manually_specified_reviewers = manually_specified_reviewers or []
-    modified_files = modified_files or ['foo/xyz.cc']
-    owners_by_path = owners_by_path or {'foo/xyz.cc': ['john@example.com']}
-    response = response or {
-      "owner": {"email": 'john@example.com'},
-      "labels": {"Code-Review": {
-        u'all': [
-          {
-            u'email': a,
-            u'value': +1
-          } for a in approvers
-        ],
-        u'default_value': 0,
-        u'values': {u' 0': u'No score',
-                    u'+1': u'Looks good to me',
-                    u'-1': u"I would prefer that you didn't submit this"}
-      }},
-      "reviewers": {"REVIEWER": [{u'email': a}] for a in approvers},
-    }
+  def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None,
+      reviewers=None, is_committing=True,
+      response=None, uncovered_files=None, expected_output='',
+      manually_specified_reviewers=None, dry_run=None,
+      modified_file='foo/xyz.cc', allow_tbr=True):
+    if approvers is None:
+      # The set of people who lgtm'ed a change.
+      approvers = set()
+    if reviewers is None:
+      # The set of people needed to lgtm a change. We default to
+      # the same list as the people who approved it. We use 'reviewers'
+      # to avoid a name collision w/ owners.py.
+      reviewers = approvers
+    if uncovered_files is None:
+      uncovered_files = set()
+    if manually_specified_reviewers is None:
+      manually_specified_reviewers = []
 
     change = mock.MagicMock(presubmit.Change)
-    change.OriginalOwnersFiles.return_value = {}
-    change.RepositoryRoot.return_value = None
-    change.ReviewersFromDescription.return_value = manually_specified_reviewers
-    change.TBRsFromDescription.return_value = []
-    change.author_email = 'john@example.com'
     change.issue = issue
-
-    affected_files = []
-    for f in modified_files:
-      affected_file = mock.MagicMock(presubmit.GitAffectedFile)
-      affected_file.LocalPath.return_value = f
-      affected_files.append(affected_file)
-    change.AffectedFiles.return_value = affected_files
-
+    change.author_email = 'john@example.com'
+    change.ReviewersFromDescription = lambda: manually_specified_reviewers
+    change.TBRsFromDescription = lambda: []
+    change.RepositoryRoot = lambda: None
+    affected_file = mock.MagicMock(presubmit.GitAffectedFile)
     input_api = self.MockInputApi(change, False)
     input_api.gerrit = presubmit.GerritAccessor('host')
+
+    fake_db = mock.MagicMock(owners.Database)
+    fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP)
+    fake_db.files_not_covered_by.return_value = uncovered_files
+
+    input_api.owners_db = fake_db
     input_api.is_committing = is_committing
     input_api.tbr = tbr
     input_api.dry_run = dry_run
-    input_api.gerrit._FetchChangeDetail = lambda _: response
 
-    input_api.owners_client = owners_client.DepotToolsClient('root', 'branch')
-
-    with mock.patch('owners_client.DepotToolsClient.ListOwners',
-                    side_effect=lambda f: owners_by_path.get(f, [])):
-      results = presubmit_canned_checks.CheckOwners(
-          input_api, presubmit.OutputApi, allow_tbr=allow_tbr)
+    affected_file.LocalPath.return_value = modified_file
+    change.AffectedFiles.return_value = [affected_file]
+    if not is_committing or issue or ('OWNERS' in modified_file):
+      change.OriginalOwnersFiles.return_value = {}
+      if issue and not response:
+        response = {
+          "owner": {"email": change.author_email},
+          "labels": {"Code-Review": {
+            u'all': [
+              {
+                u'email': a,
+                u'value': +1
+              } for a in approvers
+            ],
+            u'default_value': 0,
+            u'values': {u' 0': u'No score',
+                        u'+1': u'Looks good to me',
+                        u'-1': u"I would prefer that you didn't submit this"}
+          }},
+          "reviewers": {"REVIEWER": [{u'email': a}] for a in approvers},
+        }
+
+      if is_committing:
+        people = approvers
+      else:
+        people = reviewers
+
+      if issue:
+        input_api.gerrit._FetchChangeDetail = lambda _: response
+
+      people.add(change.author_email)
+      if not is_committing and uncovered_files:
+        fake_db.reviewers_for.return_value = change.author_email
+
+    results = presubmit_canned_checks.CheckOwners(
+        input_api, presubmit.OutputApi, allow_tbr=allow_tbr)
     for result in results:
       result.handle()
     if expected_output:
       self.assertRegexpMatches(sys.stdout.getvalue(), expected_output)
     else:
       self.assertEqual(sys.stdout.getvalue(), expected_output)
+
+    files_not_covered_by_calls = fake_db.files_not_covered_by.mock_calls
+    if len(files_not_covered_by_calls):
+      actual_reviewers = fake_db.files_not_covered_by.mock_calls[0][1][1]
+      self.assertIn(change.author_email, actual_reviewers)
     sys.stdout.truncate(0)
 
   def testCannedCheckOwners_DryRun(self):
@@ -2775,16 +2797,15 @@ the current line as well!
       }},
       "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]},
     }
-    self.AssertOwnersWorks(
-        approvers=set(),
+    self.AssertOwnersWorks(approvers=set(),
         dry_run=True,
         response=response,
+        reviewers=set(["ben@example.com"]),
         expected_output='This is a dry run, but these failures would be ' +
                         'reported on commit:\nMissing LGTM from someone ' +
                         'other than john@example.com\n')
 
-    self.AssertOwnersWorks(
-        approvers=set(['ben@example.com']),
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
         is_committing=False,
         response=response,
         expected_output='')
@@ -2806,16 +2827,12 @@ the current line as well!
       }},
       "reviewers": {"REVIEWER": [{u'email': u'bot@example.com'}]},
     }
-    self.AssertOwnersWorks(
-        approvers=set(),
-        modified_files={'foo/xyz.cc': ['john@example.com']},
+    self.AssertOwnersWorks(approvers=set(),
         response=response,
         is_committing=True,
         expected_output='')
 
-    self.AssertOwnersWorks(
-        approvers=set(),
-        modified_files={'foo/xyz.cc': ['john@example.com']},
+    self.AssertOwnersWorks(approvers=set(),
         is_committing=False,
         response=response,
         expected_output='')
@@ -2837,14 +2854,12 @@ the current line as well!
       }},
       "reviewers": {"REVIEWER": [{u'email': u'sheriff@example.com'}]},
     }
-    self.AssertOwnersWorks(
-        approvers=set(),
+    self.AssertOwnersWorks(approvers=set(),
         response=response,
         is_committing=True,
         expected_output='')
 
-    self.AssertOwnersWorks(
-        approvers=set(),
+    self.AssertOwnersWorks(approvers=set(),
         is_committing=False,
         response=response,
         expected_output='')
@@ -2873,14 +2888,12 @@ the current line as well!
       }},
       "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]},
     }
-    self.AssertOwnersWorks(
-        approvers=set(['ben@example.com']),
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
         response=response,
         is_committing=True,
         expected_output='')
 
-    self.AssertOwnersWorks(
-        approvers=set(['ben@example.com']),
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
         is_committing=False,
         response=response,
         expected_output='')
@@ -2903,8 +2916,7 @@ the current line as well!
       }},
       "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]},
     }
-    self.AssertOwnersWorks(
-        approvers=set(['ben@example.com']),
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
         response=response,
         is_committing=True,
         expected_output='')
@@ -2935,6 +2947,7 @@ the current line as well!
     }
     self.AssertOwnersWorks(
         approvers=set(),
+        reviewers=set(["ben@example.com"]),
         response=response,
         is_committing=True,
         expected_output=
@@ -2942,6 +2955,7 @@ the current line as well!
 
     self.AssertOwnersWorks(
         approvers=set(),
+        reviewers=set(["ben@example.com"]),
         is_committing=False,
         response=response,
         expected_output='')
@@ -2966,6 +2980,7 @@ the current line as well!
     }
     self.AssertOwnersWorks(
         approvers=set(),
+        reviewers=set(["ben@example.com"]),
         response=response,
         is_committing=True,
         expected_output=
@@ -2984,12 +2999,14 @@ the current line as well!
     }
     self.AssertOwnersWorks(
         approvers=set(),
+        reviewers=set(),
         response=response,
         expected_output=
             'Missing LGTM from someone other than john@example.com\n')
 
     self.AssertOwnersWorks(
         approvers=set(),
+        reviewers=set(),
         is_committing=False,
         response=response,
         expected_output='')
@@ -3003,37 +3020,36 @@ the current line as well!
 
   def testCannedCheckOwners_NoIssue(self):
     self.AssertOwnersWorks(issue=None,
-        modified_files=['foo'],
+        uncovered_files=set(['foo']),
         expected_output="OWNERS check failed: this CL has no Gerrit "
                         "change number, so we can't check it for approvals.\n")
     self.AssertOwnersWorks(issue=None,
         is_committing=False,
-        modified_files=['foo'],
+        uncovered_files=set(['foo']),
         expected_output=re.compile(
             'Missing OWNER reviewers for these files:\n'
             '    foo\n', re.MULTILINE))
 
   def testCannedCheckOwners_NoIssueLocalReviewers(self):
-    self.AssertOwnersWorks(
-        issue=None,
+    self.AssertOwnersWorks(issue=None,
+        reviewers=set(['jane@example.com']),
         manually_specified_reviewers=['jane@example.com'],
         expected_output="OWNERS check failed: this CL has no Gerrit "
                         "change number, so we can't check it for approvals.\n")
-    self.AssertOwnersWorks(
-        issue=None,
+    self.AssertOwnersWorks(issue=None,
+        reviewers=set(['jane@example.com']),
         manually_specified_reviewers=['jane@example.com'],
         is_committing=False,
         expected_output='')
 
   def testCannedCheckOwners_NoIssueLocalReviewersDontInferEmailDomain(self):
-    self.AssertOwnersWorks(
-        issue=None,
+    self.AssertOwnersWorks(issue=None,
+        reviewers=set(['jane']),
         manually_specified_reviewers=['jane@example.com'],
         expected_output="OWNERS check failed: this CL has no Gerrit "
                         "change number, so we can't check it for approvals.\n")
-    self.AssertOwnersWorks(
-        issue=None,
-        modified_files=['foo'],
+    self.AssertOwnersWorks(issue=None,
+        uncovered_files=set(['foo']),
         manually_specified_reviewers=['jane'],
         is_committing=False,
         expected_output=re.compile(
@@ -3073,7 +3089,8 @@ the current line as well!
   def testCannedCheckOwners_TBROWNERSFile(self):
     self.AssertOwnersWorks(
         tbr=True,
-        modified_files=['foo/OWNERS'],
+        uncovered_files=set(['foo/OWNERS']),
+        modified_file='foo/OWNERS',
         expected_output='Missing LGTM from an OWNER for these files:\n'
         '    foo/OWNERS\n'
         'TBR for OWNERS files are ignored.\n')
@@ -3081,27 +3098,26 @@ the current line as well!
   def testCannedCheckOwners_TBRNonOWNERSFile(self):
     self.AssertOwnersWorks(
         tbr=True,
-        modified_files=['foo/OWNERS', 'foo/xyz.cc'],
-        owners_by_path={'foo/OWNERS': ['john@example.com'],
-                        'foo/xyz.cc': []},
+        uncovered_files=set(['foo/xyz.cc']),
+        modified_file='foo/OWNERS',
         expected_output='--tbr was specified, skipping OWNERS check\n')
 
   def testCannedCheckOwners_WithoutOwnerLGTM(self):
-    self.AssertOwnersWorks(
-        modified_files=['foo'],
+    self.AssertOwnersWorks(uncovered_files=set(['foo']),
         expected_output='Missing LGTM from an OWNER for these files:\n'
                         '    foo\n')
-    self.AssertOwnersWorks(
-        modified_files=['foo'],
+    self.AssertOwnersWorks(uncovered_files=set(['foo']),
         is_committing=False,
         expected_output=re.compile(
             'Missing OWNER reviewers for these files:\n'
             '    foo\n', re.MULTILINE))
 
   def testCannedCheckOwners_WithLGTMs(self):
-    self.AssertOwnersWorks(approvers=set(['ben@example.com']))
     self.AssertOwnersWorks(approvers=set(['ben@example.com']),
-                           is_committing=False)
+                           uncovered_files=set())
+    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
+                           is_committing=False,
+                           uncovered_files=set())
 
   @mock.patch(BUILTIN_OPEN, mock.mock_open())
   def testCannedRunUnitTests(self):