Thread Watcher: don't fail when test already failed.

testing_support/thread_watcher.py

Index: testing_support/thread_watcher.py
diff --git a/testing_support/thread_watcher.py b/testing_support/thread_watcher.py
index ca5180bad59f101c4da41700586ec3b6663d86b5..52007949c2a94d58e9f1f0d14efd3c77bddf0af9 100644
--- a/testing_support/thread_watcher.py
+++ b/testing_support/thread_watcher.py
@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 
+import logging
 import sys
 import threading
 import traceback
@@ -14,6 +15,12 @@ class ThreadWatcherMixIn(object):
     self._pre_test_threads = [t.ident for t in threading.enumerate()]
 
   def tearDown(self):
+    # If test failed before this check, don't raise another exception
+    # overwriting the original one, and making it harder to debug.
+    # Besides, that exception might be the reason cleanup didn't happen.
+    if not self._test_likely_passed():

[Michael Achenbach, 2016/07/07 12:30:41]

readability nit: Maybe negate the logic in the method as that's the main use
case? E.g.

if self._has_known_test_failure():
  ... 

[tandrii(chromium), 2016/07/07 12:56:39]

Good idea! Done.

+      return
+
     post_test_threads = threading.enumerate()
     new_threads = [t for t in post_test_threads
                    if t.ident not in self._pre_test_threads]
@@ -31,8 +38,25 @@ class ThreadWatcherMixIn(object):
       details = ''.join(details)
 
       self.fail('Found %d running thread(s) after the test.\n%s' % (
-        len(new_threads), details))
+                len(new_threads), details))
+
+  def _test_likely_passed(self):

[Michael Achenbach, 2016/07/07 12:30:41]

I'm not so happy about the method name. Doesn't look like there's something
probabilistic about this. The "likelihood" seems to depend solely on the API of
unittest and expect_tests. E.g. the default case is taken if the expected
attributes don't exist.

[tandrii(chromium), 2016/07/07 12:56:39]

That's fair. I've renamed it.

+    """Conservatively returns True if current test has likely not failed yet.
 
+    This is necessary to determine whether we should fail the test when finding
+    stray threads. If the test has failed in some other way, we should avoid
+    throwing an exception because it will mask the original error.
+    """
+    # When run using unittest runner,
+    # self._resultForDoCleanups is set to internal unittest object.
+    if hasattr(self, '_resultForDoCleanups') and self._resultForDoCleanups:
+      return self._resultForDoCleanups.wasSuccessful()
+    # expect_tests runner does so differently (see
+    # https://codereview.chromium.org/2121343004).
+    if hasattr(self, '_test_failed_with_exception'):
+      return not self._test_failed_with_exception
+    # Default case - be conservative.
+    return True
 
 
 class TestCase(unittest.TestCase, ThreadWatcherMixIn):