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

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

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