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

We force the returned value to use strings for stdout/stderr, but the
exceptions thrown here don't do that.  We only catch the exception in
two places when the underlying command fails, so we rarely hit this
code path.  Add more unittests to make sure it works.

Bug: 185196437
Test: unittests pass
Test: repo upload in a project w/clang_format enabled & no /usr/bin/python works
Change-Id: Ic29c5dbd324a24333d99a500cc82913ae7bea93a

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()