From 0a2bb37a5fc1e50efc1c17c7e32867a67876b822 Mon Sep 17 00:00:00 2001 From: "dpranke@chromium.org" Date: Fri, 25 Mar 2011 01:16:22 +0000 Subject: [PATCH] improve logging slightly Review URL: http://codereview.chromium.org/6736018 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@79356 0039d316-1c4b-4281-b951-d872f2087c98 --- git_cl/git_cl.py | 16 ++++------------ presubmit_canned_checks.py | 8 +++++--- presubmit_support.py | 14 ++++++++++---- tests/presubmit_unittest.py | 22 +++++++++++++++------- 4 files changed, 34 insertions(+), 26 deletions(-) diff --git a/git_cl/git_cl.py b/git_cl/git_cl.py index 218c9f3da..665e2a0c5 100644 --- a/git_cl/git_cl.py +++ b/git_cl/git_cl.py @@ -840,18 +840,10 @@ def CMDpresubmit(parser, args): # Default to diffing against the "upstream" branch. base_branch = cl.GetUpstreamBranch() - if options.upload: - print '*** Presubmit checks for UPLOAD would report: ***' - RunHook(committing=False, upstream_branch=base_branch, - rietveld_server=cl.GetRietveldServer(), tbr=False, - may_prompt=False) - return 0 - else: - print '*** Presubmit checks for DCOMMIT would report: ***' - RunHook(committing=True, upstream_branch=base_branch, - rietveld_server=cl.GetRietveldServer, tbr=False, - may_prompt=False) - return 0 + RunHook(committing=not options.upload, upstream_branch=base_branch, + rietveld_server=cl.GetRietveldServer(), tbr=False, + may_prompt=False) + return 0 @usage('[args to "git diff"]') diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 2adc371a8..2a868a575 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -631,10 +631,12 @@ 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')] + return [output_api.PresubmitNotifyResult( + '--tbr was specified, skipping OWNERS check')] if not input_api.change.issue: - return [output_api.PresubmitError('Change not uploaded for review')] + return [output_api.PresubmitError( + "OWNERS check failed: this change has no Rietveld issue number, so " + "we can't check it for approvals.")] affected_files = set([f.LocalPath() for f in input_api.change.AffectedFiles(source_file_filter)]) diff --git a/presubmit_support.py b/presubmit_support.py index e15b634f8..c8b4a460c 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1040,7 +1040,10 @@ def DoPresubmitChecks(change, if there were errors or warnings and the caller should abort. """ output = PresubmitOutput(input_stream, output_stream) - output.write("Running presubmit hooks...\n") + if committing: + output.write("Running presubmit commit checks ...\n") + else: + output.write("Running presubmit upload checks ...\n") start_time = time.time() presubmit_files = ListRelevantPresubmitFiles(change.AbsoluteLocalPaths(True), change.RepositoryRoot()) @@ -1072,6 +1075,7 @@ def DoPresubmitChecks(change, else: notifications.append(result) + output.write('\n') for name, items in (('Messages', notifications), ('Warnings', warnings), ('ERRORS', errors)): @@ -1083,10 +1087,12 @@ def DoPresubmitChecks(change, total_time = time.time() - start_time if total_time > 1.0: - output.write("Presubmit checks took %.1fs to calculate.\n" % total_time) + output.write("Presubmit checks took %.1fs to calculate.\n\n" % total_time) - if not errors and warnings: - if may_prompt: + if not errors: + if not warnings: + output.write('Presubmit checks passed.\n') + elif may_prompt: output.prompt_yes_no('There were presubmit warnings. ' 'Are you sure you wish to continue? (y/N): ') else: diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index d47e1d0b1..b420a59e9 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -420,7 +420,8 @@ class PresubmitUnittest(PresubmitTestsBase): change, False, True, None, input_buf, None, False) self.failIf(output.should_continue()) self.assertEqual(output.getvalue().count('!!'), 2) - self.assertEqual(output.getvalue().count('Running presubmit hooks...\n'), 1) + self.assertEqual(output.getvalue().count( + 'Running presubmit upload checks ...\n'), 1) def testDoPresubmitChecksPromptsAfterWarnings(self): join = presubmit.os.path.join @@ -459,7 +460,8 @@ class PresubmitUnittest(PresubmitTestsBase): change, False, True, None, input_buf, None, True) self.failUnless(output.should_continue()) self.assertEquals(output.getvalue().count('??'), 2) - self.assertEqual(output.getvalue().count('Running presubmit hooks...\n'), 1) + self.assertEqual(output.getvalue().count( + 'Running presubmit upload checks ...\n'), 1) def testDoPresubmitChecksNoWarningPromptIfErrors(self): join = presubmit.os.path.join @@ -492,7 +494,8 @@ class PresubmitUnittest(PresubmitTestsBase): self.assertEqual(output.getvalue().count('??'), 2) self.assertEqual(output.getvalue().count('XX!!XX'), 2) self.assertEqual(output.getvalue().count('(y/N)'), 0) - self.assertEqual(output.getvalue().count('Running presubmit hooks...\n'), 1) + self.assertEqual(output.getvalue().count( + 'Running presubmit upload checks ...\n'), 1) def testDoDefaultPresubmitChecksAndFeedback(self): join = presubmit.os.path.join @@ -526,9 +529,10 @@ def CheckChangeOnCommit(input_api, output_api): output = presubmit.DoPresubmitChecks( change, False, True, None, input_buf, DEFAULT_SCRIPT, False) self.failIf(output.should_continue()) - text = ('Running presubmit hooks...\n' + text = ('Running presubmit upload checks ...\n' 'Warning, no presubmit.py found.\n' 'Running default presubmit script.\n' + '\n' '** Presubmit ERRORS **\n!!\n\n' 'Was the presubmit check useful? Please send feedback & hate mail ' 'to maruel@chromium.org!\n') @@ -600,11 +604,14 @@ def CheckChangeOnCommit(input_api, output_api): self.failUnless(presubmit.DoPresubmitChecks( change, False, True, output, input_buf, DEFAULT_SCRIPT, False)) self.assertEquals(output.getvalue(), - ('Running presubmit hooks...\n' + ('Running presubmit upload checks ...\n' 'Warning, no presubmit.py found.\n' 'Running default presubmit script.\n' + '\n' '** Presubmit Messages **\n' - 'http://tracker.com/42\n\n')) + 'http://tracker.com/42\n' + '\n' + 'Presubmit checks passed.\n')) def testGetTrySlavesExecuter(self): self.mox.ReplayAll() @@ -1971,7 +1978,8 @@ mac|success|blew def testCannedCheckOwners_NoIssue(self): self.AssertOwnersWorks(issue=None, - expected_output='Change not uploaded for review\n') + expected_output="OWNERS check failed: this change has no Rietveld " + "issue number, so we can't check it for approvals.\n") def testCannedCheckOwners_NoLGTM(self): self.AssertOwnersWorks(expected_output='Missing LGTM from someone '