diff --git a/owners.py b/owners.py index 6a48f43ab..a8a3bfd71 100644 --- a/owners.py +++ b/owners.py @@ -4,6 +4,7 @@ """A database of OWNERS files.""" +import collections import re @@ -58,9 +59,9 @@ class Database(object): self.stop_looking = set(['']) def reviewers_for(self, files): - """Returns a suggested set of reviewers that will cover the set of files. + """Returns a suggested set of reviewers that will cover the files. - files is a set of paths relative to (and under) self.root.""" + files is a sequence of paths relative to (and under) self.root.""" self._check_paths(files) self._load_data_needed_for(files) return self._covering_set_of_owners_for(files) @@ -70,7 +71,11 @@ class Database(object): return not self.files_not_covered_by(files, reviewers) def files_not_covered_by(self, files, reviewers): - """Returns the set of files that are not owned by at least one reviewer.""" + """Returns the set of files that are not owned by at least one reviewer. + + Args: + files is a sequence of paths relative to (and under) self.root. + reviewers is a sequence of strings matching self.email_regexp.""" self._check_paths(files) self._check_reviewers(reviewers) if not reviewers: @@ -85,13 +90,19 @@ class Database(object): uncovered_files.extend(files_in_d) return set(uncovered_files) + def _check_collection(self, obj): + assert (isinstance(obj, collections.Iterable) and + isinstance(obj, collections.Sized) and + not isinstance(obj, basestring)) + def _check_paths(self, files): def _is_under(f, pfx): return self.os_path.abspath(self.os_path.join(pfx, f)).startswith(pfx) + self._check_collection(files) assert all(_is_under(f, self.os_path.abspath(self.root)) for f in files) def _check_reviewers(self, reviewers): - """Verifies each reviewer is a valid email address.""" + self._check_collection(reviewers) assert all(self.email_regexp.match(r) for r in reviewers) def _files_by_dir(self, files): diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index ea571636b..de24a7b7c 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -98,6 +98,25 @@ class OwnersDatabaseTest(unittest.TestCase): self.assert_not_covered_by(['content/content.gyp'], [ben], ['content/content.gyp']) + 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', []) + 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 assert_reviewers_for(self, files, expected_reviewers): db = self.db() self.assertEquals(db.reviewers_for(set(files)), set(expected_reviewers)) @@ -109,6 +128,17 @@ class OwnersDatabaseTest(unittest.TestCase): def test_reviewers_for__set_noparent_works(self): self.assert_reviewers_for(['content/content.gyp'], [john, darin]) + def test_reviewers_for__valid_inputs(self): + db = self.db() + + # Check that we're passed in a sequence that isn't a string. + self.assertRaises(AssertionError, db.reviewers_for, 'foo') + self.assertRaises(AssertionError, db.reviewers_for, (f for f in ['x', 'y'])) + + # Check that the files are under the root. + db.root = '/checkout' + self.assertRaises(AssertionError, db.reviewers_for, ['/OWNERS']) + def test_reviewers_for__wildcard_dir(self): self.assert_reviewers_for(['DEPS'], [owners.EVERYONE])