Add support for parallel presubmit unit testing.

Enable parallel tests on depot_tools.
On Z620 presubmit times: 3m -> 35s.

Review URL: https://codereview.chromium.org/14247012

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@195377 0039d316-1c4b-4281-b951-d872f2087c98

PRESUBMIT.py

@@ -23,21 +23,21 @@ def CommonChecks(input_api, output_api, tests_to_black_list):
     'R0401',  # Cyclic import
     'W0613',  # Unused argument
   ]
-  results.extend(input_api.canned_checks.RunPylint(
-      input_api,
-      output_api,
-      white_list=[r'.*\.py$'],
-      black_list=black_list,
-      disabled_warnings=disabled_warnings))
-
-  # TODO(maruel): Make sure at least one file is modified first.
-  # TODO(maruel): If only tests are modified, only run them.
-  results.extend(input_api.canned_checks.RunUnitTestsInDirectory(
-      input_api,
-      output_api,
-      'tests',
-      whitelist=[r'.*test\.py$'],
-      blacklist=tests_to_black_list))
+  results.extend(input_api.RunTests(
+      input_api.canned_checks.GetPylint(
+          input_api,
+          output_api,
+          white_list=[r'.*\.py$'],
+          black_list=black_list,
+          disabled_warnings=disabled_warnings) +
+      # TODO(maruel): Make sure at least one file is modified first.
+      # TODO(maruel): If only tests are modified, only run them.
+      input_api.canned_checks.GetUnitTestsInDirectory(
+          input_api,
+          output_api,
+          'tests',
+          whitelist=[r'.*test\.py$'],
+          blacklist=tests_to_black_list)))
   return results
 
 

presubmit_canned_checks.py

@@ -483,8 +483,7 @@ def CheckTreeIsOpen(input_api, output_api,
                                       long_text=str(e))]
   return []
 
-
-def RunUnitTestsInDirectory(
+def GetUnitTestsInDirectory(
     input_api, output_api, directory, whitelist=None, blacklist=None):
   """Lists all files in a directory and runs them. Doesn't recurse.
 
@@ -517,10 +516,10 @@ def RunUnitTestsInDirectory(
           'Out of %d files, found none that matched w=%r, b=%r in directory %s'
           % (found, whitelist, blacklist, directory))
     ]
-  return RunUnitTests(input_api, output_api, unit_tests)
+  return GetUnitTests(input_api, output_api, unit_tests)
 
 
-def RunUnitTests(input_api, output_api, unit_tests):
+def GetUnitTests(input_api, output_api, unit_tests):
   """Runs all unit tests in a directory.
 
   On Windows, sys.executable is used for unit tests ending with ".py".
@@ -541,20 +540,14 @@ def RunUnitTests(input_api, output_api, unit_tests):
     if input_api.verbose:
       print('Running %s' % unit_test)
       cmd.append('--verbose')
-    try:
-      if input_api.verbose:
-        input_api.subprocess.check_call(cmd, cwd=input_api.PresubmitLocalPath())
-      else:
-        input_api.subprocess.check_output(
-            cmd,
-            stderr=input_api.subprocess.STDOUT,
-            cwd=input_api.PresubmitLocalPath())
-    except (OSError, input_api.subprocess.CalledProcessError), e:
-      results.append(message_type('%s failed!\n%s' % (unit_test, e)))
+    results.append(input_api.Command(
+        name=unit_test,
+        cmd=cmd,
+        kwargs={'cwd': input_api.PresubmitLocalPath()},
+        message=message_type))
   return results
 
-
-def RunPythonUnitTests(input_api, output_api, unit_tests):
+def GetPythonUnitTests(input_api, output_api, unit_tests):
   """Run the unit tests out of process, capture the output and use the result
   code to determine success.
 
@@ -590,14 +583,42 @@ def RunPythonUnitTests(input_api, output_api, unit_tests):
         backpath.append(env.get('PYTHONPATH'))
       env['PYTHONPATH'] = input_api.os_path.pathsep.join((backpath))
     cmd = [input_api.python_executable, '-m', '%s' % unit_test]
-    try:
-      input_api.subprocess.check_output(
-          cmd, stderr=input_api.subprocess.STDOUT, cwd=cwd, env=env)
-    except (OSError, input_api.subprocess.CalledProcessError), e:
-      results.append(message_type('%s failed!\n%s' % (unit_test_name, e)))
+    results.append(input_api.Command(
+        name=unit_test_name,
+        cmd=cmd,
+        kwargs={'env': env, 'cwd': cwd},
+        message=message_type))
   return results
 
 
+def RunUnitTestsInDirectory(input_api, *args, **kwargs):
+  """Run tests in a directory serially.
+
+  For better performance, use GetUnitTestsInDirectory and then
+  pass to input_api.RunTests.
+  """
+  return input_api.RunTests(
+      GetUnitTestsInDirectory(input_api, *args, **kwargs), False)
+
+
+def RunUnitTests(input_api, *args, **kwargs):
+  """Run tests serially.
+
+  For better performance, use GetUnitTests and then pass to
+  input_api.RunTests.
+  """
+  return input_api.RunTests(GetUnitTests(input_api, *args, **kwargs), False)
+
+
+def RunPythonUnitTests(input_api, *args, **kwargs):
+  """Run python tests in a directory serially.
+
+  DEPRECATED
+  """
+  return input_api.RunTests(
+      GetPythonUnitTests(input_api, *args, **kwargs), False)
+
+
 def _FetchAllFiles(input_api, white_list, black_list):
   """Hack to fetch all files."""
   # We cannot use AffectedFiles here because we want to test every python
@@ -626,7 +647,7 @@ def _FetchAllFiles(input_api, white_list, black_list):
   return files
 
 
-def RunPylint(input_api, output_api, white_list=None, black_list=None,
+def GetPylint(input_api, output_api, white_list=None, black_list=None,
               disabled_warnings=None, extra_paths_list=None):
   """Run pylint on python files.
 
@@ -669,6 +690,7 @@ def RunPylint(input_api, output_api, white_list=None, black_list=None,
   files = _FetchAllFiles(input_api, white_list, black_list)
   if not files:
     return []
+  files.sort()
 
   input_api.logging.info('Running pylint on %d files', len(files))
   input_api.logging.debug('Running pylint on: %s', files)
@@ -679,31 +701,23 @@ def RunPylint(input_api, output_api, white_list=None, black_list=None,
   env['PYTHONPATH'] = input_api.os_path.pathsep.join(
       extra_paths_list + sys.path).encode('utf8')
 
-  def run_lint(files):
-    # We can't import pylint directly due to licensing issues, so we run
-    # it in another process. Windows needs help running python files so we
-    # explicitly specify the interpreter to use. It also has limitations on
-    # the size of the command-line, so we pass arguments via a pipe.
-    command = [input_api.python_executable,
-               input_api.os_path.join(_HERE, 'third_party', 'pylint.py'),
-               '--args-on-stdin']
-    try:
-      child = input_api.subprocess.Popen(command, env=env,
-          stdin=input_api.subprocess.PIPE)
-
-      # Dump the arguments to the child process via a pipe.
-      for filename in files:
-        child.stdin.write(filename + '\n')
-      for arg in extra_args:
-        child.stdin.write(arg + '\n')
-      child.stdin.close()
-
-      child.communicate()
-      return child.returncode
-    except OSError:
-      return 'Pylint failed!'
-
-  result = None
+  def GetPylintCmd(files):
+    # Windows needs help running python files so we explicitly specify
+    # the interpreter to use. It also has limitations on the size of
+    # the command-line, so we pass arguments via a pipe.
+    if len(files) == 1:
+      description = files[0]
+    else:
+      description = '%s files' % len(files)
+
+    return input_api.Command(
+        name='Pylint (%s)' % description,
+        cmd=[input_api.python_executable,
+             input_api.os_path.join(_HERE, 'third_party', 'pylint.py'),
+             '--args-on-stdin'],
+        kwargs={'env': env, 'stdin': '\n'.join(files + extra_args)},
+        message=error_type)
+
   # Always run pylint and pass it all the py files at once.
   # Passing py files one at time is slower and can produce
   # different results.  input_api.verbose used to be used
@@ -713,17 +727,18 @@ def RunPylint(input_api, output_api, white_list=None, black_list=None,
   # a quick local edit to diagnose pylint issues more
   # easily.
   if True:
-    print('Running pylint on %d files.' % len(files))
-    result = run_lint(sorted(files))
+    return [GetPylintCmd(files)]
   else:
-    for filename in sorted(files):
-      print('Running pylint on %s' % filename)
-      result = run_lint([filename]) or result
-  if isinstance(result, basestring):
-    return [error_type(result)]
-  elif result:
-    return [error_type('Fix pylint errors first.')]
-  return []
+    return map(GetPylintCmd, files)
+
+
+def RunPylint(input_api, *args, **kwargs):
+  """Legacy presubmit function.
+
+  For better performance, get all tests and then pass to
+  input_api.RunTests.
+  """
+  return input_api.RunTests(GetPylint(input_api, *args, **kwargs), False)
 
 
 # TODO(dpranke): Get the host_url from the input_api instead

presubmit_support.py

@@ -15,6 +15,7 @@ __version__ = '1.6.2'
 import cpplint
 import cPickle  # Exposed through the API.
 import cStringIO  # Exposed through the API.
+import collections
 import contextlib
 import fnmatch
 import glob
@@ -22,6 +23,7 @@ import inspect
 import json  # Exposed through the API.
 import logging
 import marshal  # Exposed through the API.
+import multiprocessing
 import optparse
 import os  # Somewhat exposed through the API.
 import pickle  # Exposed through the API.
@@ -54,6 +56,9 @@ class PresubmitFailure(Exception):
   pass
 
 
+CommandData = collections.namedtuple('CommandData',
+                                     ['name', 'cmd', 'kwargs', 'message'])
+
 def normpath(path):
   '''Version of os.path.normpath that also changes backward slashes to
   forward slashes when not running on Windows.
@@ -104,68 +109,99 @@ class PresubmitOutput(object):
     return ''.join(self.written_output)
 
 
-class OutputApi(object):
-  """An instance of OutputApi gets passed to presubmit scripts so that they
-  can output various types of results.
-  """
-  def __init__(self, is_committing):
-    self.is_committing = is_committing
-
-  class PresubmitResult(object):
-    """Base class for result objects."""
-    fatal = False
-    should_prompt = False
-
-    def __init__(self, message, items=None, long_text=''):
-      """
-      message: A short one-line message to indicate errors.
-      items: A list of short strings to indicate where errors occurred.
-      long_text: multi-line text output, e.g. from another tool
-      """
-      self._message = message
-      self._items = []
-      if items:
-        self._items = items
-      self._long_text = long_text.rstrip()
+# Top level object so multiprocessing can pickle
+# Public access through OutputApi object.
+class _PresubmitResult(object):
+  """Base class for result objects."""
+  fatal = False
+  should_prompt = False
 
-    def handle(self, output):
-      output.write(self._message)
+  def __init__(self, message, items=None, long_text=''):
+    """
+    message: A short one-line message to indicate errors.
+    items: A list of short strings to indicate where errors occurred.
+    long_text: multi-line text output, e.g. from another tool
+    """
+    self._message = message
+    self._items = items or []
+    if items:
+      self._items = items
+    self._long_text = long_text.rstrip()
+
+  def handle(self, output):
+    output.write(self._message)
+    output.write('\n')
+    for index, item in enumerate(self._items):
+      output.write('  ')
+      # Write separately in case it's unicode.
+      output.write(str(item))
+      if index < len(self._items) - 1:
+        output.write(' \\')
       output.write('\n')
-      for index, item in enumerate(self._items):
-        output.write('  ')
-        # Write separately in case it's unicode.
-        output.write(str(item))
-        if index < len(self._items) - 1:
-          output.write(' \\')
-        output.write('\n')
-      if self._long_text:
-        output.write('\n***************\n')
-        # Write separately in case it's unicode.
-        output.write(self._long_text)
-        output.write('\n***************\n')
-      if self.fatal:
-        output.fail()
+    if self._long_text:
+      output.write('\n***************\n')
+      # Write separately in case it's unicode.
+      output.write(self._long_text)
+      output.write('\n***************\n')
+    if self.fatal:
+      output.fail()
+
+
+# Top level object so multiprocessing can pickle
+# Public access through OutputApi object.
+class _PresubmitAddReviewers(_PresubmitResult):
+  """Add some suggested reviewers to the change."""
+  def __init__(self, reviewers):
+    super(_PresubmitAddReviewers, self).__init__('')
+    self.reviewers = reviewers
 
-  class PresubmitAddReviewers(PresubmitResult):
-    """Add some suggested reviewers to the change."""
-    def __init__(self, reviewers):
-      super(OutputApi.PresubmitAddReviewers, self).__init__('')
-      self.reviewers = reviewers
+  def handle(self, output):
+    output.reviewers.extend(self.reviewers)
 
-    def handle(self, output):
-      output.reviewers.extend(self.reviewers)
 
-  class PresubmitError(PresubmitResult):
-    """A hard presubmit error."""
-    fatal = True
+# Top level object so multiprocessing can pickle
+# Public access through OutputApi object.
+class _PresubmitError(_PresubmitResult):
+  """A hard presubmit error."""
+  fatal = True
 
-  class PresubmitPromptWarning(PresubmitResult):
-    """An warning that prompts the user if they want to continue."""
-    should_prompt = True
 
-  class PresubmitNotifyResult(PresubmitResult):
-    """Just print something to the screen -- but it's not even a warning."""
-    pass
+# Top level object so multiprocessing can pickle
+# Public access through OutputApi object.
+class _PresubmitPromptWarning(_PresubmitResult):
+  """An warning that prompts the user if they want to continue."""
+  should_prompt = True
+
+
+# Top level object so multiprocessing can pickle
+# Public access through OutputApi object.
+class _PresubmitNotifyResult(_PresubmitResult):
+  """Just print something to the screen -- but it's not even a warning."""
+  pass
+
+
+# Top level object so multiprocessing can pickle
+# Public access through OutputApi object.
+class _MailTextResult(_PresubmitResult):
+  """A warning that should be included in the review request email."""
+  def __init__(self, *args, **kwargs):
+    super(_MailTextResult, self).__init__()
+    raise NotImplementedError()
+
+
+class OutputApi(object):
+  """An instance of OutputApi gets passed to presubmit scripts so that they
+  can output various types of results.
+  """
+  PresubmitResult = _PresubmitResult
+  PresubmitAddReviewers = _PresubmitAddReviewers
+  PresubmitError = _PresubmitError
+  PresubmitPromptWarning = _PresubmitPromptWarning
+  PresubmitNotifyResult = _PresubmitNotifyResult
+  MailTextResult = _MailTextResult
+
+  def __init__(self, is_committing):
+    self.is_committing = is_committing
 
   def PresubmitPromptOrNotify(self, *args, **kwargs):
     """Warn the user when uploading, but only notify if committing."""
@@ -173,12 +209,6 @@ class OutputApi(object):
       return self.PresubmitNotifyResult(*args, **kwargs)
     return self.PresubmitPromptWarning(*args, **kwargs)
 
-  class MailTextResult(PresubmitResult):
-    """A warning that should be included in the review request email."""
-    def __init__(self, *args, **kwargs):
-      super(OutputApi.MailTextResult, self).__init__()
-      raise NotImplementedError()
-
 
 class InputApi(object):
   """An instance of this object is passed to presubmit scripts so they can
@@ -284,6 +314,7 @@ class InputApi(object):
     self.owners_db = owners.Database(change.RepositoryRoot(),
         fopen=file, os_path=self.os_path, glob=self.glob)
     self.verbose = verbose
+    self.Command = CommandData
 
     # Replace <hash_map> and <hash_set> as headers that need to be included
     # with "base/hash_tables.h" instead.
@@ -437,6 +468,26 @@ class InputApi(object):
     """Returns if a change is TBR'ed."""
     return 'TBR' in self.change.tags
 
+  @staticmethod
+  def RunTests(tests_mix, parallel=True):
+    tests = []
+    msgs = []
+    for t in tests_mix:
+      if isinstance(t, OutputApi.PresubmitResult):
+        msgs.append(t)
+      else:
+        assert issubclass(t.message, _PresubmitResult)
+        tests.append(t)
+    if parallel:
+      pool = multiprocessing.Pool()
+      # async recipe works around multiprocessing bug handling Ctrl-C
+      msgs.extend(pool.map_async(CallCommand, tests).get(99999))
+      pool.close()
+      pool.join()
+    else:
+      msgs.extend(map(CallCommand, tests))
+    return [m for m in msgs if m]
+
 
 class AffectedFile(object):
   """Representation of a file in a change."""
@@ -1238,6 +1289,18 @@ def canned_check_filter(method_names):
     for name, method in filtered.iteritems():
       setattr(presubmit_canned_checks, name, method)
 
+def CallCommand(cmd_data):
+  # multiprocessing needs a top level function with a single argument.
+  cmd_data.kwargs['stdout'] = subprocess.PIPE
+  cmd_data.kwargs['stderr'] = subprocess.STDOUT
+  try:
+    (out, _), code = subprocess.communicate(cmd_data.cmd, **cmd_data.kwargs)
+    if code != 0:
+      return cmd_data.message('%s failed\n%s' % (cmd_data.name, out))
+  except OSError as e:
+    return cmd_data.message(
+        '%s exec failure\n   %s\n%s' % (cmd_data.name, e, out))
+
 
 def Main(argv):
   parser = optparse.OptionParser(usage="%prog [options] <files...>",

tests/presubmit_unittest.py

@@ -10,9 +10,9 @@
 import logging
 import os
 import StringIO
-import subprocess
 import sys
 import time
+import unittest
 
 _ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 sys.path.insert(0, _ROOT)
@@ -20,6 +20,7 @@ sys.path.insert(0, _ROOT)
 from testing_support.super_mox import mox, SuperMoxTestBase
 
 import owners
+import subprocess2 as subprocess
 import presubmit_support as presubmit
 import rietveld
 
@@ -157,7 +158,7 @@ class PresubmitUnittest(PresubmitTestsBase):
     self.mox.ReplayAll()
     members = [
       'AffectedFile', 'Change', 'DoGetTrySlaves', 'DoPresubmitChecks',
-      'GetTrySlavesExecuter', 'GitAffectedFile',
+      'GetTrySlavesExecuter', 'GitAffectedFile', 'CallCommand', 'CommandData',
       'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'Main',
       'NonexistantCannedCheckFilter', 'OutputApi', 'ParseFiles',
       'PresubmitFailure', 'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs',
@@ -167,7 +168,7 @@ class PresubmitUnittest(PresubmitTestsBase):
       'marshal', 'normpath', 'optparse', 'os', 'owners', 'pickle',
       'presubmit_canned_checks', 'random', 're', 'rietveld', 'scm',
       'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest',
-      'urllib2', 'warn',
+      'urllib2', 'warn', 'collections', 'multiprocessing',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(presubmit, members)
@@ -881,7 +882,7 @@ class InputApiUnittest(PresubmitTestsBase):
       'AffectedTextFiles',
       'DEFAULT_BLACK_LIST', 'DEFAULT_WHITE_LIST',
       'DepotToLocalPath', 'FilterSourceFile', 'LocalPaths',
-      'LocalToDepotPath',
+      'LocalToDepotPath', 'Command', 'RunTests',
       'PresubmitLocalPath', 'ReadFile', 'RightHandSideLines', 'ServerPaths',
       'basename', 'cPickle', 'cpplint', 'cStringIO', 'canned_checks', 'change',
       'environ', 'glob', 'host_url', 'is_committing', 'json', 'logging',
@@ -1485,6 +1486,13 @@ class ChangeUnittest(PresubmitTestsBase):
     self.assertEquals('Y', change.AffectedFiles(include_dirs=True)[0].Action())
 
 
+def CommHelper(input_api, cmd, ret=None, **kwargs):
+  ret = ret or (('', None), 0)
+  input_api.subprocess.communicate(
+      cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kwargs
+      ).AndReturn(ret)
+
+
 class CannedChecksUnittest(PresubmitTestsBase):
   """Tests presubmit_canned_checks.py."""
 
@@ -1502,7 +1510,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.traceback = presubmit.traceback
     input_api.urllib2 = self.mox.CreateMock(presubmit.urllib2)
     input_api.unittest = unittest
-    input_api.subprocess = self.mox.CreateMock(presubmit.subprocess)
+    input_api.subprocess = self.mox.CreateMock(subprocess)
+    presubmit.subprocess = input_api.subprocess
     class fake_CalledProcessError(Exception):
       def __str__(self):
         return 'foo'
@@ -1517,6 +1526,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.platform = sys.platform
     input_api.time = time
     input_api.canned_checks = presubmit_canned_checks
+    input_api.Command = presubmit.CommandData
+    input_api.RunTests = presubmit.InputApi.RunTests
     return input_api
 
   def testMembersChanged(self):
@@ -1544,6 +1555,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
       'CheckSvnForCommonMimeTypes', 'CheckSvnProperty',
       'RunPythonUnitTests', 'RunPylint',
       'RunUnitTests', 'RunUnitTestsInDirectory',
+      'GetPythonUnitTests', 'GetPylint',
+      'GetUnitTests', 'GetUnitTestsInDirectory',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(presubmit_canned_checks, members)
@@ -2117,10 +2130,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
   def testRunPythonUnitTestsNonExistentUpload(self):
     input_api = self.MockInputApi(None, False)
-    input_api.subprocess.check_output(
-        ['pyyyyython', '-m', '_non_existent_module'], cwd=None, env=None,
-        stderr=input_api.subprocess.STDOUT).AndRaise(
-            input_api.subprocess.CalledProcessError())
+    CommHelper(input_api, ['pyyyyython', '-m', '_non_existent_module'],
+                    ret=(('foo', None), 1), cwd=None, env=None)
     self.mox.ReplayAll()
 
     results = presubmit_canned_checks.RunPythonUnitTests(
@@ -2131,10 +2142,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
   def testRunPythonUnitTestsNonExistentCommitting(self):
     input_api = self.MockInputApi(None, True)
-    input_api.subprocess.check_output(
-        ['pyyyyython', '-m', '_non_existent_module'], cwd=None, env=None,
-        stderr=input_api.subprocess.STDOUT).AndRaise(
-            input_api.subprocess.CalledProcessError())
+    CommHelper(input_api, ['pyyyyython', '-m', '_non_existent_module'],
+                    ret=(('foo', None), 1), cwd=None, env=None)
     self.mox.ReplayAll()
 
     results = presubmit_canned_checks.RunPythonUnitTests(
@@ -2146,10 +2155,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api = self.MockInputApi(None, False)
     input_api.unittest = self.mox.CreateMock(unittest)
     input_api.cStringIO = self.mox.CreateMock(presubmit.cStringIO)
-    input_api.subprocess.check_output(
-        ['pyyyyython', '-m', 'test_module'], cwd=None, env=None,
-        stderr=input_api.subprocess.STDOUT).AndRaise(
-            input_api.subprocess.CalledProcessError())
+    CommHelper(input_api, ['pyyyyython', '-m', 'test_module'],
+                    ret=(('foo', None), 1), cwd=None, env=None)
     self.mox.ReplayAll()
 
     results = presubmit_canned_checks.RunPythonUnitTests(
@@ -2157,29 +2164,26 @@ class CannedChecksUnittest(PresubmitTestsBase):
     self.assertEquals(len(results), 1)
     self.assertEquals(results[0].__class__,
                       presubmit.OutputApi.PresubmitNotifyResult)
-    self.assertEquals('test_module failed!\nfoo', results[0]._message)
+    self.assertEquals('test_module failed\nfoo', results[0]._message)
 
   def testRunPythonUnitTestsFailureCommitting(self):
     input_api = self.MockInputApi(None, True)
-    input_api.subprocess.check_output(
-        ['pyyyyython', '-m', 'test_module'], cwd=None, env=None,
-        stderr=input_api.subprocess.STDOUT).AndRaise(
-            input_api.subprocess.CalledProcessError())
+    CommHelper(input_api, ['pyyyyython', '-m', 'test_module'],
+                    ret=(('foo', None), 1), cwd=None, env=None)
     self.mox.ReplayAll()
 
     results = presubmit_canned_checks.RunPythonUnitTests(
         input_api, presubmit.OutputApi, ['test_module'])
     self.assertEquals(len(results), 1)
     self.assertEquals(results[0].__class__, presubmit.OutputApi.PresubmitError)
-    self.assertEquals('test_module failed!\nfoo', results[0]._message)
+    self.assertEquals('test_module failed\nfoo', results[0]._message)
 
   def testRunPythonUnitTestsSuccess(self):
     input_api = self.MockInputApi(None, False)
     input_api.cStringIO = self.mox.CreateMock(presubmit.cStringIO)
     input_api.unittest = self.mox.CreateMock(unittest)
-    input_api.subprocess.check_output(
-        ['pyyyyython', '-m', 'test_module'], cwd=None, env=None,
-        stderr=input_api.subprocess.STDOUT)
+    CommHelper(input_api, ['pyyyyython', '-m', 'test_module'],
+                    cwd=None, env=None)
     self.mox.ReplayAll()
 
     results = presubmit_canned_checks.RunPythonUnitTests(
@@ -2197,23 +2201,15 @@ class CannedChecksUnittest(PresubmitTestsBase):
     pylint = os.path.join(_ROOT, 'third_party', 'pylint.py')
     pylintrc = os.path.join(_ROOT, 'pylintrc')
 
-    # Create a mock Popen object, and set up its expectations.
-    child = self.mox.CreateMock(subprocess.Popen)
-    child.stdin = self.mox.CreateMock(file)
-    child.stdin.write('file1.py\n')
-    child.stdin.write('--rcfile=%s\n' % pylintrc)
-    child.stdin.close()
-    child.communicate()
-    child.returncode = 0
-
-    input_api.subprocess.Popen(['pyyyyython', pylint, '--args-on-stdin'],
-        env=mox.IgnoreArg(), stdin=subprocess.PIPE).AndReturn(child)
+    CommHelper(input_api,
+        ['pyyyyython', pylint, '--args-on-stdin'],
+        env=mox.IgnoreArg(), stdin='file1.py\n--rcfile=%s' % pylintrc)
     self.mox.ReplayAll()
 
     results = presubmit_canned_checks.RunPylint(
         input_api, presubmit.OutputApi)
     self.assertEquals([], results)
-    self.checkstdout('Running pylint on 1 files.\n')
+    self.checkstdout('')
 
   def testCheckBuildbotPendingBuildsBad(self):
     input_api = self.MockInputApi(None, True)
@@ -2455,14 +2451,11 @@ class CannedChecksUnittest(PresubmitTestsBase):
     unit_tests = ['allo', 'bar.py']
     input_api.PresubmitLocalPath().AndReturn(self.fake_root_dir)
     input_api.PresubmitLocalPath().AndReturn(self.fake_root_dir)
-    input_api.subprocess.check_call(
-        ['allo', '--verbose'], cwd=self.fake_root_dir)
+    CommHelper(input_api, ['allo', '--verbose'], cwd=self.fake_root_dir)
     cmd = ['bar.py', '--verbose']
     if input_api.platform == 'win32':
       cmd.insert(0, input_api.python_executable)
-    input_api.subprocess.check_call(
-        cmd, cwd=self.fake_root_dir).AndRaise(
-            input_api.subprocess.CalledProcessError())
+    CommHelper(input_api, cmd, cwd=self.fake_root_dir, ret=(('', None), 1))
 
     self.mox.ReplayAll()
     results = presubmit_canned_checks.RunUnitTests(
@@ -2485,7 +2478,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
     path = presubmit.os.path.join(self.fake_root_dir, 'random_directory')
     input_api.os_listdir(path).AndReturn(['.', '..', 'a', 'b', 'c'])
     input_api.os_path.isfile = lambda x: not x.endswith('.')
-    input_api.subprocess.check_call(
+    CommHelper(
+        input_api,
         [presubmit.os.path.join('random_directory', 'b'), '--verbose'],
         cwd=self.fake_root_dir)
     input_api.logging.debug('Found 5 files, running 1')