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

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

This prevents hiding the likely real root cause of the failure.

Does not depend, but works with https://codereview.chromium.org/2121343004
change to expect_tests.

R=machenbach@chromium.org, sergiyb@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/testing/testing_support/+/5f1ddf31502b99f02679e13665cac7db15309ea9

[tandrii(chromium), 2016-07-06 16:35:25]

Description was changed from

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

This prevents hiding the likely real root cause of the failure.

R=sergiyb@chromium.org
BUG=
==========

to

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

This prevents hiding the likely real root cause of the failure.

Does not depend, but works with https://codereview.chromium.org/2121343004 
change to expect_tests.

R=sergiyb@chromium.org
BUG=
==========

[tandrii(chromium), 2016-07-06 16:36:29]

PTAL

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

lgtm w/ suggestions

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
File testing_support/thread_watcher.py (right):

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
testing_support/thread_watcher.py:38: # Test failed before this check, so don't
raise another exception
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.

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
testing_support/thread_watcher.py:46: def _should_fail(self):
suggestion: rename to _test_likely_passed(self):

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
testing_support/thread_watcher.py:47: """Conservatively returns True if current
test has likely not failed yet."""
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! And added tests.

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
File testing_support/thread_watcher.py (right):

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
testing_support/thread_watcher.py:38: # Test failed before this check, so don't
raise another exception
On 2016/07/07 07:05:01, Sergiy Byelozyorov wrote:
> 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.

Done.

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
testing_support/thread_watcher.py:46: def _should_fail(self):
On 2016/07/07 07:05:01, Sergiy Byelozyorov wrote:
> suggestion: rename to _test_likely_passed(self):

Done.

https://codereview.chromium.org/2127943002/diff/1/testing_support/thread_watc...
testing_support/thread_watcher.py:47: """Conservatively returns True if current
test has likely not failed yet."""
On 2016/07/07 07:05:01, Sergiy Byelozyorov wrote:
> 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."""

Done.

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

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

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

https://codereview.chromium.org/2127943002/diff/20001/testing_support/tests/t...
File testing_support/tests/thread_watcher_test.py (right):

https://codereview.chromium.org/2127943002/diff/20001/testing_support/tests/t...
testing_support/tests/thread_watcher_test.py:89: def
test_extra_threads_unittest_pass(self):
Same method signature as the method above? Also the two below. Does this execute
both methods or only one? Maybe the upper one should be called *_fail?

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
File testing_support/thread_watcher.py (right):

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
testing_support/thread_watcher.py:14: def setUp(self):
Can setUp be called concurrently?

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
testing_support/thread_watcher.py:21: if not self._test_likely_passed():
readability nit: Maybe negate the logic in the method as that's the main use
case? E.g.

if self._has_known_test_failure():
  ...

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
testing_support/thread_watcher.py:43: def _test_likely_passed(self):
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]

https://codereview.chromium.org/2127943002/diff/20001/testing_support/tests/t...
File testing_support/tests/thread_watcher_test.py (right):

https://codereview.chromium.org/2127943002/diff/20001/testing_support/tests/t...
testing_support/tests/thread_watcher_test.py:89: def
test_extra_threads_unittest_pass(self):
On 2016/07/07 12:30:41, Michael Achenbach wrote:
> Same method signature as the method above? Also the two below. Does this
execute
> both methods or only one? Maybe the upper one should be called *_fail?

Done.

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
File testing_support/thread_watcher.py (right):

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
testing_support/thread_watcher.py:14: def setUp(self):
On 2016/07/07 12:30:41, Michael Achenbach wrote:
> Can setUp be called concurrently?

nope, at least not in the same thread on the same instance of TestCase.

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
testing_support/thread_watcher.py:21: if not self._test_likely_passed():
On 2016/07/07 12:30:41, Michael Achenbach wrote:
> readability nit: Maybe negate the logic in the method as that's the main use
> case? E.g.
> 
> if self._has_known_test_failure():
>   ... 

Good idea! Done.

https://codereview.chromium.org/2127943002/diff/20001/testing_support/thread_...
testing_support/thread_watcher.py:43: def _test_likely_passed(self):
On 2016/07/07 12:30:41, Michael Achenbach wrote:
> 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.

That's fair. I've renamed it.

[Michael Achenbach, 2016-07-07 12:59:42]

lgtm

[tandrii(chromium), 2016-07-07 13:00:08]

Description was changed from

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

This prevents hiding the likely real root cause of the failure.

Does not depend, but works with https://codereview.chromium.org/2121343004 
change to expect_tests.

R=sergiyb@chromium.org
BUG=
==========

to

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

This prevents hiding the likely real root cause of the failure.

Does not depend, but works with https://codereview.chromium.org/2121343004
change to expect_tests.

R=machenbach@chromium.org, sergiyb@chromium.org
BUG=

Committed:
https://chromium.googlesource.com/infra/testing/testing_support/+/5f1ddf31502...
==========

[tandrii(chromium), 2016-07-07 13:00:10]

Committed patchset #3 (id:40001) manually as
5f1ddf31502b99f02679e13665cac7db15309ea9 (presubmit successful).

[Sergiy Byelozyorov, 2016-07-08 07:36:41]

lgtm