Fix the owners implementation to validate method inputs.

R=chase@chromium.org,maruel@chromium.org
Review URL: http://codereview.chromium.org/6677090

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@78454 0039d316-1c4b-4281-b951-d872f2087c98

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):

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])