[owners] Use owners_client in owners_finder.py

This change also adds a ScoreOwners function in owners_client
that replaces user scoring functionality in owners_finder.

Change-Id: Ifd8841c6d320d9bb644907b6eca0a02d4ef35640
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2641532
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>

owners_client.py

@@ -104,34 +104,44 @@ class OwnersClient(object):
         status[path] = self.INSUFFICIENT_REVIEWERS
     return status
 
+  def ScoreOwners(self, paths):
+    """Get sorted list of owners for the given paths."""
+    positions_by_owner = {}
+    owners_by_path = self.BatchListOwners(paths)
+    for owners in owners_by_path.values():
+      for i, owner in enumerate(owners):
+        # Gerrit API lists owners of a path sorted by an internal score, so
+        # owners that appear first should be prefered.
+        # We define the score of an owner based on the pair
+        # (# of files owned, minimum position on all owned files)
+        positions_by_owner.setdefault(owner, []).append(i)
+
+    # Sort owners by their score. Rank owners higher for more files owned and
+    # lower for a larger minimum position across all owned files. Randomize
+    # order for owners with same score to avoid bias.
+    return sorted(
+        positions_by_owner,
+        key=lambda o: (-len(positions_by_owner[o]),
+                       min(positions_by_owner[o]) + random.random()))
+
   def SuggestOwners(self, paths):
     """Suggest a set of owners for the given paths."""
     paths_by_owner = {}
-    score_by_owner = {}
     owners_by_path = self.BatchListOwners(paths)
     for path, owners in owners_by_path.items():
-      for i, owner in enumerate(owners):
+      for owner in owners:
         paths_by_owner.setdefault(owner, set()).add(path)
-        # Gerrit API lists owners of a path sorted by an internal score, so
-        # owners that appear first should be prefered.
-        # We define the score of an owner to be their minimum position in all
-        # paths.
-        score_by_owner[owner] = min(i, score_by_owner.get(owner, i))
-
-    # Sort owners by their score. Randomize order of owners with same score.
-    owners = sorted(
-        score_by_owner,
-        key=lambda o: (score_by_owner[o], random.random()))
 
     # Select the minimum number of owners that can approve all paths.
-    # We start at 2 to avoid sending all changes that require multiple reviewers
+    # We start at 2 to avoid sending all changes that require multiple
-    # to top-level owners.
+    # reviewers to top-level owners.
+    owners = self.ScoreOwners(paths)
     if len(owners) < 2:
       return owners
 
     for num_owners in range(2, len(owners)):
-      # Iterate all combinations of `num_owners` by decreasing score, and select
+      # Iterate all combinations of `num_owners` by decreasing score, and
-      # the first one that covers all paths.
+      # select the first one that covers all paths.
       for selected in _owner_combinations(owners, num_owners):
         covered = set.union(*(paths_by_owner[o] for o in selected))
         if len(covered) == len(paths):
@@ -170,9 +180,9 @@ class DepotToolsClient(OwnersClient):
       # all_possible_owners returns a dict {owner: [(path, distance)]}. We want
       # to return a list of owners sorted by increasing distance.
       distance_by_owner = self._db.all_possible_owners([path], None)
-      # We add a small random number to the distance, so that owners at the same
+      # We add a small random number to the distance, so that owners at the
-      # distance are returned in random order to avoid overloading those who
+      # same distance are returned in random order to avoid overloading those
-      # would appear first.
+      # who would appear first.
       return sorted(
           distance_by_owner,
           key=lambda o: distance_by_owner[o][0][1] + random.random())

owners_finder.py

@@ -8,10 +8,10 @@ from __future__ import print_function
 
 import os
 import copy
-import owners as owners_module
+import owners_client
-import random
 
 
+import git_common
 import gclient_utils
 
 
@@ -42,10 +42,6 @@ class OwnersFinder(object):
       self.COLOR_GREY = ''
       self.COLOR_RESET = ''
 
-    self.db = owners_module.Database(local_root, fopen, os_path)
-    self.db.override_files = override_files or {}
-    self.db.load_data_needed_for(files)
-
     self.os_path = os_path
 
     self.author = author
@@ -57,29 +53,26 @@ class OwnersFinder(object):
       reviewers.append(author)
 
     # Eliminate files that existing reviewers can review.
-    filtered_files = list(self.db.files_not_covered_by(
+    self.client = owners_client.DepotToolsClient(
-        filtered_files, reviewers))
+      root=local_root,
+      branch=git_common.current_branch(),
+      fopen=fopen,
+      os_path=os_path)
+
+    approval_status = self.client.GetFilesApprovalStatus(
+      filtered_files, reviewers, [])
+    filtered_files = [
+      f for f in filtered_files
+      if approval_status[f] != owners_client.OwnersClient.APPROVED]
 
     # If some files are eliminated.
     if len(filtered_files) != len(files):
       files = filtered_files
-      # Reload the database.
-      self.db = owners_module.Database(local_root, fopen, os_path)
-      self.db.override_files = override_files or {}
-      self.db.load_data_needed_for(files)
 
-    self.all_possible_owners = self.db.all_possible_owners(files, None)
+    self.files_to_owners = self.client.BatchListOwners(files)
-    if author and author in self.all_possible_owners:
-      del self.all_possible_owners[author]
 
     self.owners_to_files = {}
-    self._map_owners_to_files(files)
+    self._map_owners_to_files()
-
-    self.files_to_owners = {}
-    self._map_files_to_owners()
-
-    self.owners_score = self.db.total_costs_by_owner(
-        self.all_possible_owners, files)
 
     self.original_files_to_owners = copy.deepcopy(self.files_to_owners)
 
@@ -143,19 +136,11 @@ class OwnersFinder(object):
     self.print_result()
     return 0
 
-  def _map_owners_to_files(self, files):
+  def _map_owners_to_files(self):
-    for owner in self.all_possible_owners:
+    for file_name in self.files_to_owners:
-      for dir_name, _ in self.all_possible_owners[owner]:
+      for owner in self.files_to_owners[file_name]:
-        for file_name in files:
+        self.owners_to_files.setdefault(owner, set())
-          if file_name.startswith(dir_name):
+        self.owners_to_files[owner].add(file_name)
-            self.owners_to_files.setdefault(owner, set())
-            self.owners_to_files[owner].add(file_name)
-
-  def _map_files_to_owners(self):
-    for owner in self.owners_to_files:
-      for file_name in self.owners_to_files[owner]:
-        self.files_to_owners.setdefault(file_name, set())
-        self.files_to_owners[file_name].add(owner)
 
   def reset(self):
     self.files_to_owners = copy.deepcopy(self.original_files_to_owners)
@@ -166,10 +151,10 @@ class OwnersFinder(object):
 
     # Randomize owners' names so that if many reviewers have identical scores
     # they will be randomly ordered to avoid bias.
-    owners = list(self.owners_to_files.keys())
+    owners = self.client.ScoreOwners(self.files_to_owners.keys())
-    random.shuffle(owners)
+    if self.author and self.author in owners:
-    self.owners_queue = sorted(owners,
+      owners.remove(self.author)
-                               key=lambda owner: self.owners_score[owner])
+    self.owners_queue = owners
     self.find_mandatory_owners()
 
   def select_owner(self, owner, findMandatoryOwners=True):

tests/owners_client_test.py

@@ -144,6 +144,36 @@ class OwnersClientTest(unittest.TestCase):
          (emily, chris),
          (emily, dave)])
 
+  def testScoreOwners(self):
+    self.client.owners_by_path = {
+        'a': [alice, bob, chris]
+    }
+    self.assertEqual(
+      self.client.ScoreOwners(self.client.owners_by_path.keys()),
+      [alice, bob, chris]
+    )
+
+    self.client.owners_by_path = {
+        'a': [alice, bob],
+        'b': [bob],
+        'c': [bob, chris]
+    }
+    self.assertEqual(
+      self.client.ScoreOwners(self.client.owners_by_path.keys()),
+      [bob, alice, chris]
+    )
+
+    self.client.owners_by_path = {
+        'a': [alice, bob, chris, dave],
+        'b': [chris, bob, dave],
+        'c': [chris, dave],
+        'd': [alice, chris, dave]
+    }
+    self.assertEqual(
+      self.client.ScoreOwners(self.client.owners_by_path.keys()),
+      [chris, dave, alice, bob]
+    )
+
   def testSuggestOwners(self):
     self.client.owners_by_path = {'a': [alice]}
     self.assertEqual(
@@ -174,7 +204,7 @@ class OwnersClientTest(unittest.TestCase):
     }
     self.assertEqual(
         sorted(self.client.SuggestOwners(['ad', 'cad', 'ead', 'bd'])),
-        [alice, bob])
+        [alice, dave])
 
     self.client.owners_by_path = {
         'a': [alice],

tests/owners_finder_test.py

@@ -9,13 +9,17 @@ import os
 import sys
 import unittest
 
+if sys.version_info.major == 2:
+  import mock
+else:
+  from unittest import mock
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
 from testing_support import filesystem_mock
 
 import owners_finder
-import owners
+import owners_client
 
 
 ben = 'ben@example.com'
@@ -29,6 +33,14 @@ tom = 'tom@example.com'
 nonowner = 'nonowner@example.com'
 
 
+def _get_score_owners_darin_variant():
+  return [brett, darin, john, peter, ken, ben, tom]
+
+
+def _get_score_owners_john_variant():
+  return [brett, john, darin, peter, ken, ben, tom]
+
+
 def owners_file(*email_addresses, **kwargs):
   s = ''
   if kwargs.get('comment'):
@@ -64,7 +76,8 @@ def test_repo():
     '/content/common/common.cc': '',
     '/content/foo/OWNERS': owners_file(jochen, comment='foo'),
     '/content/foo/foo.cc': '',
-    '/content/views/OWNERS': owners_file(ben, john, owners.EVERYONE,
+    '/content/views/OWNERS': owners_file(ben, john,
+                                         owners_client.OwnersClient.EVERYONE,
                                          noparent=True),
     '/content/views/pie.h': '',
   })
@@ -114,6 +127,9 @@ class _BaseTestCase(unittest.TestCase):
     self.repo = test_repo()
     self.root = '/'
     self.fopen = self.repo.open_for_reading
+    mock.patch('owners_client.DepotToolsClient._GetOriginalOwnersFiles',
+      return_value={}).start()
+    self.addCleanup(mock.patch.stopall)
 
   def ownersFinder(self, files, author=nonowner, reviewers=None):
     reviewers = reviewers or []
@@ -162,13 +178,16 @@ class OwnersFinderTests(_BaseTestCase):
     self.assertEqual(finder.unreviewed_files, {'content/bar/foo.cc'})
 
   def test_reset(self):
+    mock.patch('owners_client.DepotToolsClient.ScoreOwners',
+      side_effect=[
+        _get_score_owners_darin_variant(),
+        _get_score_owners_darin_variant(),
+        _get_score_owners_darin_variant()
+      ]).start()
+
     finder = self.defaultFinder()
     for _ in range(2):
       expected = [brett, darin, john, peter, ken, ben, tom]
-      # darin and john have equal cost, the others have distinct costs.
-      # If the owners_queue has those two swapped then swap them in expected.
-      if finder.owners_queue[1] != expected[1]:
-        expected[1], expected[2] = expected[2], expected[1]
       self.assertEqual(finder.owners_queue, expected)
       self.assertEqual(finder.unreviewed_files, {
           'base/vlog.h',
@@ -191,6 +210,13 @@ class OwnersFinderTests(_BaseTestCase):
       finder.resetText()
 
   def test_select(self):
+    mock.patch('owners_client.DepotToolsClient.ScoreOwners',
+      side_effect=[
+        _get_score_owners_darin_variant(),
+        _get_score_owners_john_variant(),
+        _get_score_owners_darin_variant()
+      ]).start()
+
     finder = self.defaultFinder()
     finder.select_owner(john)
     self.assertEqual(finder.owners_queue, [brett, peter, ken, ben, tom])
@@ -218,11 +244,6 @@ class OwnersFinderTests(_BaseTestCase):
     finder = self.defaultFinder()
     finder.select_owner(brett)
     expected = [darin, john, peter, ken, tom]
-    # darin and john have equal cost, the others have distinct costs.
-    # If the owners_queue has those two swapped then swap them in expected.
-    if finder.owners_queue[0] == john:
-      expected[0], expected[1] = expected[1], expected[0]
-
     self.assertEqual(finder.owners_queue, expected)
     self.assertEqual(finder.selected_owners, {brett})
     self.assertEqual(finder.deselected_owners, {ben})
@@ -237,6 +258,9 @@ class OwnersFinderTests(_BaseTestCase):
                      ['Selected: ' + brett, 'Deselected: ' + ben])
 
   def test_deselect(self):
+    mock.patch('owners_client.DepotToolsClient.ScoreOwners',
+      return_value=_get_score_owners_darin_variant()).start()
+
     finder = self.defaultFinder()
     finder.deselect_owner(john)
     self.assertEqual(finder.owners_queue, [brett, peter, ken, ben, tom])