diff options
author | Mike Frysinger <vapier@google.com> | 2021-04-14 22:11:24 -0400 |
---|---|---|
committer | Mike Frysinger <vapier@google.com> | 2021-04-14 22:11:24 -0400 |
commit | a89a1380979c491642d8e6080969ce54b0f38031 (patch) | |
tree | e7fa9b5fd46ec0efdc9f0ee7a4881202664effbd | |
parent | a3ae3b069e87f3bcc3073409a2d7c64e50abe20b (diff) | |
download | repohooks-a89a1380979c491642d8e6080969ce54b0f38031.tar.gz |
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
-rw-r--r-- | rh/utils.py | 25 | ||||
-rwxr-xr-x | rh/utils_unittest.py | 45 |
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() |