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

testing_support/thread_watcher.py

 # Copyright (c) 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 
+import logging
 import sys
 import threading
 import traceback
 import unittest
 
 
 class ThreadWatcherMixIn(object):
   def setUp(self):
     self._pre_test_threads = [t.ident for t in threading.enumerate()]
 
   def tearDown(self):
     post_test_threads = threading.enumerate()
     new_threads = [t for t in post_test_threads
                    if t.ident not in self._pre_test_threads]
     if new_threads:
       details = []
       for th in new_threads:
         details.append('\nThread %s stacktrace:\n' % th)
         try:
           details.extend(traceback.format_stack(sys._current_frames()[t.ident]))
         except Exception:
           if t.ident in sys._current_frames():
             raise
           # This can happen due to threads starting or stopping concurrently.
           details.append('  Thread stopped while acquiring stacktrace.\n')
       details = ''.join(details)
 
-      self.fail('Found %d running thread(s) after the test.\n%s' % (
-        len(new_threads), details))
+      msg = 'Found %d running thread(s) after the test.\n%s' % (
+          len(new_threads), details)
+      if self._should_fail():
+        self.fail(msg)  # This raises exception.
+      # Test failed before this check, so don't raise another exception

[Sergiy Byelozyorov, 2016/07/07 07:05:01]

IMHO this comment should be before line 36 and IMHO we shouldn't  print warnings
below as user will see thread errors after fixing their code anyway.

[tandrii(chromium), 2016/07/07 11:47:05]

Done.

+      # overwriting the original one, and making it harder to debug.
+      # Besides, that exception might be the reason cleanup didn't happen.
+      logging.warn(msg)
+      logging.warn('Note: extra running threads might be due to your test '
+                   'failure, which prevented *your cleanup code* from '
+                   'executing.')
 
+  def _should_fail(self):

[Sergiy Byelozyorov, 2016/07/07 07:05:01]

suggestion: rename to _test_likely_passed(self):

[tandrii(chromium), 2016/07/07 11:47:05]

Done.

+    """Conservatively returns True if current test has likely not failed yet."""

[Sergiy Byelozyorov, 2016/07/07 07:05:01]

How about this?

"""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."""

[tandrii(chromium), 2016/07/07 11:47:05]

Done.

+    # 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.
+    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):
   """Base unittest class that fails on leaked threads after the test."""
   def setUp(self):
     super(TestCase, self).setUp()
     ThreadWatcherMixIn.setUp(self)
 
   def tearDown(self):
     ThreadWatcherMixIn.tearDown(self)
     super(TestCase, self).tearDown()