diff --git a/owners.py b/owners.py index 8007e171c..c53f0123d 100644 --- a/owners.py +++ b/owners.py @@ -129,13 +129,8 @@ class Database(object): suggested_owners = set(['']) return suggested_owners - # TODO(dpranke): rename to objects_not_covered_by - def directories_not_covered_by(self, files, reviewers): - """Returns the set of directories that are not owned by a 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. + def files_not_covered_by(self, files, reviewers): + """Returns the files not owned by one of the reviewers. Args: files is a sequence of paths relative to (and under) self.root. @@ -145,20 +140,11 @@ class Database(object): self._check_reviewers(reviewers) self._load_data_needed_for(files) - objs = set() - for f in files: - if f in self.owners_for: - objs.add(f) - else: - objs.add(self.os_path.dirname(f)) - covered_objs = self._objs_covered_by(reviewers) - uncovered_objs = [self._enclosing_obj_with_owners(o) for o in objs - if not self._is_obj_covered_by(o, covered_objs)] + uncovered_files = [f for f in files + if not self._is_obj_covered_by(f, covered_objs)] - return set(uncovered_objs) - - objects_not_covered_by = directories_not_covered_by + return set(uncovered_files) def _check_paths(self, files): def _is_under(f, pfx): @@ -171,38 +157,29 @@ class Database(object): _assert_is_collection(reviewers) assert all(self.email_regexp.match(r) for r in reviewers) - # TODO(dpranke): Rename to _objs_covered_by and update_callers - def _dirs_covered_by(self, reviewers): - dirs = self.owned_by[EVERYONE] + def _objs_covered_by(self, reviewers): + objs = self.owned_by[EVERYONE] for r in reviewers: - dirs = dirs | self.owned_by.get(r, set()) - return dirs - - _objs_covered_by = _dirs_covered_by + objs = objs | self.owned_by.get(r, set()) + return objs - def _stop_looking(self, dirname): - return dirname in self.stop_looking + def _stop_looking(self, objname): + return objname in self.stop_looking - # TODO(dpranke): Rename to _is_dir_covered_by and update callers. - def _is_dir_covered_by(self, dirname, covered_dirs): - while not dirname in covered_dirs and not self._stop_looking(dirname): - dirname = self.os_path.dirname(dirname) - return dirname in covered_dirs + def _is_obj_covered_by(self, objname, covered_objs): + while not objname in covered_objs and not self._stop_looking(objname): + objname = self.os_path.dirname(objname) + return objname in covered_objs - _is_obj_covered_by = _is_dir_covered_by - - # TODO(dpranke): Rename to _enclosing_obj_with_owners and update callers. - def _enclosing_dir_with_owners(self, directory): + def _enclosing_dir_with_owners(self, objname): """Returns the innermost enclosing directory that has an OWNERS file.""" - dirpath = directory + dirpath = objname while not dirpath in self.owners_for: if self._stop_looking(dirpath): break dirpath = self.os_path.dirname(dirpath) return dirpath - _enclosing_obj_with_owners = _enclosing_dir_with_owners - 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 f3f757a9e..2a6f8fd38 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -797,16 +797,16 @@ def CheckOwners(input_api, output_api, source_file_filter=None): reviewers_plus_owner = reviewers.union(set([owner_email])) else: message = ('\nUntil the issue is uploaded, this list will include ' - 'directories for which you \nare an OWNER.') + 'files for which you are an OWNER.') owner_email = '' reviewers_plus_owner = set() - missing_directories = owners_db.directories_not_covered_by(affected_files, - reviewers_plus_owner) - if missing_directories: + missing_files = owners_db.files_not_covered_by(affected_files, + reviewers_plus_owner) + if missing_files: output_list = [ - output('Missing %s for files in these directories:\n %s%s' % - (needed, '\n '.join(missing_directories), message))] + output('Missing %s for these files:\n %s%s' % + (needed, '\n '.join(missing_files), message))] if not input_api.is_committing: suggested_owners = owners_db.reviewers_for(affected_files) output_list.append(output('Suggested OWNERS:\n %s' % diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index 20491ab4d..d860b4397 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -80,59 +80,59 @@ class OwnersDatabaseTest(_BaseTestCase): def test_constructor(self): self.assertNotEquals(self.db(), None) - def test_dirs_not_covered_by__valid_inputs(self): + def test_files_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.directories_not_covered_by, 'foo', []) + self.assertRaises(AssertionError, db.files_not_covered_by, 'foo', []) if hasattr(owners.collections, 'Iterable'): - self.assertRaises(AssertionError, db.directories_not_covered_by, + 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.directories_not_covered_by, + self.assertRaises(AssertionError, db.files_not_covered_by, ['/OWNERS'], []) db.root = '/' # Check invalid email address. - self.assertRaises(AssertionError, db.directories_not_covered_by, + self.assertRaises(AssertionError, db.files_not_covered_by, ['OWNERS'], ['foo']) - def assert_dirs_not_covered_by(self, files, reviewers, unreviewed_dirs): + def assert_files_not_covered_by(self, files, reviewers, unreviewed_files): db = self.db() - self.assertEquals( - db.directories_not_covered_by(set(files), set(reviewers)), - set(unreviewed_dirs)) + self.assertEquals(db.files_not_covered_by(set(files), set(reviewers)), + set(unreviewed_files)) - def test_dirs_not_covered_by__owners_propagates_down(self): - self.assert_dirs_not_covered_by( + def test_files_not_covered_by__owners_propagates_down(self): + self.assert_files_not_covered_by( ['chrome/gpu/gpu_channel.h', 'chrome/renderer/gpu/gpu_channel_host.h'], [ben], []) - def test_dirs_not_covered_by__partial_covering(self): - self.assert_dirs_not_covered_by( + def test_files_not_covered_by__partial_covering(self): + self.assert_files_not_covered_by( ['content/content.gyp', 'chrome/renderer/gpu/gpu_channel_host.h'], - [peter], ['content']) + [peter], ['content/content.gyp']) - def test_dirs_not_covered_by__set_noparent_works(self): - self.assert_dirs_not_covered_by(['content/content.gyp'], [ben], - ['content']) + def test_files_not_covered_by__set_noparent_works(self): + self.assert_files_not_covered_by(['content/content.gyp'], [ben], + ['content/content.gyp']) - def test_dirs_not_covered_by__no_reviewer(self): - self.assert_dirs_not_covered_by( + def test_files_not_covered_by__no_reviewer(self): + self.assert_files_not_covered_by( ['content/content.gyp', 'chrome/renderer/gpu/gpu_channel_host.h'], - [], ['content']) + [], ['content/content.gyp']) - def test_dirs_not_covered_by__combines_directories(self): - self.assert_dirs_not_covered_by(['content/content.gyp', + def test_files_not_covered_by__combines_directories(self): + self.assert_files_not_covered_by(['content/content.gyp', 'content/bar/foo.cc', 'chrome/renderer/gpu/gpu_channel_host.h'], [peter], - ['content']) + ['content/content.gyp', + 'content/bar/foo.cc']) - def test_dirs_not_covered_by__multiple_directories(self): - self.assert_dirs_not_covered_by( + def test_files_not_covered_by__multiple_directories(self): + self.assert_files_not_covered_by( ['content/content.gyp', # Not covered 'content/bar/foo.cc', # Not covered (combines in) 'content/baz/froboz.h', # Not covered @@ -140,23 +140,23 @@ class OwnersDatabaseTest(_BaseTestCase): 'chrome/renderer/gpu/gpu_channel_host.h' # Owned by * via parent ], [ken], - ['content', 'content/baz']) + ['content/content.gyp', 'content/bar/foo.cc', 'content/baz/froboz.h']) def test_per_file(self): # brett isn't allowed to approve ugly.cc self.files['/content/baz/OWNERS'] = owners_file(brett, lines=['per-file ugly.*=tom@example.com']) - self.assert_dirs_not_covered_by(['content/baz/ugly.cc'], + self.assert_files_not_covered_by(['content/baz/ugly.cc'], [brett], []) # tom is allowed to approve ugly.cc, but not froboz.h - self.assert_dirs_not_covered_by(['content/baz/ugly.cc'], + self.assert_files_not_covered_by(['content/baz/ugly.cc'], [tom], []) - self.assert_dirs_not_covered_by(['content/baz/froboz.h'], + self.assert_files_not_covered_by(['content/baz/froboz.h'], [tom], - ['content/baz']) + ['content/baz/froboz.h']) def test_per_file_with_spaces(self): # This is the same as test_per_file(), except that we include spaces @@ -164,16 +164,16 @@ class OwnersDatabaseTest(_BaseTestCase): # tom is allowed to approve ugly.cc, but not froboz.h self.files['/content/baz/OWNERS'] = owners_file(brett, lines=['per-file ugly.* = tom@example.com']) - self.assert_dirs_not_covered_by(['content/baz/ugly.cc'], + self.assert_files_not_covered_by(['content/baz/ugly.cc'], [brett], []) - self.assert_dirs_not_covered_by(['content/baz/ugly.cc'], + self.assert_files_not_covered_by(['content/baz/ugly.cc'], [tom], []) - self.assert_dirs_not_covered_by(['content/baz/froboz.h'], + self.assert_files_not_covered_by(['content/baz/froboz.h'], [tom], - ['content/baz']) + ['content/baz/froboz.h']) def test_per_file__set_noparent(self): self.files['/content/baz/OWNERS'] = owners_file(brett, @@ -181,22 +181,22 @@ class OwnersDatabaseTest(_BaseTestCase): 'per-file ugly.*=set noparent']) # brett isn't allowed to approve ugly.cc - self.assert_dirs_not_covered_by(['content/baz/ugly.cc'], + self.assert_files_not_covered_by(['content/baz/ugly.cc'], [brett], ['content/baz/ugly.cc']) # tom is allowed to approve ugly.cc, but not froboz.h - self.assert_dirs_not_covered_by(['content/baz/ugly.cc'], + self.assert_files_not_covered_by(['content/baz/ugly.cc'], [tom], []) - self.assert_dirs_not_covered_by(['content/baz/froboz.h'], + self.assert_files_not_covered_by(['content/baz/froboz.h'], [tom], - ['content/baz']) + ['content/baz/froboz.h']) def test_per_file_wildcard(self): self.files['/OWNERS'] = 'per-file DEPS=*\n' - self.assert_dirs_not_covered_by(['DEPS'], [brett], []) + self.assert_files_not_covered_by(['DEPS'], [brett], []) def test_mock_relpath(self): # This test ensures the mock relpath has the arguments in the right @@ -207,7 +207,7 @@ class OwnersDatabaseTest(_BaseTestCase): def test_per_file_glob_across_dirs_not_allowed(self): self.files['/OWNERS'] = 'per-file content/*=john@example.org\n' self.assertRaises(owners.SyntaxErrorInOwnersFile, - self.db().directories_not_covered_by, ['DEPS'], [brett]) + self.db().files_not_covered_by, ['DEPS'], [brett]) def assert_syntax_error(self, owners_file_contents): db = self.db() diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 7e7c170e2..0014c703d 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2219,14 +2219,14 @@ class CannedChecksUnittest(PresubmitTestsBase): def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, reviewers=None, is_committing=True, rietveld_response=None, - uncovered_dirs=None, expected_output=''): + uncovered_files=None, expected_output=''): if approvers is None: approvers = set() if reviewers is None: reviewers = set() reviewers = reviewers.union(approvers) - if uncovered_dirs is None: - uncovered_dirs = set() + if uncovered_files is None: + uncovered_files = set() change = self.mox.CreateMock(presubmit.Change) change.issue = issue @@ -2263,9 +2263,9 @@ class CannedChecksUnittest(PresubmitTestsBase): rietveld_response) people.add(owner_email) - fake_db.directories_not_covered_by(set(['foo/xyz.cc']), - people).AndReturn(uncovered_dirs) - if not is_committing and uncovered_dirs: + fake_db.files_not_covered_by(set(['foo/xyz.cc']), + people).AndReturn(uncovered_files) + if not is_committing and uncovered_files: fake_db.reviewers_for(set(['foo/xyz.cc'])).AndReturn(owner_email) self.mox.ReplayAll() @@ -2352,18 +2352,16 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCannedCheckOwners_NoIssue(self): self.AssertOwnersWorks(issue=None, - uncovered_dirs=set(['foo']), + uncovered_files=set(['foo']), 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, - uncovered_dirs=set(['foo']), - expected_output='Missing OWNER reviewers for files in these ' - 'directories:\n' + uncovered_files=set(['foo']), + expected_output='Missing OWNER reviewers for these files:\n' ' foo\n' 'Until the issue is uploaded, this list will include ' - 'directories for which you \n' - 'are an OWNER.\n') + 'files for which you are an OWNER.\n') def testCannedCheckOwners_NoLGTM(self): self.AssertOwnersWorks(expected_output='Missing LGTM from someone ' @@ -2384,22 +2382,20 @@ class CannedChecksUnittest(PresubmitTestsBase): self.AssertOwnersWorks(tbr=True, is_committing=False, expected_output='') def testCannedCheckOwners_WithoutOwnerLGTM(self): - self.AssertOwnersWorks(uncovered_dirs=set(['foo']), - expected_output='Missing LGTM from an OWNER for files in these ' - 'directories:\n' + self.AssertOwnersWorks(uncovered_files=set(['foo']), + expected_output='Missing LGTM from an OWNER for these files:\n' ' foo\n') - self.AssertOwnersWorks(uncovered_dirs=set(['foo']), - is_committing=False, - expected_output='Missing OWNER reviewers for files in these ' - 'directories:\n' + self.AssertOwnersWorks(uncovered_files=set(['foo']), + is_committing=False, + expected_output='Missing OWNER reviewers for these files:\n' ' foo\n') def testCannedCheckOwners_WithLGTMs(self): self.AssertOwnersWorks(approvers=set(['ben@example.com']), - uncovered_dirs=set()) + uncovered_files=set()) self.AssertOwnersWorks(approvers=set(['ben@example.com']), is_committing=False, - uncovered_dirs=set()) + uncovered_files=set()) def testCannedRunUnitTests(self): change = presubmit.Change(