diff --git a/git_cl.py b/git_cl.py index 9b943d26c..141364bc0 100755 --- a/git_cl.py +++ b/git_cl.py @@ -4831,21 +4831,12 @@ def CMDowners(parser, args): print('\n'.join(owners)) return 0 - root = settings.GetRoot() - owner_files = [f for f in affected_files if 'OWNERS' in os.path.basename(f)] - original_owner_files = { - f: scm.GIT.GetOldContents(root, f, base_branch).splitlines() - for f in owner_files} - return owners_finder.OwnersFinder( affected_files, - root, author, [] if options.ignore_current else cl.GetReviewers(), - fopen=open, - os_path=os.path, + cl.owners_client, disable_color=options.no_color, - override_files=original_owner_files, ignore_author=options.ignore_self).run() diff --git a/owners_finder.py b/owners_finder.py index 7813bffb9..c05aa9c85 100644 --- a/owners_finder.py +++ b/owners_finder.py @@ -8,7 +8,6 @@ from __future__ import print_function import os import copy -import owners_client import git_common @@ -28,11 +27,9 @@ class OwnersFinder(object): indentation = 0 - def __init__(self, files, local_root, author, reviewers, - fopen, os_path, + def __init__(self, files, author, reviewers, owners_client, email_postfix='@chromium.org', disable_color=False, - override_files=None, ignore_author=False): self.email_postfix = email_postfix @@ -42,8 +39,6 @@ class OwnersFinder(object): self.COLOR_GREY = '' self.COLOR_RESET = '' - self.os_path = os_path - self.author = author filtered_files = files @@ -53,23 +48,18 @@ class OwnersFinder(object): reviewers.append(author) # Eliminate files that existing reviewers can review. - self.client = owners_client.DepotToolsClient( - root=local_root, - branch=git_common.current_branch(), - fopen=fopen, - os_path=os_path) - - approval_status = self.client.GetFilesApprovalStatus( + self.owners_client = owners_client + approval_status = self.owners_client.GetFilesApprovalStatus( filtered_files, reviewers, []) filtered_files = [ f for f in filtered_files - if approval_status[f] != owners_client.OwnersClient.APPROVED] + if approval_status[f] != self.owners_client.APPROVED] # If some files are eliminated. if len(filtered_files) != len(files): files = filtered_files - self.files_to_owners = self.client.BatchListOwners(files) + self.files_to_owners = self.owners_client.BatchListOwners(files) self.owners_to_files = {} self._map_owners_to_files() @@ -151,7 +141,7 @@ class OwnersFinder(object): # Randomize owners' names so that if many reviewers have identical scores # they will be randomly ordered to avoid bias. - owners = self.client.ScoreOwners(self.files_to_owners.keys()) + owners = list(self.owners_client.ScoreOwners(self.files_to_owners.keys())) if self.author and self.author in owners: owners.remove(self.author) self.owners_queue = owners diff --git a/tests/owners_finder_test.py b/tests/owners_finder_test.py index cd7c793f4..56955954f 100755 --- a/tests/owners_finder_test.py +++ b/tests/owners_finder_test.py @@ -33,12 +33,6 @@ 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): @@ -50,45 +44,36 @@ def owners_file(*email_addresses, **kwargs): return s + '\n'.join(email_addresses) + '\n' -def test_repo(): - return filesystem_mock.MockFileSystem(files={ - '/DEPS': '', - '/OWNERS': owners_file(ken, peter, tom, - comment='OWNERS_STATUS = build/OWNERS.status'), - '/build/OWNERS.status': '%s: bar' % jochen, - '/base/vlog.h': '', - '/chrome/OWNERS': owners_file(ben, brett), - '/chrome/browser/OWNERS': owners_file(brett), - '/chrome/browser/defaults.h': '', - '/chrome/gpu/OWNERS': owners_file(ken), - '/chrome/gpu/gpu_channel.h': '', - '/chrome/renderer/OWNERS': owners_file(peter), - '/chrome/renderer/gpu/gpu_channel_host.h': '', - '/chrome/renderer/safe_browsing/scorer.h': '', - '/content/OWNERS': owners_file(john, darin, comment='foo', noparent=True), - '/content/content.gyp': '', - '/content/bar/foo.cc': '', - '/content/baz/OWNERS': owners_file(brett), - '/content/baz/froboz.h': '', - '/content/baz/ugly.cc': '', - '/content/baz/ugly.h': '', - '/content/common/OWNERS': owners_file(jochen), - '/content/common/common.cc': '', - '/content/foo/OWNERS': owners_file(jochen, comment='foo'), - '/content/foo/foo.cc': '', - '/content/views/OWNERS': owners_file(ben, john, - owners_client.OwnersClient.EVERYONE, - noparent=True), - '/content/views/pie.h': '', - }) +class TestClient(owners_client.OwnersClient): + def __init__(self): + super(TestClient, self).__init__() + self.owners_by_path = { + 'DEPS': [ken, peter, tom], + 'base/vlog.h': [ken, peter, tom], + 'chrome/browser/defaults.h': [brett, ben, ken, peter, tom], + 'chrome/gpu/gpu_channel.h': [ken, ben, brett, ken, peter, tom], + 'chrome/renderer/gpu/gpu_channel_host.h': [peter, ben, brett, ken, tom], + 'chrome/renderer/safe_browsing/scorer.h': [peter, ben, brett, ken, tom], + 'content/content.gyp': [john, darin], + 'content/bar/foo.cc': [john, darin], + 'content/baz/froboz.h': [brett, john, darin], + 'content/baz/ugly.cc': [brett, john, darin], + 'content/baz/ugly.h': [brett, john, darin], + 'content/common/common.cc': [jochen, john, darin], + 'content/foo/foo.cc': [jochen, john, darin], + 'content/views/pie.h': [ben, john, self.EVERYONE], + } + + def ListOwners(self, path): + path = path.replace(os.sep, '/') + return self.owners_by_path[path] class OutputInterceptedOwnersFinder(owners_finder.OwnersFinder): - def __init__(self, files, local_root, author, reviewers, - fopen, os_path, disable_color=False): + def __init__( + self, files, author, reviewers, client, disable_color=False): super(OutputInterceptedOwnersFinder, self).__init__( - files, local_root, author, reviewers, fopen, os_path, - disable_color=disable_color) + files, author, reviewers, client, disable_color=disable_color) self.output = [] self.indentation_stack = [] @@ -123,24 +108,10 @@ class _BaseTestCase(unittest.TestCase): 'content/views/pie.h' ] - def setUp(self): - 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 [] - finder = OutputInterceptedOwnersFinder(files, - self.root, - author, - reviewers, - fopen=self.fopen, - os_path=self.repo, - disable_color=True) - return finder + return OutputInterceptedOwnersFinder( + files, author, reviewers, TestClient(), disable_color=True) def defaultFinder(self): return self.ownersFinder(self.default_files) @@ -177,14 +148,9 @@ class OwnersFinderTests(_BaseTestCase): finder = self.ownersFinder(files, reviewers=[brett]) 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() - + @mock.patch('owners_client.OwnersClient.ScoreOwners') + def test_reset(self, mockScoreOwners): + mockScoreOwners.return_value = [brett, darin, john, peter, ken, ben, tom] finder = self.defaultFinder() for _ in range(2): expected = [brett, darin, john, peter, ken, ben, tom] @@ -209,14 +175,9 @@ class OwnersFinderTests(_BaseTestCase): finder.reset() 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() - + @mock.patch('owners_client.OwnersClient.ScoreOwners') + def test_select(self, mockScoreOwners): + mockScoreOwners.return_value = [brett, darin, john, peter, ken, ben, tom] finder = self.defaultFinder() finder.select_owner(john) self.assertEqual(finder.owners_queue, [brett, peter, ken, ben, tom]) @@ -257,10 +218,9 @@ class OwnersFinderTests(_BaseTestCase): self.assertEqual(finder.output, ['Selected: ' + brett, 'Deselected: ' + ben]) - def test_deselect(self): - mock.patch('owners_client.DepotToolsClient.ScoreOwners', - return_value=_get_score_owners_darin_variant()).start() - + @mock.patch('owners_client.OwnersClient.ScoreOwners') + def test_deselect(self, mockScoreOwners): + mockScoreOwners.return_value = [brett, darin, john, peter, ken, ben, tom] finder = self.defaultFinder() finder.deselect_owner(john) self.assertEqual(finder.owners_queue, [brett, peter, ken, ben, tom])