Show more stack trace in instrumentation tests.

Show more stack trace in instrumentation tests.

When unknown exceptions are raised during instrumentation tests,
catch the exception and output its stack trace as part of the log
rather than just displaying 'CRASHED' or 'UNKNOWN' statuses.

This should also show up in buildbot logs.

BUG=567841
Committed: https://crrev.com/e6e09ec25d398d954fe93865b681f711a1c22322
Cr-Commit-Position: refs/heads/master@{#365586}

[Peter Wen, 2015-12-09 21:03:29]

Description was changed from

==========
Show more stacktrace in instrumentation tests.

When unknown exceptions are raised during instrumentation tests,
catch the exception and output its stacktrace as part of the log
rather than just displaying 'CRASHED' or 'UNKNOWN' statuses.

This should also show up in buildbot logs.

BUG=567841
==========

to

==========
Show more stack trace in instrumentation tests.

When unknown exceptions are raised during instrumentation tests,
catch the exception and output its stack trace as part of the log
rather than just displaying 'CRASHED' or 'UNKNOWN' statuses.

This should also show up in buildbot logs.

BUG=567841
==========

[Peter Wen, 2015-12-11 20:54:32]

wnwen@chromium.org changed reviewers:
+ jbudorick@chromium.org, yfriedman@chromium.org

[Peter Wen, 2015-12-11 20:54:32]

This CL adds an unhandled exception handler to Chrome instrumentation tests and
sends the stack trace through am so that test_runner can display it.

Example STDIO here (after I disabled lots of StrictMode whitelists in patch #7):
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...

I've added a custom message for StrictMode violations as their exceptions happen
after the fact and the full detailed trace is available in the logcat.

PTAL. :)

[jbudorick, 2015-12-11 21:04:30]

https://codereview.chromium.org/1514453007/diff/140001/build/android/pylib/in...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1514453007/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:107:
test_class = bundle.get('class', '') or test_class
nit if this logic sticks around: just do

  test_class = bundle.get('class', test_class)

https://codereview.chromium.org/1514453007/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:108:
test_method = bundle.get('test', '') or test_method
nit: same

https://codereview.chromium.org/1514453007/diff/140001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
(right):

https://codereview.chromium.org/1514453007/diff/140001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:133:
Bundle resultsBundle = new Bundle();
Why not put the class & test in the Bundle rather than persisting class & test
name between statuses? I'd prefer to be consistent with how Instrumentation
currently sends statuses in this regard.

[Peter Wen, 2015-12-11 22:07:21]

Thanks for the quick initial review! PTAL.

https://codereview.chromium.org/1514453007/diff/140001/build/android/pylib/in...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1514453007/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:107:
test_class = bundle.get('class', '') or test_class
On 2015/12/11 21:04:30, jbudorick wrote:
> nit if this logic sticks around: just do
> 
>   test_class = bundle.get('class', test_class)

Acknowledged.

https://codereview.chromium.org/1514453007/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:108:
test_method = bundle.get('test', '') or test_method
On 2015/12/11 21:04:30, jbudorick wrote:
> nit: same

Acknowledged.

https://codereview.chromium.org/1514453007/diff/140001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
(right):

https://codereview.chromium.org/1514453007/diff/140001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:133:
Bundle resultsBundle = new Bundle();
On 2015/12/11 21:04:30, jbudorick wrote:
> Why not put the class & test in the Bundle rather than persisting class & test
> name between statuses? I'd prefer to be consistent with how Instrumentation
> currently sends statuses in this regard.

Sgtm. Done.

[jbudorick, 2015-12-11 22:25:06]

build/ lgtm

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
(right):

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:77:
import java.lang.Thread.UncaughtExceptionHandler;
Do we have to import this? Is it not accessible directly through Thread?

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:131:
stackTrace += "\nSearch logcat for \"StrictMode policy violation\" for full
stack.";
Do we not have access to the full stack here? Is it too unwieldy to include in
the Bundle?

[Peter Wen, 2015-12-14 16:29:36]

What do you think about adding "--strict-mode=testing" as a follow-up CL after
this lands, John?

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
(right):

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:77:
import java.lang.Thread.UncaughtExceptionHandler;
On 2015/12/11 22:25:06, jbudorick wrote:
> Do we have to import this? Is it not accessible directly through Thread?

Done.

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:131:
stackTrace += "\nSearch logcat for \"StrictMode policy violation\" for full
stack.";
On 2015/12/11 22:25:06, jbudorick wrote:
> Do we not have access to the full stack here? Is it too unwieldy to include in
> the Bundle?

The only place that StrictMode stack trace is available to us is logcat, as the
exception is thrown after the violating code already ran.

I plan to add code to grep logcat for the last StrictMode stack trace but I'd
rather it not block turning this on. Adding --strict-mode=testing ASAP would
help with preventing new violations, and it's easy to repro and grep logcat
manually.

[jbudorick, 2015-12-15 02:02:04]

adding --strict-mode=testing sgtm. there are a couple of different ways to do
that, though.

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
(right):

https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:131:
stackTrace += "\nSearch logcat for \"StrictMode policy violation\" for full
stack.";
On 2015/12/14 16:29:36, Peter Wen wrote:
> On 2015/12/11 22:25:06, jbudorick wrote:
> > Do we not have access to the full stack here? Is it too unwieldy to include
in
> > the Bundle?
> 
> The only place that StrictMode stack trace is available to us is logcat, as
the
> exception is thrown after the violating code already ran.
> 
> I plan to add code to grep logcat for the last StrictMode stack trace but I'd
> rather it not block turning this on. Adding --strict-mode=testing ASAP would
> help with preventing new violations, and it's easy to repro and grep logcat
> manually.

Note that there's going to be some movement in the near future in how our test
harnesses handle logcat due to https://codereview.chromium.org/1484253003/. You
may want to wait for that to land before doing too much in that regard.

[Peter Wen, 2015-12-16 16:02:13]

On 2015/12/15 02:02:04, jbudorick wrote:
> adding --strict-mode=testing sgtm. there are a couple of different ways to do
> that, though.
> 
>
https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
> File
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
> (right):
> 
>
https://codereview.chromium.org/1514453007/diff/160001/chrome/test/android/ja...
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:131:
> stackTrace += "\nSearch logcat for \"StrictMode policy violation\" for full
> stack.";
> On 2015/12/14 16:29:36, Peter Wen wrote:
> > On 2015/12/11 22:25:06, jbudorick wrote:
> > > Do we not have access to the full stack here? Is it too unwieldy to
include
> in
> > > the Bundle?
> > 
> > The only place that StrictMode stack trace is available to us is logcat, as
> the
> > exception is thrown after the violating code already ran.
> > 
> > I plan to add code to grep logcat for the last StrictMode stack trace but
I'd
> > rather it not block turning this on. Adding --strict-mode=testing ASAP would
> > help with preventing new violations, and it's easy to repro and grep logcat
> > manually.
> 
> Note that there's going to be some movement in the near future in how our test
> harnesses handle logcat due to https://codereview.chromium.org/1484253003/.
You
> may want to wait for that to land before doing too much in that regard.

Great, thanks for the heads up!

[Yaron, 2015-12-16 17:15:05]

lgtm

[Peter Wen, 2015-12-16 18:10:16]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2015-12-16 18:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1514453007/#ps180001
(title: "Better imports.")

[commit-bot: I haz the power, 2015-12-16 18:11:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1514453007/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1514453007/180001

[commit-bot: I haz the power, 2015-12-16 19:20:18]

Description was changed from

==========
Show more stack trace in instrumentation tests.

When unknown exceptions are raised during instrumentation tests,
catch the exception and output its stack trace as part of the log
rather than just displaying 'CRASHED' or 'UNKNOWN' statuses.

This should also show up in buildbot logs.

BUG=567841
==========

to

==========
Show more stack trace in instrumentation tests.

When unknown exceptions are raised during instrumentation tests,
catch the exception and output its stack trace as part of the log
rather than just displaying 'CRASHED' or 'UNKNOWN' statuses.

This should also show up in buildbot logs.

BUG=567841
==========

[commit-bot: I haz the power, 2015-12-16 19:20:19]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-12-16 19:21:29]

Description was changed from

==========
Show more stack trace in instrumentation tests.

When unknown exceptions are raised during instrumentation tests,
catch the exception and output its stack trace as part of the log
rather than just displaying 'CRASHED' or 'UNKNOWN' statuses.

This should also show up in buildbot logs.

BUG=567841
==========

to

==========
Show more stack trace in instrumentation tests.

When unknown exceptions are raised during instrumentation tests,
catch the exception and output its stack trace as part of the log
rather than just displaying 'CRASHED' or 'UNKNOWN' statuses.

This should also show up in buildbot logs.

BUG=567841

Committed: https://crrev.com/e6e09ec25d398d954fe93865b681f711a1c22322
Cr-Commit-Position: refs/heads/master@{#365586}
==========

[commit-bot: I haz the power, 2015-12-16 19:21:30]

Patchset 10 (id:??) landed as
https://crrev.com/e6e09ec25d398d954fe93865b681f711a1c22322
Cr-Commit-Position: refs/heads/master@{#365586}