From 5ae86d2021277b545889959125e778820849cf15 Mon Sep 17 00:00:00 2001 From: Jeremy Roman Date: Thu, 26 Apr 2018 15:36:02 -0400 Subject: [PATCH] Update canned PRESUBMIT checks to report that TBR does not apply to OWNERS. Expansion of the tests to check for this are required. Change-Id: Iae6294501659738df5097ce5a72da79e72818d98 Reviewed-on: https://chromium-review.googlesource.com/1030956 Reviewed-by: Aaron Gable Commit-Queue: Jeremy Roman --- presubmit_canned_checks.py | 6 +++++- tests/presubmit_unittest.py | 34 ++++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 584a70801d..946d7739d8 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -845,9 +845,10 @@ def CheckOwnersFormat(input_api, output_api): def CheckOwners(input_api, output_api, source_file_filter=None): affected_files = set([f.LocalPath() for f in input_api.change.AffectedFiles(file_filter=source_file_filter)]) + affects_owners = any('OWNERS' in name for name in affected_files) if input_api.is_committing: - if input_api.tbr and not any(['OWNERS' in name for name in affected_files]): + if input_api.tbr and not affects_owners: return [output_api.PresubmitNotifyResult( '--tbr was specified, skipping OWNERS check')] needed = 'LGTM from an OWNER' @@ -885,6 +886,9 @@ def CheckOwners(input_api, output_api, source_file_filter=None): output_list = [ output_fn('Missing %s for these files:\n %s' % (needed, '\n '.join(sorted(missing_files))))] + if input_api.tbr and affects_owners: + output_list.append(output_fn('Note that TBR does not apply to changes ' + 'that affect OWNERS files.')) if not input_api.is_committing: suggested_owners = owners_db.reviewers_for(missing_files, owner_email) owners_with_comments = [] diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index f4719859bf..47994f1c26 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -13,6 +13,7 @@ import itertools import logging import multiprocessing import os +import re import sys import time import unittest @@ -2507,15 +2508,18 @@ class CannedChecksUnittest(PresubmitTestsBase): change.OriginalOwnersFiles().AndReturn({}) if not is_committing and uncovered_files: fake_db.reviewers_for(set(['foo']), - change.author_email).AndReturn(change.author_email) + change.author_email).AndReturn([change.author_email]) self.mox.ReplayAll() output = presubmit.PresubmitOutput() results = presubmit_canned_checks.CheckOwners(input_api, presubmit.OutputApi) - if results: - results[0].handle(output) - self.assertEquals(output.getvalue(), expected_output) + for result in results: + result.handle(output) + if isinstance(expected_output, re._pattern_type): + self.assertRegexpMatches(output.getvalue(), expected_output) + else: + self.assertEquals(output.getvalue(), expected_output) def testCannedCheckOwners_DryRun(self): response = { @@ -2710,8 +2714,9 @@ class CannedChecksUnittest(PresubmitTestsBase): self.AssertOwnersWorks(issue=None, is_committing=False, uncovered_files=set(['foo']), - expected_output='Missing OWNER reviewers for these files:\n' - ' foo\n') + expected_output=re.compile( + 'Missing OWNER reviewers for these files:\n' + ' foo\n', re.MULTILINE)) def testCannedCheckOwners_NoIssueLocalReviewers(self): self.AssertOwnersWorks(issue=None, @@ -2735,8 +2740,9 @@ class CannedChecksUnittest(PresubmitTestsBase): uncovered_files=set(['foo']), manually_specified_reviewers=['jane'], is_committing=False, - expected_output='Missing OWNER reviewers for these files:\n' - ' foo\n') + expected_output=re.compile( + 'Missing OWNER reviewers for these files:\n' + ' foo\n', re.MULTILINE)) def testCannedCheckOwners_NoLGTM(self): self.AssertOwnersWorks(expected_output='Missing LGTM from someone ' @@ -2760,8 +2766,11 @@ class CannedChecksUnittest(PresubmitTestsBase): self.AssertOwnersWorks( tbr=True, uncovered_files=set(['foo']), modified_file='foo/OWNERS', - expected_output='Missing LGTM from an OWNER for these files:\n' - ' foo\n') + expected_output=re.compile( + 'Missing LGTM from an OWNER for these files:\n' + ' foo\n' + '.*TBR does not apply to changes that affect OWNERS files.', + re.MULTILINE)) def testCannedCheckOwners_WithoutOwnerLGTM(self): self.AssertOwnersWorks(uncovered_files=set(['foo']), @@ -2769,8 +2778,9 @@ class CannedChecksUnittest(PresubmitTestsBase): ' foo\n') self.AssertOwnersWorks(uncovered_files=set(['foo']), is_committing=False, - expected_output='Missing OWNER reviewers for these files:\n' - ' foo\n') + expected_output=re.compile( + 'Missing OWNER reviewers for these files:\n' + ' foo\n', re.MULTILINE)) def testCannedCheckOwners_WithLGTMs(self): self.AssertOwnersWorks(approvers=set(['ben@example.com']),