From de9e3cabe60e57f3b7f923e59aed8cb4f87e2f0a Mon Sep 17 00:00:00 2001
From: Edward Lemur <ehmaldonado@chromium.org>
Date: Thu, 24 Oct 2019 21:13:31 +0000
Subject: [PATCH] presubmit: Make ThreadPool surface exceptions on CallCommand.

Exceptions other than OSError are not surfaced.
This caused errors like this to be printed, but not block presubmit,
allowing bugs to sneak in.

Exception in thread Thread-8:
Traceback (most recent call last):
  File "C:\b\s\w\ir\cipd_bin_packages\cpython\bin\Lib\threading.py", line 801, in __bootstrap_inner
    self.run()
  File "C:\b\s\w\ir\cipd_bin_packages\cpython\bin\Lib\threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "C:\b\s\w\ir\kitchen-checkout\depot_tools\presubmit_support.py", line 199, in _WorkerFn
    result = self.CallCommand(test)
  File "C:\b\s\w\ir\kitchen-checkout\depot_tools\presubmit_support.py", line 170, in CallCommand
    p = subprocess.Popen(cmd, **test.kwargs)
  File "C:\b\s\w\ir\kitchen-checkout\depot_tools\subprocess2.py", line 143, in __init__
    super(Popen, self).__init__(args, **kwargs)
  File "C:\b\s\w\ir\cipd_bin_packages\cpython\bin\Lib\subprocess.py", line 390, in __init__
    errread, errwrite)
  File "C:\b\s\w\ir\cipd_bin_packages\cpython\bin\Lib\subprocess.py", line 640, in _execute_child
    startupinfo)
TypeError: environment can only contain strings

https://logs.chromium.org/logs/infra/buildbucket/cr-buildbucket.appspot.com/8898840708364523888/+/steps/presubmit/0/stdout

Change-Id: I34e65d8c0050eed7ed26fd782e0a5dc8616f30f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1877051
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Anthony Polito <apolito@google.com>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
---
 presubmit_support.py        |  7 +++++++
 tests/presubmit_unittest.py | 41 +++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/presubmit_support.py b/presubmit_support.py
index 934f72e35a..2fa71c0fa9 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -34,6 +34,7 @@ import tempfile  # Exposed through the API.
 import threading
 import time
 import types
+import traceback
 import unittest  # Exposed through the API.
 from warnings import warn
 
@@ -174,6 +175,12 @@ class ThreadPool(object):
       duration = time.time() - start
       return test.message(
           '%s exec failure (%4.2fs)\n   %s' % (test.name, duration, e))
+    except Exception as e:
+      duration = time.time() - start
+      return test.message(
+          '%s exec failure (%4.2fs)\n%s' % (
+              test.name, duration, traceback.format_exc()))
+
     if p.returncode != 0:
       return test.message(
           '%s (%4.2fs) failed\n%s' % (test.name, duration, stdout))
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 4790ea7eaa..0b0cfaef49 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -2892,6 +2892,47 @@ the current line as well!
     self.assertIsNone(commands[0].info)
 
 
+class ThreadPoolTest(unittest.TestCase):
+  def setUp(self):
+    super(ThreadPoolTest, self).setUp()
+    mock.patch('subprocess2.Popen').start()
+    mock.patch('presubmit_support.sigint_handler').start()
+    presubmit.sigint_handler.wait.return_value = ('stdout', '')
+    self.addCleanup(mock.patch.stopall)
+
+  def testSurfaceExceptions(self):
+    def FakePopen(cmd, **kwargs):
+      if cmd[0] == '3':
+        raise TypeError('TypeError')
+      if cmd[0] == '4':
+        raise OSError('OSError')
+      if cmd[0] == '5':
+        return mock.Mock(returncode=1)
+      return mock.Mock(returncode=0)
+    subprocess.Popen.side_effect = FakePopen
+
+    mock_tests = [
+        presubmit.CommandData(
+            name=str(i),
+            cmd=[str(i)],
+            kwargs={},
+            message=lambda x: x,
+        )
+        for i in range(10)
+    ]
+
+    t = presubmit.ThreadPool(1)
+    t.AddTests(mock_tests)
+    messages = sorted(t.RunAsync())
+
+    self.assertEqual(3, len(messages))
+    self.assertIn(
+      '3 exec failure (0.00s)\nTraceback (most recent call last):',
+      messages[0])
+    self.assertEqual('4 exec failure (0.00s)\n   OSError', messages[1])
+    self.assertEqual('5 (0.00s) failed\nstdout', messages[2])
+
+
 if __name__ == '__main__':
   import unittest
   unittest.main()