From d0c226a0d3e16af1a5e40dc127373413d0252328 Mon Sep 17 00:00:00 2001 From: Clemens Hammacher Date: Mon, 16 Jan 2017 14:09:52 +0100 Subject: [PATCH] Output missing LGTMs notification also on dry-run On larger CLs, this can be used to see whether all needed LGTMs are there on a dry run. R=tandrii@chromium.org Change-Id: I5fe4022feaca73d08ead9aed14ca270192740675 Reviewed-on: https://chromium-review.googlesource.com/426819 Commit-Queue: Andrii Shyshkalov Reviewed-by: Andrii Shyshkalov Reviewed-by: smut --- presubmit_canned_checks.py | 19 ++++++++++--------- tests/presubmit_unittest.py | 38 ++++++++++++++++++------------------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 8f2e1a253..37c5143be 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -858,18 +858,19 @@ def CheckOwners(input_api, output_api, source_file_filter=None): if input_api.tbr: return [output_api.PresubmitNotifyResult( '--tbr was specified, skipping OWNERS check')] + needed = 'LGTM from an OWNER' + output_fn = output_api.PresubmitError if input_api.change.issue: if input_api.dry_run: - return [output_api.PresubmitNotifyResult( - 'This is a dry run, skipping OWNERS check')] + output_fn = lambda text: output_api.PresubmitNotifyResult( + 'This is a dry run, but these failures would be reported on ' + + 'commit:\n' + text) else: 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 + output_fn = output_api.PresubmitNotifyResult affected_files = set([f.LocalPath() for f in input_api.AffectedFiles(file_filter=source_file_filter)]) @@ -891,17 +892,17 @@ def CheckOwners(input_api, output_api, source_file_filter=None): if missing_files: output_list = [ - output('Missing %s for these files:\n %s' % - (needed, '\n '.join(sorted(missing_files))))] + output_fn('Missing %s for these files:\n %s' % + (needed, '\n '.join(sorted(missing_files))))] if not input_api.is_committing: suggested_owners = owners_db.reviewers_for(missing_files, owner_email) - output_list.append(output('Suggested OWNERS: ' + + output_list.append(output_fn('Suggested OWNERS: ' + '(Use "git-cl owners" to interactively select owners.)\n %s' % ('\n '.join(suggested_owners or [])))) return output_list if input_api.is_committing and not reviewers: - return [output('Missing LGTM from someone other than %s' % owner_email)] + return [output_fn('Missing LGTM from someone other than %s' % owner_email)] return [] def GetCodereviewOwnerAndReviewers(input_api, email_regexp, approval_needed): diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index c078c3212..2c036e27e 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2194,9 +2194,8 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api.dry_run = dry_run if not is_committing or (not tbr and issue): - if not dry_run: - affected_file.LocalPath().AndReturn('foo/xyz.cc') - input_api.AffectedFiles(file_filter=None).AndReturn([affected_file]) + affected_file.LocalPath().AndReturn('foo/xyz.cc') + input_api.AffectedFiles(file_filter=None).AndReturn([affected_file]) if issue and not rietveld_response and not gerrit_response: rietveld_response = { "owner_email": change.author_email, @@ -2212,21 +2211,20 @@ class CannedChecksUnittest(PresubmitTestsBase): else: people = reviewers - if not dry_run: - if issue: - if rietveld_response: - input_api.rietveld.get_issue_properties( - issue=int(input_api.change.issue), messages=True).AndReturn( - rietveld_response) - elif gerrit_response: - input_api.gerrit._FetchChangeDetail = lambda _: gerrit_response - - people.add(change.author_email) - 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']), - change.author_email).AndReturn(change.author_email) + if issue: + if rietveld_response: + input_api.rietveld.get_issue_properties( + issue=int(input_api.change.issue), messages=True).AndReturn( + rietveld_response) + elif gerrit_response: + input_api.gerrit._FetchChangeDetail = lambda _: gerrit_response + + people.add(change.author_email) + 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']), + change.author_email).AndReturn(change.author_email) self.mox.ReplayAll() output = presubmit.PresubmitOutput() @@ -2245,7 +2243,9 @@ class CannedChecksUnittest(PresubmitTestsBase): dry_run=True, rietveld_response=response, reviewers=set(["ben@example.com"]), - expected_output='This is a dry run, skipping OWNERS check\n') + expected_output='This is a dry run, but these failures would be ' + + 'reported on commit:\nMissing LGTM from someone ' + + 'other than john@example.com\n') self.AssertOwnersWorks(approvers=set(['ben@example.com']), is_committing=False,