RunCommand was catching and rethrowing an exception, which seemed to generate

extra noise in the final exception output.

This change stops catching and rethrowing the same exception, and generally
makes the RunCommand slightly more readable (if longer).

Some unit tests are added, but they only test RunCommand, and not all options
to RunCommand.

BUG=chromium-os:11717
TEST=Manual, and new lib/cros_build_lib_unittest

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

Change-Id: Ibe9b13ff302621e93196008f86914cb65a232d61

lib/cros_build_lib.py

@@ -70,28 +70,35 @@ def RunCommand(cmd, print_cmd=True, error_ok=False, error_message=None,
          (GetCallerName(), cmd, cwd))
 
   for retry_count in range(num_retries + 1):
-    try:
-      proc = subprocess.Popen(cmd, cwd=cwd, stdin=stdin,
-                              stdout=stdout, stderr=stderr)
-      (output, error) = proc.communicate(input)
-      if exit_code and retry_count == num_retries:
-        return proc.returncode
 
-      if proc.returncode == 0:
-        break
+    # If it's not the first attempt, it's a retry
+    if retry_count > 0 and print_cmd:
+      Info('PROGRAM(%s) -> RunCommand: retrying %r in dir %s' %
+           (GetCallerName(), cmd, cwd))
+    
+    proc = subprocess.Popen(cmd, cwd=cwd, stdin=stdin,
+                            stdout=stdout, stderr=stderr)
+    (output, error) = proc.communicate(input)
 
+    # if the command worked, don't retry any more.
+    if proc.returncode == 0:
+      break
+
+  # If the command (and all retries) failed, handle error result
+  if proc.returncode != 0:
+    if error_ok:
+      if print_cmd:
+        Warning('Command "%r" failed.\n' % (cmd) +
+                (error_message or error or output or ''))
+    else:
       raise RunCommandException('Command "%r" failed.\n' % (cmd) +
                                 (error_message or error or output or ''))
-    except RunCommandException as e:
-      if not error_ok and retry_count == num_retries:
-        raise e
-      else:
-        Warning(str(e))
-        if print_cmd:
-          Info('PROGRAM(%s) -> RunCommand: retrying %r in dir %s' %
-               (GetCallerName(), cmd, cwd))
 
-  return output
+  # return final result
+  if exit_code:
+    return proc.returncode
+  else:
+    return output
 
 
 def RunCommandCaptureOutput(cmd, print_cmd=True, cwd=None, input=None,

lib/cros_build_lib_unittest.py

@@ -0,0 +1,76 @@
+#!/usr/bin/python
+#
+# Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Unit tests for cros_build_lib."""
+
+import unittest
+
+import cros_build_lib
+
+class CrosBuildLibTest(unittest.TestCase):
+  """Test class for cros_build_lib."""
+
+  def testRunCommandSimple(self):
+    """Test that RunCommand can run a simple successful command."""
+    result = cros_build_lib.RunCommand(['ls'],
+                                       # Keep the test quiet options
+                                       print_cmd=False,
+                                       redirect_stdout=True,
+                                       redirect_stderr=True,
+                                       # Test specific options
+                                       exit_code=True)
+    self.assertEqual(result, 0)
+
+  def testRunCommandError(self):
+    """Test that RunCommand can return an error code for a failed command."""
+    result = cros_build_lib.RunCommand(['ls', '/nosuchdir'],
+                                       # Keep the test quiet options
+                                       print_cmd=False,
+                                       redirect_stdout=True,
+                                       redirect_stderr=True,
+                                       # Test specific options
+                                       error_ok=True,
+                                       exit_code=True)
+    self.assertNotEqual(result, 0)
+
+  def testRunCommandErrorRetries(self):
+    """Test that RunCommand can retry a failed command that always fails."""
+
+    # We don't actually check that it's retrying, just exercise the code path. 
+    result = cros_build_lib.RunCommand(['ls', '/nosuchdir'],
+                                       # Keep the test quiet options
+                                       print_cmd=False,
+                                       redirect_stdout=True,
+                                       redirect_stderr=True,
+                                       # Test specific options
+                                       num_retries=2,
+                                       error_ok=True,
+                                       exit_code=True)
+    self.assertNotEqual(result, 0)
+
+  def testRunCommandErrorException(self):
+    """Test that RunCommand can throw an exception when a command fails."""
+
+    function = lambda : cros_build_lib.RunCommand(['ls', '/nosuchdir'],
+                                                  # Keep the test quiet options
+                                                  print_cmd=False,
+                                                  redirect_stdout=True,
+                                                  redirect_stderr=True) 
+    self.assertRaises(cros_build_lib.RunCommandException, function)
+
+  def testRunCommandCaptureOutput(self):
+    """Test that RunCommand can capture stdout if a command succeeds."""
+    
+    result = cros_build_lib.RunCommand(['echo', '-n', 'Hi'],
+                                       # Keep the test quiet options
+                                       print_cmd=False,
+                                       redirect_stdout=True,
+                                       redirect_stderr=True)
+    self.assertEqual(result, 'Hi')
+
+
+if __name__ == '__main__':
+  unittest.main()