Show a list of directories missing OWNER reviewers on upload, and show directories rather than files missing OWNER approvals on commit.

BUG=117166
TEST=covered by presubmit unittests

Review URL: http://codereview.chromium.org/9621012

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

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)

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

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

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)