diff --git a/owners.py b/owners.py index fc5fa923c8..d8692c362b 100644 --- a/owners.py +++ b/owners.py @@ -1,4 +1,4 @@ -# Copyright (c) 2010 The Chromium Authors. All rights reserved. +# Copyright (c) 2012 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. @@ -75,29 +75,27 @@ class Database(object): self._load_data_needed_for(files) return self._covering_set_of_owners_for(files) - def files_are_covered_by(self, files, reviewers): - """Returns whether every file is owned by at least one reviewer.""" - return not self.files_not_covered_by(files, reviewers) + def directories_not_covered_by(self, files, reviewers): + """Returns the set of directories that are not owned by a reviewer. - def files_not_covered_by(self, files, reviewers): - """Returns the set of files that are not owned by at least one reviewer. + Determines which of the given files are not owned by at least one of the + reviewers, then returns a set containing the applicable enclosing + directories, i.e. the ones upward from the files that have OWNERS files. Args: files is a sequence of paths relative to (and under) self.root. - reviewers is a sequence of strings matching self.email_regexp.""" + reviewers is a sequence of strings matching self.email_regexp. + """ self._check_paths(files) self._check_reviewers(reviewers) - if not reviewers: - return files - self._load_data_needed_for(files) - files_by_dir = self._files_by_dir(files) + + dirs = set([self.os_path.dirname(f) for f in files]) covered_dirs = self._dirs_covered_by(reviewers) - uncovered_files = [] - for d, files_in_d in files_by_dir.iteritems(): - if not self._is_dir_covered_by(d, covered_dirs): - uncovered_files.extend(files_in_d) - return set(uncovered_files) + uncovered_dirs = [self._enclosing_dir_with_owners(d) for d in dirs + if not self._is_dir_covered_by(d, covered_dirs)] + + return set(uncovered_dirs) def _check_paths(self, files): def _is_under(f, pfx): @@ -109,12 +107,6 @@ class Database(object): _assert_is_collection(reviewers) assert all(self.email_regexp.match(r) for r in reviewers) - def _files_by_dir(self, files): - dirs = {} - for f in files: - dirs.setdefault(self.os_path.dirname(f), []).append(f) - return dirs - def _dirs_covered_by(self, reviewers): dirs = self.owned_by[EVERYONE] for r in reviewers: @@ -129,6 +121,15 @@ class Database(object): dirname = self.os_path.dirname(dirname) return dirname in covered_dirs + def _enclosing_dir_with_owners(self, directory): + """Returns the innermost enclosing directory that has an OWNERS file.""" + dirpath = directory + while not dirpath in self.owners_for: + if self._stop_looking(dirpath): + break + dirpath = self.os_path.dirname(dirpath) + return dirpath + def _load_data_needed_for(self, files): for f in files: dirpath = self.os_path.dirname(f) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 8a9bb0382e..271753611e 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -729,47 +729,62 @@ def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings, def CheckOwners(input_api, output_api, source_file_filter=None): - if not input_api.is_committing: - return [] - if input_api.tbr: - return [output_api.PresubmitNotifyResult( - '--tbr was specified, skipping OWNERS check')] - if not input_api.change.issue: - return [output_api.PresubmitError( - "OWNERS check failed: this change has no Rietveld issue number, so " - "we can't check it for approvals.")] + if input_api.is_committing: + if input_api.tbr: + return [output_api.PresubmitNotifyResult( + '--tbr was specified, skipping OWNERS check')] + if not input_api.change.issue: + return [output_api.PresubmitError("OWNERS check failed: this change has " + "no Rietveld issue number, so we can't check it for approvals.")] + needed = 'LGTM from an OWNER' + output = output_api.PresubmitError + else: + needed = 'OWNER reviewers' + output = output_api.PresubmitNotifyResult affected_files = set([f.LocalPath() for f in input_api.change.AffectedFiles(file_filter=source_file_filter)]) owners_db = input_api.owners_db - owner_email, approvers = _RietveldOwnerAndApprovers(input_api, - owners_db.email_regexp) - if not owner_email: + owner_email, reviewers = _RietveldOwnerAndReviewers( + input_api, + owners_db.email_regexp, + approval_needed=input_api.is_committing) + + if owner_email: + reviewers_plus_owner = reviewers.union(set([owner_email])) + elif input_api.is_committing: return [output_api.PresubmitWarning( 'The issue was not uploaded so you have no OWNER approval.')] + else: + owner_email = '' + reviewers_plus_owner = set() - approvers_plus_owner = approvers.union(set([owner_email])) - - missing_files = owners_db.files_not_covered_by(affected_files, - approvers_plus_owner) - if missing_files: - return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' % - ','.join(missing_files))] + missing_directories = owners_db.directories_not_covered_by(affected_files, + reviewers_plus_owner) + if missing_directories: + return [output('Missing %s for files in these directories:\n %s' % + (needed, '\n '.join(missing_directories)))] - if not approvers: - return [output_api.PresubmitError('Missing LGTM from someone other than %s' - % owner_email)] + if input_api.is_committing and not reviewers: + return [output('Missing LGTM from someone other than %s' % owner_email)] return [] -def _RietveldOwnerAndApprovers(input_api, email_regexp): - """Return the owner and approvers of a change, if any.""" +def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False): + """Return the owner and reviewers of a change, if any. + + If approval_needed is True, only reviewers who have approved the change + will be returned. + """ if not input_api.change.issue: return None, None issue_props = input_api.rietveld.get_issue_properties( int(input_api.change.issue), True) + if not approval_needed: + return issue_props['owner_email'], set(issue_props['reviewers']) + owner_email = issue_props['owner_email'] def match_reviewer(r): diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index c9a0f3f3d4..22629418bb 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -1,5 +1,5 @@ #!/usr/bin/env python -# Copyright (c) 2011 The Chromium Authors. All rights reserved. +# Copyright (c) 2012 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. @@ -44,6 +44,9 @@ def test_repo(): '/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/views/OWNERS': owners_file(ben, john, owners.EVERYONE, noparent=True), }) @@ -65,69 +68,67 @@ class OwnersDatabaseTest(unittest.TestCase): def test_constructor(self): self.assertNotEquals(self.db(), None) - def assert_covered_by(self, files, reviewers): + def test_dirs_not_covered_by__valid_inputs(self): db = self.db() - self.assertTrue(db.files_are_covered_by(set(files), set(reviewers))) - def test_covered_by__everyone(self): - self.assert_covered_by(['DEPS'], [john]) - self.assert_covered_by(['DEPS'], [darin]) - - def test_covered_by__explicit(self): - self.assert_covered_by(['content/content.gyp'], [john]) - self.assert_covered_by(['chrome/gpu/OWNERS'], [ken]) - - def test_covered_by__owners_plus_everyone(self): - self.assert_covered_by(['/content/views/OWNERS'], [ben]) - self.assert_covered_by(['/content/views/OWNERS'], [ken]) + # Check that we're passed in a sequence that isn't a string. + self.assertRaises(AssertionError, db.directories_not_covered_by, 'foo', []) + if hasattr(owners.collections, 'Iterable'): + self.assertRaises(AssertionError, db.directories_not_covered_by, + (f for f in ['x', 'y']), []) - def test_covered_by__owners_propagates_down(self): - self.assert_covered_by(['chrome/gpu/OWNERS'], [ben]) + # Check that the files are under the root. + db.root = '/checkout' + self.assertRaises(AssertionError, db.directories_not_covered_by, + ['/OWNERS'], []) + db.root = '/' - def test_covered_by__no_file_in_dir(self): - self.assert_covered_by(['/chrome/renderer/gpu/gpu_channel_host.h'], [peter]) + # Check invalid email address. + self.assertRaises(AssertionError, db.directories_not_covered_by, + ['OWNERS'], ['foo']) - def assert_not_covered_by(self, files, reviewers, unreviewed_files): + def assert_dirs_not_covered_by(self, files, reviewers, unreviewed_dirs): db = self.db() - self.assertEquals(db.files_not_covered_by(set(files), set(reviewers)), - set(unreviewed_files)) + self.assertEquals( + db.directories_not_covered_by(set(files), set(reviewers)), + set(unreviewed_dirs)) - def test_not_covered_by__need_at_least_one_reviewer(self): - self.assert_not_covered_by(['DEPS'], [], ['DEPS']) - - def test_not_covered_by__owners_propagates_down(self): - self.assert_not_covered_by( + def test_dirs_not_covered_by__owners_propagates_down(self): + self.assert_dirs_not_covered_by( ['chrome/gpu/gpu_channel.h', 'chrome/renderer/gpu/gpu_channel_host.h'], [ben], []) - def test_not_covered_by__partial_covering(self): - self.assert_not_covered_by( + def test_dirs_not_covered_by__partial_covering(self): + self.assert_dirs_not_covered_by( ['content/content.gyp', 'chrome/renderer/gpu/gpu_channel_host.h'], - [peter], ['content/content.gyp']) - - def test_not_covered_by__set_noparent_works(self): - self.assert_not_covered_by(['content/content.gyp'], [ben], - ['content/content.gyp']) + [peter], ['content']) - def test_not_covered_by__valid_inputs(self): - db = self.db() - - # Check that we're passed in a sequence that isn't a string. - self.assertRaises(AssertionError, db.files_not_covered_by, 'foo', []) - if hasattr(owners.collections, 'Iterable'): - self.assertRaises(AssertionError, db.files_not_covered_by, - (f for f in ['x', 'y']), []) - - # Check that the files are under the root. - db.root = '/checkout' - self.assertRaises(AssertionError, db.files_not_covered_by, ['/OWNERS'], - []) - db.root = '/' - - # Check invalid email address. - self.assertRaises(AssertionError, db.files_not_covered_by, ['OWNERS'], - ['foo']) + def test_dirs_not_covered_by__set_noparent_works(self): + self.assert_dirs_not_covered_by(['content/content.gyp'], [ben], + ['content']) + def test_dirs_not_covered_by__no_reviewer(self): + self.assert_dirs_not_covered_by( + ['content/content.gyp', 'chrome/renderer/gpu/gpu_channel_host.h'], + [], ['content']) + + def test_dirs_not_covered_by__combines_directories(self): + self.assert_dirs_not_covered_by(['content/content.gyp', + 'content/bar/foo.cc', + 'chrome/renderer/gpu/gpu_channel_host.h'], + [peter], + ['content']) + + def test_dirs_not_covered_by__multiple_directories(self): + self.assert_dirs_not_covered_by( + ['content/content.gyp', # Not covered + 'content/bar/foo.cc', # Not covered (combines in) + 'content/baz/froboz.h', # Not covered + 'chrome/gpu/gpu_channel.h', # Owned by ken + 'chrome/renderer/gpu/gpu_channel_host.h' # Owned by * via parent + ], + [ken], + ['content', 'content/baz']) def assert_reviewers_for(self, files, expected_reviewers): db = self.db() diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 1663c32635..84585e9932 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2109,11 +2109,15 @@ class CannedChecksUnittest(PresubmitTestsBase): presubmit.OutputApi.PresubmitNotifyResult) def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, - rietveld_response=None, uncovered_files=None, expected_output=''): + reviewers=None, is_committing=True, rietveld_response=None, + uncovered_dirs=None, expected_output=''): if approvers is None: approvers = set() - if uncovered_files is None: - uncovered_files = set() + if reviewers is None: + reviewers = set() + reviewers = reviewers.union(approvers) + if uncovered_dirs is None: + uncovered_dirs = set() change = self.mox.CreateMock(presubmit.Change) change.issue = issue @@ -2122,11 +2126,11 @@ class CannedChecksUnittest(PresubmitTestsBase): 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 = True + input_api.is_committing = is_committing input_api.tbr = tbr - if not tbr and issue: - affected_file.LocalPath().AndReturn('foo.cc') + if not is_committing or (not tbr and issue): + affected_file.LocalPath().AndReturn('foo/xyz.cc') change.AffectedFiles(file_filter=None).AndReturn([affected_file]) owner_email = 'john@example.com' if not rietveld_response: @@ -2136,11 +2140,21 @@ class CannedChecksUnittest(PresubmitTestsBase): {"sender": a, "text": "I approve", "approval": True} for a in approvers ], + "reviewers": reviewers } - input_api.rietveld.get_issue_properties( - int(input_api.change.issue), True).AndReturn(rietveld_response) - fake_db.files_not_covered_by(set(['foo.cc']), - approvers.union(set([owner_email]))).AndReturn(uncovered_files) + + if is_committing: + people = approvers + else: + people = reviewers + + if issue: + input_api.rietveld.get_issue_properties( + int(input_api.change.issue), True).AndReturn(rietveld_response) + people.add(owner_email) + + fake_db.directories_not_covered_by(set(['foo/xyz.cc']), + people).AndReturn(uncovered_dirs) self.mox.ReplayAll() output = presubmit.PresubmitOutput() @@ -2158,11 +2172,17 @@ class CannedChecksUnittest(PresubmitTestsBase): "sender": "ben@example.com", "text": "foo", "approval": True, }, ], + "reviewers": ["ben@example.com"], } self.AssertOwnersWorks(approvers=set(['ben@example.com']), rietveld_response=response, expected_output='') + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + is_committing=False, + rietveld_response=response, + expected_output='') + def testCannedCheckOwners_NotApproved(self): response = { "owner_email": "john@example.com", @@ -2171,46 +2191,89 @@ class CannedChecksUnittest(PresubmitTestsBase): "sender": "ben@example.com", "text": "foo", "approval": False, }, ], + "reviewers": ["ben@example.com"], } self.AssertOwnersWorks( approvers=set(), + reviewers=set(["ben@example.com"]), rietveld_response=response, expected_output= 'Missing LGTM from someone other than john@example.com\n') + self.AssertOwnersWorks( + approvers=set(), + reviewers=set(["ben@example.com"]), + is_committing=False, + rietveld_response=response, + expected_output='') + + def testCannedCheckOwners_NoReviewers(self): + response = { + "owner_email": "john@example.com", + "messages": [ + { + "sender": "ben@example.com", "text": "foo", "approval": False, + }, + ], + "reviewers":[], + } + self.AssertOwnersWorks( + approvers=set(), + reviewers=set(), + rietveld_response=response, + expected_output= + 'Missing LGTM from someone other than john@example.com\n') + + self.AssertOwnersWorks( + approvers=set(), + reviewers=set(), + is_committing=False, + rietveld_response=response, + expected_output='') + def testCannedCheckOwners_NoIssue(self): self.AssertOwnersWorks(issue=None, expected_output="OWNERS check failed: this change has no Rietveld " "issue number, so we can't check it for approvals.\n") + self.AssertOwnersWorks(issue=None, is_committing=False, + expected_output="") + def testCannedCheckOwners_NoLGTM(self): self.AssertOwnersWorks(expected_output='Missing LGTM from someone ' 'other than john@example.com\n') + self.AssertOwnersWorks(is_committing=False, expected_output='') def testCannedCheckOwners_OnlyOwnerLGTM(self): self.AssertOwnersWorks(approvers=set(['john@example.com']), expected_output='Missing LGTM from someone ' 'other than john@example.com\n') + self.AssertOwnersWorks(approvers=set(['john@example.com']), + is_committing=False, + expected_output='') def testCannedCheckOwners_TBR(self): self.AssertOwnersWorks(tbr=True, expected_output='--tbr was specified, skipping OWNERS check\n') - - def testCannedCheckOwners_Upload(self): - class FakeInputAPI(object): - is_committing = False - - results = presubmit_canned_checks.CheckOwners(FakeInputAPI(), - presubmit.OutputApi) - self.assertEqual(results, []) + self.AssertOwnersWorks(tbr=True, is_committing=False, expected_output='') def testCannedCheckOwners_WithoutOwnerLGTM(self): - self.AssertOwnersWorks(uncovered_files=set(['foo.cc']), - expected_output='Missing LGTM from an OWNER for: foo.cc\n') + self.AssertOwnersWorks(uncovered_dirs=set(['foo']), + expected_output='Missing LGTM from an OWNER for files in these ' + 'directories:\n' + ' foo\n') + self.AssertOwnersWorks(uncovered_dirs=set(['foo']), + is_committing=False, + expected_output='Missing OWNER reviewers for files in these ' + 'directories:\n' + ' foo\n') def testCannedCheckOwners_WithLGTMs(self): self.AssertOwnersWorks(approvers=set(['ben@example.com']), - uncovered_files=set()) + uncovered_dirs=set()) + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + is_committing=False, + uncovered_dirs=set()) def testCannedRunUnitTests(self): change = presubmit.Change( @@ -2299,7 +2362,7 @@ class CannedChecksUnittest(PresubmitTestsBase): text_files=None, license_header=None, project_name=None, - owners_check=True) + owners_check=False) self.assertEqual(1, len(results)) self.assertEqual( 'Found line ending with white spaces in:', results[0]._message)