Update subprocess2.check_output() to behave like subprocess.check_output().

stderr is not redirected by default. stdout is not allowed.
Both were oversight.
Do not override stdin=None in case the user would response to stderr output for
example.

Increase test coverage.

R=dpranke@chromium.org
BUG=
TEST=


Review URL: http://codereview.chromium.org/7860041

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

PRESUBMIT.py

@@ -85,7 +85,8 @@ def RunGitClTests(input_api, output_api):
               [input_api.os_path.join(test_path, test)], cwd=test_path)
         else:
           input_api.subprocess.check_output(
-              [input_api.os_path.join(test_path, test)], cwd=test_path)
+              [input_api.os_path.join(test_path, test)], cwd=test_path,
+              stderr=input_api.subprocess.STDOUT)
       except (OSError, input_api.subprocess.CalledProcessError), e:
         results.append(output_api.PresubmitError('%s failed\n%s' % (test, e)))
   except local_rietveld.Failure, e:

checkout.py

@@ -140,6 +140,7 @@ class RawCheckout(CheckoutBase):
               stdout = subprocess2.check_output(
                   ['patch', '-p%s' % p.patchlevel],
                   stdin=p.get(),
+                  stderr=subprocess2.STDOUT,
                   cwd=self.project_path)
             elif p.is_new and not os.path.exists(filepath):
               # There is only a header. Just create the file.
@@ -218,7 +219,9 @@ class SvnMixIn(object):
     """
     kwargs.setdefault('cwd', self.project_path)
     return subprocess2.check_output(
-        self._add_svn_flags(args, True, credentials), **kwargs)
+        self._add_svn_flags(args, True, credentials),
+        stderr=subprocess2.STDOUT,
+        **kwargs)
 
   @staticmethod
   def _parse_svn_info(output, key):
@@ -496,7 +499,8 @@ class GitCheckoutBase(CheckoutBase):
 
   def _check_output_git(self, args, **kwargs):
     kwargs.setdefault('cwd', self.project_path)
-    return subprocess2.check_output(['git'] + args, **kwargs)
+    return subprocess2.check_output(
+        ['git'] + args, stderr=subprocess2.STDOUT, **kwargs)
 
   def _branches(self):
     """Returns the list of branches and the active one."""

gclient_scm.py

@@ -665,7 +665,9 @@ class GitWrapper(SCMWrapper):
 
   def _Capture(self, args):
     return subprocess2.check_output(
-        ['git'] + args, cwd=self.checkout_path).strip()
+        ['git'] + args,
+        stderr=subprocess2.PIPE,
+        cwd=self.checkout_path).strip()
 
   def _Run(self, args, options, **kwargs):
     kwargs.setdefault('cwd', self.checkout_path)

presubmit_canned_checks.py

@@ -515,7 +515,9 @@ def RunUnitTests(input_api, output_api, unit_tests):
         input_api.subprocess.check_call(cmd, cwd=input_api.PresubmitLocalPath())
       else:
         input_api.subprocess.check_output(
-            cmd, cwd=input_api.PresubmitLocalPath())
+            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)))
   return results
@@ -558,7 +560,8 @@ def RunPythonUnitTests(input_api, output_api, unit_tests):
       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, cwd=cwd, env=env)
+      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)))
   return results

scm.py

@@ -79,6 +79,7 @@ def determine_scm(root):
       subprocess2.check_output(
           ['git', 'rev-parse', '--show-cdup'],
           stdout=subprocess2.VOID,
+          stderr=subprocess2.VOID,
           cwd=root)
       return 'git'
     except (OSError, subprocess2.CalledProcessError):

subprocess2.py

@@ -249,6 +249,8 @@ def call(args, **kwargs):
   """Emulates subprocess.call().
 
   Automatically convert stdout=PIPE or stderr=PIPE to VOID.
+  In no case they can be returned since no code path raises
+  subprocess2.CalledProcessError.
   """
   if kwargs.get('stdout') == PIPE:
     kwargs['stdout'] = VOID
@@ -281,16 +283,12 @@ def capture(args, **kwargs):
   Returns stdout.
 
   - Discards returncode.
-  - Discards stderr. By default sets stderr=STDOUT.
-  - Blocks stdin by default since no output will be visible.
+  - Blocks stdin by default if not specified since no output will be visible.
   """
-  if kwargs.get('stdin') is None:
-    kwargs['stdin'] = VOID
-  if kwargs.get('stdout') is None:
-    kwargs['stdout'] = PIPE
-  if kwargs.get('stderr') is None:
-    kwargs['stderr'] = STDOUT
-  return communicate(args, **kwargs)[0][0]
+  kwargs.setdefault('stdin', VOID)
+
+  # Like check_output, deny the caller from using stdout arg.
+  return communicate(args, stdout=PIPE, **kwargs)[0][0]
 
 
 def check_output(args, **kwargs):
@@ -298,15 +296,10 @@ def check_output(args, **kwargs):
 
   Captures stdout of a process call and returns stdout only.
 
-  - Discards stderr. By default sets stderr=STDOUT.
   - Throws if return code is not 0.
   - Works even prior to python 2.7.
-  - Blocks stdin by default since no output will be visible.
+  - Blocks stdin by default if not specified since no output will be visible.
+  - As per doc, "The stdout argument is not allowed as it is used internally."
   """
-  if kwargs.get('stdin') is None:
-    kwargs['stdin'] = VOID
-  if kwargs.get('stdout') is None:
-    kwargs['stdout'] = PIPE
-  if kwargs.get('stderr') is None:
-    kwargs['stderr'] = STDOUT
-  return check_call_out(args, **kwargs)[0]
+  kwargs.setdefault('stdin', VOID)
+  return check_call_out(args, stdout=PIPE, **kwargs)[0]

tests/subprocess2_test.py

@@ -34,6 +34,8 @@ class Subprocess2Test(unittest.TestCase):
     for module, names in self.TO_SAVE.iteritems():
       self.saved[module] = dict(
           (name, getattr(module, name)) for name in names)
+    # TODO(maruel): Do a reopen() on sys.__stdout__ and sys.__stderr__ so they
+    # can be trapped in the child process for better coverage.
 
   def tearDown(self):
     for module, saved in self.saved.iteritems():
@@ -89,6 +91,18 @@ class Subprocess2Test(unittest.TestCase):
     }
     self.assertEquals(expected, results)
 
+  def test_capture_defaults(self):
+    results = self._fake_communicate()
+    self.assertEquals(
+        'stdout', subprocess2.capture(['foo'], a=True))
+    expected = {
+        'args': ['foo'],
+        'a':True,
+        'stdin': subprocess2.VOID,
+        'stdout': subprocess2.PIPE,
+    }
+    self.assertEquals(expected, results)
+
   def test_communicate_defaults(self):
     results = self._fake_Popen()
     self.assertEquals(
@@ -129,7 +143,6 @@ class Subprocess2Test(unittest.TestCase):
         'a':True,
         'stdin': subprocess2.VOID,
         'stdout': subprocess2.PIPE,
-        'stderr': subprocess2.STDOUT,
     }
     self.assertEquals(expected, results)
 
@@ -142,22 +155,69 @@ class Subprocess2Test(unittest.TestCase):
     self.assertEquals(subprocess2.TIMED_OUT, returncode)
     self.assertEquals(['', None], out)
 
-  def test_void(self):
-    out = subprocess2.check_output(
+  def test_check_output_no_stdout(self):
+    try:
+      subprocess2.check_output(self.exe, stdout=subprocess2.PIPE)
+      self.fail()
+    except TypeError:
+      pass
+
+  def test_stdout_void(self):
+    (out, err), code = subprocess2.communicate(
          self.exe + ['--stdout', '--stderr'],
-         stdout=subprocess2.VOID)
+         stdout=subprocess2.VOID,
+         stderr=subprocess2.PIPE)
     self.assertEquals(None, out)
-    out = subprocess2.check_output(
+    self.assertEquals('a\nbb\nccc\n', err)
+    self.assertEquals(0, code)
+
+  def test_stderr_void(self):
+    (out, err), code = subprocess2.communicate(
          self.exe + ['--stdout', '--stderr'],
-          universal_newlines=True,
+         universal_newlines=True,
+         stdout=subprocess2.PIPE,
          stderr=subprocess2.VOID)
     self.assertEquals('A\nBB\nCCC\n', out)
+    self.assertEquals(None, err)
+    self.assertEquals(0, code)
+
+  def test_check_output_throw_stdout(self):
+    try:
+      subprocess2.check_output(
+          self.exe + ['--fail', '--stdout'], universal_newlines=True)
+      self.fail()
+    except subprocess2.CalledProcessError, e:
+      self.assertEquals('A\nBB\nCCC\n', e.stdout)
+      self.assertEquals(None, e.stderr)
+      self.assertEquals(64, e.returncode)
 
-  def test_check_output_throw(self):
+  def test_check_output_throw_no_stderr(self):
     try:
       subprocess2.check_output(
           self.exe + ['--fail', '--stderr'], universal_newlines=True)
       self.fail()
+    except subprocess2.CalledProcessError, e:
+      self.assertEquals('', e.stdout)
+      self.assertEquals(None, e.stderr)
+      self.assertEquals(64, e.returncode)
+
+  def test_check_output_throw_stderr(self):
+    try:
+      subprocess2.check_output(
+          self.exe + ['--fail', '--stderr'], stderr=subprocess2.PIPE,
+          universal_newlines=True)
+      self.fail()
+    except subprocess2.CalledProcessError, e:
+      self.assertEquals('', e.stdout)
+      self.assertEquals('a\nbb\nccc\n', e.stderr)
+      self.assertEquals(64, e.returncode)
+
+  def test_check_output_throw_stderr_stdout(self):
+    try:
+      subprocess2.check_output(
+          self.exe + ['--fail', '--stderr'], stderr=subprocess2.STDOUT,
+          universal_newlines=True)
+      self.fail()
     except subprocess2.CalledProcessError, e:
       self.assertEquals('a\nbb\nccc\n', e.stdout)
       self.assertEquals(None, e.stderr)