utils: run: make sure exceptions always use strings for output am: a89a138097

Original change: https://android-review.googlesource.com/c/platform/tools/repohooks/+/1675672

Change-Id: I3a55ae0b856938061254560267fd14723f7aab4b

2 files changed, 60 insertions, 10 deletions

diff --git a/rh/utils.py b/rh/utils.py
index 52a6828..4d17bb8 100644
--- a/rh/utils.py
+++ b/rh/utils.py
@@ -361,6 +361,12 @@ def run(cmd, redirect_stdout=False, redirect_stderr=False, cwd=None, input=None,
     env = env.copy() if env is not None else os.environ.copy()
     env.update(extra_env if extra_env else {})
 
+    def ensure_text(s):
+        """Make sure |s| is a string if it's bytes."""
+        if isinstance(s, bytes):
+            s = s.decode('utf-8', 'replace')
+        return s
+
     result.args = cmd
 
     proc = None
@@ -406,19 +412,20 @@ def run(cmd, redirect_stdout=False, redirect_stderr=False, cwd=None, input=None,
             if extra_env:
                 msg += ', extra env=%s' % extra_env
             raise CalledProcessError(
-                result.returncode, result.cmd, stdout=result.stdout,
-                stderr=result.stderr, msg=msg)
+                result.returncode, result.cmd, msg=msg,
+                stdout=ensure_text(result.stdout),
+                stderr=ensure_text(result.stderr))
     except OSError as e:
         estr = str(e)
         if e.errno == errno.EACCES:
             estr += '; does the program need `chmod a+x`?'
         if not check:
-            result = CompletedProcess(
-                args=cmd, stderr=estr.encode('utf-8'), returncode=255)
+            result = CompletedProcess(args=cmd, stderr=estr, returncode=255)
         else:
             raise CalledProcessError(
-                result.returncode, result.cmd, stdout=result.stdout,
-                stderr=result.stderr, msg=estr, exception=e)
+                result.returncode, result.cmd, msg=estr, exception=e,
+                stdout=ensure_text(result.stdout),
+                stderr=ensure_text(result.stderr))
     finally:
         if proc is not None:
             # Ensure the process is dead.
@@ -428,10 +435,8 @@ def run(cmd, redirect_stdout=False, redirect_stderr=False, cwd=None, input=None,
                                 None, None)
 
     # Make sure output is returned as a string rather than bytes.
-    if result.stdout is not None:
-        result.stdout = result.stdout.decode('utf-8', 'replace')
-    if result.stderr is not None:
-        result.stderr = result.stderr.decode('utf-8', 'replace')
+    result.stdout = ensure_text(result.stdout)
+    result.stderr = ensure_text(result.stderr)
 
     return result
 # pylint: enable=redefined-builtin,input-builtin
diff --git a/rh/utils_unittest.py b/rh/utils_unittest.py
index f3098a9..e6ecc97 100755
--- a/rh/utils_unittest.py
+++ b/rh/utils_unittest.py
@@ -161,6 +161,51 @@ class RunCommandTests(unittest.TestCase):
         self.assertEqual(u'ß', ret.stdout)
         self.assertIsNone(ret.stderr)
 
+    def test_check_false(self):
+        """Verify handling of check=False."""
+        ret = rh.utils.run(['false'], check=False)
+        self.assertNotEqual(0, ret.returncode)
+        self.assertIn('false', str(ret))
+
+        ret = rh.utils.run(['true'], check=False)
+        self.assertEqual(0, ret.returncode)
+        self.assertIn('true', str(ret))
+
+    def test_check_true(self):
+        """Verify handling of check=True."""
+        with self.assertRaises(rh.utils.CalledProcessError) as e:
+            rh.utils.run(['false'], check=True)
+        err = e.exception
+        self.assertNotEqual(0, err.returncode)
+        self.assertIn('false', str(err))
+
+        ret = rh.utils.run(['true'], check=True)
+        self.assertEqual(0, ret.returncode)
+        self.assertIn('true', str(ret))
+
+    def test_check_false_output(self):
+        """Verify handling of output capturing w/check=False."""
+        with self.assertRaises(rh.utils.CalledProcessError) as e:
+            rh.utils.run(['sh', '-c', 'echo out; echo err >&2; false'],
+                         check=True, capture_output=True)
+        err = e.exception
+        self.assertNotEqual(0, err.returncode)
+        self.assertIn('false', str(err))
+
+    def test_check_true_missing_prog_output(self):
+        """Verify handling of output capturing w/missing progs."""
+        with self.assertRaises(rh.utils.CalledProcessError) as e:
+            rh.utils.run(['./!~a/b/c/d/'], check=True, capture_output=True)
+        err = e.exception
+        self.assertNotEqual(0, err.returncode)
+        self.assertIn('a/b/c/d', str(err))
+
+    def test_check_false_missing_prog_output(self):
+        """Verify handling of output capturing w/missing progs."""
+        ret = rh.utils.run(['./!~a/b/c/d/'], check=False, capture_output=True)
+        self.assertNotEqual(0, ret.returncode)
+        self.assertIn('a/b/c/d', str(ret))
+
 
 if __name__ == '__main__':
     unittest.main()