From ce8e46b3b2cfd98f3320b1c0c08c26d3eebe1db6 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Fri, 26 Jun 2009 22:31:51 +0000 Subject: [PATCH] Ask for feedback one time out of 5, only when there is presubmit check notification. TEST=unit test BUG=none Review URL: http://codereview.chromium.org/147162 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@19429 0039d316-1c4b-4281-b951-d872f2087c98 --- presubmit_support.py | 16 ++++++++++++++++ tests/presubmit_unittest.py | 21 +++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/presubmit_support.py b/presubmit_support.py index 8586a4871..8ab05c133 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -22,6 +22,7 @@ import marshal # Exposed through the API. import optparse import os # Somewhat exposed through the API. import pickle # Exposed through the API. +import random import re # Exposed through the API. import subprocess # Exposed through the API. import sys # Parts exposed through API. @@ -40,6 +41,10 @@ import gclient import presubmit_canned_checks +# Ask for feedback only once in program lifetime. +_ASKED_FOR_FEEDBACK = False + + class NotImplementedException(Exception): """We're leaving placeholders in a bunch of places to remind us of the design of the API, but we have not implemented all of it yet. Implement as @@ -780,6 +785,10 @@ def DoPresubmitChecks(change, default_presubmit: A default presubmit script to execute in any case. may_prompt: Enable (y/n) questions on warning or error. + Warning: + If may_prompt is true, output_stream SHOULD be sys.stdout and input_stream + SHOULD be sys.stdin. + Return: True if execution can continue, False if not. """ @@ -830,6 +839,13 @@ def DoPresubmitChecks(change, response = input_stream.readline() if response.strip().lower() != 'y': error_count += 1 + + global _ASKED_FOR_FEEDBACK + # Ask for feedback one time out of 5. + if (len(results) and random.randint(0, 4) == 0 and not _ASKED_FOR_FEEDBACK): + output_stream.write("Was the presubmit check useful? Please send feedback " + "& hate mail to maruel@chromium.org!\n") + _ASKED_FOR_FEEDBACK = True return (error_count == 0) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index bf9237cac..f6399c7e0 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -48,7 +48,9 @@ def CheckChangeOnUpload(input_api, output_api): presubmit.os.path.dirname = os_path_dirname presubmit.os.path.normpath = os_path_normpath presubmit.os.path.splitext = os_path_splitext + self.mox.StubOutWithMock(presubmit, 'random') self.mox.StubOutWithMock(presubmit, 'sys') + presubmit._ASKED_FOR_FEEDBACK = False # Special mocks. def MockAbsPath(f): return f @@ -71,8 +73,8 @@ class PresubmitUnittest(PresubmitTestsBase): 'SvnAffectedFile', 'SvnChange', 'cPickle', 'cStringIO', 'exceptions', 'fnmatch', 'gcl', 'gclient', 'glob', 'logging', 'marshal', 'normpath', - 'optparse', - 'os', 'pickle', 'presubmit_canned_checks', 're', 'subprocess', 'sys', + 'optparse', 'os', 'pickle', + 'presubmit_canned_checks', 'random', 're', 'subprocess', 'sys', 'tempfile', 'traceback', 'types', 'unittest', 'urllib2', 'warnings', ] # If this test fails, you should add the relevant test. @@ -295,6 +297,7 @@ class PresubmitUnittest(PresubmitTestsBase): 'rU').AndReturn(self.presubmit_text) presubmit.gcl.ReadFile(haspresubmit_path, 'rU').AndReturn(self.presubmit_text) + presubmit.random.randint(0, 4).AndReturn(1) self.mox.ReplayAll() output = StringIO.StringIO() @@ -322,6 +325,8 @@ class PresubmitUnittest(PresubmitTestsBase): ).AndReturn(self.presubmit_text) presubmit.gcl.ReadFile(haspresubmit_path, 'rU' ).AndReturn(self.presubmit_text) + presubmit.random.randint(0, 4).AndReturn(1) + presubmit.random.randint(0, 4).AndReturn(1) self.mox.ReplayAll() output = StringIO.StringIO() @@ -355,6 +360,7 @@ class PresubmitUnittest(PresubmitTestsBase): presubmit.gcl.ReadFile(presubmit_path, 'rU').AndReturn(self.presubmit_text) presubmit.gcl.ReadFile(haspresubmit_path, 'rU').AndReturn( self.presubmit_text) + presubmit.random.randint(0, 4).AndReturn(1) self.mox.ReplayAll() output = StringIO.StringIO() @@ -367,7 +373,7 @@ class PresubmitUnittest(PresubmitTestsBase): self.assertEqual(output.getvalue().count('XX!!XX'), 2) self.assertEqual(output.getvalue().count('(y/N)'), 0) - def testDoDefaultPresubmitChecks(self): + def testDoDefaultPresubmitChecksAndFeedback(self): join = presubmit.os.path.join description_lines = ('Hello there', 'this is a change', @@ -386,6 +392,7 @@ def CheckChangeOnCommit(input_api, output_api): presubmit.os.path.isfile(join(self.fake_root_dir, 'haspresubmit', 'PRESUBMIT.py')).AndReturn(False) + presubmit.random.randint(0, 4).AndReturn(0) self.mox.ReplayAll() output = StringIO.StringIO() @@ -395,7 +402,12 @@ def CheckChangeOnCommit(input_api, output_api): self.fake_root_dir, files, 0, 0) self.failIf(presubmit.DoPresubmitChecks(change, False, True, output, input, DEFAULT_SCRIPT, False)) - self.assertEquals(output.getvalue().count('!!'), 1) + text = ('Warning, no presubmit.py found.\n' + 'Running default presubmit script.\n' + '** Presubmit ERRORS **\n!!\n\n' + 'Was the presubmit check useful? Please send feedback & hate mail ' + 'to maruel@chromium.org!\n') + self.assertEquals(output.getvalue(), text) def testDirectoryHandling(self): files = [ @@ -449,6 +461,7 @@ def CheckChangeOnUpload(input_api, output_api): def CheckChangeOnCommit(input_api, output_api): raise Exception("Test error") """ + presubmit.random.randint(0, 4).AndReturn(1) self.mox.ReplayAll() output = StringIO.StringIO()