[Android] Try harder to run every gtest within each try.

[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOTRUN statuses within each try.

BUG=646223
Committed: https://crrev.com/20a1d335f2cf1e97c16e1036bb427a448db84097
Cr-Commit-Position: refs/heads/master@{#435863}

[jbudorick, 2016-12-01 05:41:40]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 05:41:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 07:19:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 07:19:23]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2016-12-01 14:57:05]

jbudorick@chromium.org changed reviewers:
+ mikecase@chromium.org, shenghuazhang@chromium.org

[jbudorick, 2016-12-01 16:32:39]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 16:33:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbudorick, 2016-12-01 16:36:41]

Description was changed from

==========
[Android] Attempt to rerun UNKNOWN gtests within a single try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL attempts to rerun tests marked UNKNOWN within a given try
s.t., hopefully, no test is left UNKNOWN.

BUG=646223
==========

to

==========
[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOT_RUN statuses within each try.

BUG=646223
==========

[jbudorick, 2016-12-01 16:36:55]

Description was changed from

==========
[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOT_RUN statuses within each try.

BUG=646223
==========

to

==========
[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOT_RUN statuses within each try.

BUG=646223
==========

[jbudorick, 2016-12-01 16:37:10]

Split UNKNOWN into UNKNOWN and NOT_RUN. ptal

[commit-bot: I haz the power, 2016-12-01 17:29:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 17:29:58]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2016-12-01 18:53:33]

Description was changed from

==========
[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOT_RUN statuses within each try.

BUG=646223
==========

to

==========
[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOTRUN statuses within each try.

BUG=646223
==========

[jbudorick, 2016-12-01 18:58:04]

ok, ready for review again.

[shenghuazhang, 2016-12-01 19:46:40]

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/bas...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/bas...
build/android/pylib/base/base_test_result.py:38: ResultType.NOTRUN]
Seems like the 'NOTRUN' ResultType isn't used by anywhere. To be more unified,
is that good to add a getter function 'GetNotrun' like the other ResultTypes?

[mikecase (-- gone --), 2016-12-01 20:42:08]

question

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:426: not_run_tests =
set(test).difference(set(r.GetName() for r in results))
Could you explain this a bit. I thought you would have NOTRUN results for tests
that were not run. Instead it looks like we have no results at all?

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_test_run.py (right):

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_test_run.py:92:
tests.test_completed()
This is super edge case, but if the slowest shard is the one to fail, then this
call to test_completed() might make tests_collection empty and let the other
shards exit the run loop.

I dont think this matters currently however. Not super sure how tests are
sharded but I think this only matters if you make it so that if there are fewer
than 256 tests, those tests get sharded into smaller groups.

[jbudorick, 2016-12-02 01:45:21]

Thanks for the reviews!

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/bas...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/bas...
build/android/pylib/base/base_test_result.py:38: ResultType.NOTRUN]
On 2016/12/01 19:46:40, shenghuazhang wrote:
> Seems like the 'NOTRUN' ResultType isn't used by anywhere. To be more unified,
> is that good to add a getter function 'GetNotrun' like the other ResultTypes?

NOTRUN was more functional in an earlier patchset; now it's primarily for
logging clarity. As nothing would currently use GetNotrun, I'd rather omit it
for now and add it if we need it.

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:426: not_run_tests =
set(test).difference(set(r.GetName() for r in results))
On 2016/12/01 20:42:08, mikecase wrote:
> Could you explain this a bit. I thought you would have NOTRUN results for
tests
> that were not run. Instead it looks like we have no results at all?

At this point, results contains only what we were able to determine from the
output or the XML -- it doesn't have anything based on the test. The NOTRUN
results for all tests are over in LocalDeviceTestRun.RunTests on the main
thread.

We could (and at one point, I did) have this create NOTRUN results for
everything in |test|, then update that set w/ |results|, then return everything
that was still NOTRUN as not_run_tests, but that's just a more convoluted
version of this.

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_test_run.py (right):

https://codereview.chromium.org/2541093004/diff/40001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_test_run.py:92:
tests.test_completed()
On 2016/12/01 20:42:08, mikecase wrote:
> This is super edge case, but if the slowest shard is the one to fail, then
this
> call to test_completed() might make tests_collection empty and let the other
> shards exit the run loop.
> 
> I dont think this matters currently however. Not super sure how tests are
> sharded but I think this only matters if you make it so that if there are
fewer
> than 256 tests, those tests get sharded into smaller groups.

Good catch. Reordered.

[shenghuazhang, 2016-12-02 02:17:49]

lgtm

[mikecase (-- gone --), 2016-12-02 02:25:33]

lgtm

[jbudorick, 2016-12-02 02:26:38]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2016-12-02 02:27:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 05:04:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480645598780490,
"parent_rev": "806e1f1ce095b022e0431e3d6bbe771cb4f3700b", "commit_rev":
"9cadac5ec3622cb4f1a6c0a8a6ba6bc3674a9e00"}

[commit-bot: I haz the power, 2016-12-02 05:04:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-02 05:10:49]

Description was changed from

==========
[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOTRUN statuses within each try.

BUG=646223
==========

to

==========
[Android] Try harder to run every gtest within each try.

Gtests on Android get sharded into 256-test chunks to mitigate
the latency of spinning up the VM while also allowing for reasonably
balanced sharding across multiple attached devices. This can have
adverse side effects when a test crashes, though, leaving the remaining
tests in a given shard unrun. This is particularly problematic when a
lot of tests crash, with dozens or hundreds of tests failing as
UNKNOWN with no logs.

This CL splits out a separate NOT_RUN status from UNKNOWN to
differentiate between tests that were never run (or that we didn't see
start) and tests that we believe started but for which we were unable to
determine what happened. It then attempts to run any tests that come
back with NOTRUN statuses within each try.

BUG=646223

Committed: https://crrev.com/20a1d335f2cf1e97c16e1036bb427a448db84097
Cr-Commit-Position: refs/heads/master@{#435863}
==========

[commit-bot: I haz the power, 2016-12-02 05:10:50]

Patchset 4 (id:??) landed as
https://crrev.com/20a1d335f2cf1e97c16e1036bb427a448db84097
Cr-Commit-Position: refs/heads/master@{#435863}