From d36dbbd730f5adf4bcf0dd5b12aef70533d83986 Mon Sep 17 00:00:00 2001 From: Gavin Mak Date: Mon, 25 Jan 2021 19:34:58 +0000 Subject: [PATCH] [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 Commit-Queue: Gavin Mak --- owners_client.py | 48 +++++++++++++++++----------- owners_finder.py | 63 ++++++++++++++----------------------- tests/owners_client_test.py | 32 ++++++++++++++++++- tests/owners_finder_test.py | 46 ++++++++++++++++++++------- 4 files changed, 119 insertions(+), 70 deletions(-) diff --git a/owners_client.py b/owners_client.py index 2fba02496..fed27af79 100644 --- a/owners_client.py +++ b/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 - # to top-level owners. + # We start at 2 to avoid sending all changes that require multiple + # 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 - # the first one that covers all paths. + # Iterate all combinations of `num_owners` by decreasing score, and + # 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 - # distance are returned in random order to avoid overloading those who - # would appear first. + # We add a small random number to the distance, so that owners at the + # same distance are returned in random order to avoid overloading those + # who would appear first. return sorted( distance_by_owner, key=lambda o: distance_by_owner[o][0][1] + random.random()) diff --git a/owners_finder.py b/owners_finder.py index 99b56bedf..7813bffb9 100644 --- a/owners_finder.py +++ b/owners_finder.py @@ -8,10 +8,10 @@ from __future__ import print_function import os import copy -import owners as owners_module -import random +import owners_client +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( - filtered_files, reviewers)) + self.client = owners_client.DepotToolsClient( + 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) - if author and author in self.all_possible_owners: - del self.all_possible_owners[author] + self.files_to_owners = self.client.BatchListOwners(files) self.owners_to_files = {} - self._map_owners_to_files(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._map_owners_to_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): - for owner in self.all_possible_owners: - for dir_name, _ in self.all_possible_owners[owner]: - for file_name in files: - if file_name.startswith(dir_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 _map_owners_to_files(self): + for file_name in self.files_to_owners: + for owner in self.files_to_owners[file_name]: + self.owners_to_files.setdefault(owner, set()) + self.owners_to_files[owner].add(file_name) 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()) - random.shuffle(owners) - self.owners_queue = sorted(owners, - key=lambda owner: self.owners_score[owner]) + owners = self.client.ScoreOwners(self.files_to_owners.keys()) + if self.author and self.author in owners: + owners.remove(self.author) + self.owners_queue = owners self.find_mandatory_owners() def select_owner(self, owner, findMandatoryOwners=True): diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py index 1ecbe9c53..9aaa8493f 100644 --- a/tests/owners_client_test.py +++ b/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], diff --git a/tests/owners_finder_test.py b/tests/owners_finder_test.py index 457564761..cd7c793f4 100755 --- a/tests/owners_finder_test.py +++ b/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])