Reintroduce RunCommand cleanup and unit tests. Fixed previous issue in which exceptions were thrown if exit_code was set, and added a new unittest to prove that.

This reintroduces the change after commit 3635aaa55e194c496e11c1b74395646fb99c8801, which reverted 01c84235827527cd2f938897a4a8dfcdbf877281.

BUG=chromium-os:11717
TEST=Ran new unit tests, and did full build.

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

Change-Id: I598e459ddaa156af47d47d0482cac16ce44f07a4

lib/cros_build_lib.py

@@ -50,7 +50,8 @@ def RunCommand(cmd, print_cmd=True, error_ok=False, error_message=None,
     Else returns the output of the shell command.
 
   Raises:
-    Exception:  Raises RunCommandException on error with optional error_message.
+    Exception:  Raises RunCommandException on error with optional error_message,
+                but only if exit_code, and error_ok are both False. 
   """
   # Set default for variables.
   stdout = None
@@ -70,27 +71,34 @@ 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:
+    # If it's not the first attempt, it's a retry
-        break
+    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 they asked for an exit_code, give it to them on success or failure
+  if exit_code:
+    return proc.returncode
+
+  # If the command (and all retries) failed, handle error result
+  if proc.returncode != 0:
+    if error_ok:
+      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 final result
   return output
 
 

lib/cros_build_lib_unittest.py

@@ -0,0 +1,92 @@
+#!/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
+                                       exit_code=True)
+    self.assertNotEqual(result, 0)
+    self.assertEquals(type(result), int)
+
+  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)
+    self.assertEquals(type(result), int)
+
+  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 testRunCommandErrorCodeNoException(self):
+    """Test that RunCommand doesn't throw an exception with exit_code."""
+
+    result = cros_build_lib.RunCommand(['ls', '/nosuchdir'],
+                                       # Keep the test quiet options
+                                       print_cmd=False,
+                                       redirect_stdout=True,
+                                       redirect_stderr=True,
+                                       # Test specific options
+                                       exit_code=True)
+    # We are really testing that it doesn't throw an exception if exit_code
+    #  if true.
+    self.assertNotEqual(result, 0)
+    self.assertEquals(type(result), int)
+
+  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()