[Android] Improve error messaging for amp failures.

[Android] Improve error messaging for amp failures.

Currently, it will double report test failures, and
will improperly report crashes without a long message.

BUG=512305
Committed: https://crrev.com/2347b0e01194f15e2c4fecaaea4b2e84a0ba058a
Cr-Commit-Position: refs/heads/master@{#347785}

[rnephew (Reviews Here), 2015-08-24 18:59:50]

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

[mikecase (-- gone --), 2015-08-24 19:53:32]

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
File build/android/pylib/remote/device/remote_device_test_run.py (right):

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
build/android/pylib/remote/device/remote_device_test_run.py:373: if
_SHORT_MSG_RE.search(line):
My one comment is that you have the line ...

      if any(_SHORT_MSG_RE.search(l)
          for l in self._results['results']['output'].splitlines()):

above and its a tiny bit confusing why you have to search for _SHORT_MSG_RE
twice.

Could you do something along the lines of what I wrote below. (EDIT: now that I
wrote this out, idk if its actually any clearer  :/)

output_lines = self._results['results']['output'].splitlines()
short_error_messages  = [l for l in output_lines if _SHORT_MSG_RE.search(l)]
long_error_messages  = [l for l in output_lines if _LONG_MSG_RE.search(l)]

if short_error_messages:
  self._LogLogcat()
  # If there is a long message, use that instead.
  if long_messages:
    result_message = long_error_messages[0].split('=')[1]
  else:
    result_message = short_error_messages[0].split('=')[1]
  results.AddResult(result_message, base_test_result.ResultType.CRASH))

[rnephew (Reviews Here), 2015-08-24 20:32:51]

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
File build/android/pylib/remote/device/remote_device_test_run.py (right):

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
build/android/pylib/remote/device/remote_device_test_run.py:373: if
_SHORT_MSG_RE.search(line):
On 2015/08/24 19:53:32, mikecase wrote:
> My one comment is that you have the line ...
> 
>       if any(_SHORT_MSG_RE.search(l)
>           for l in self._results['results']['output'].splitlines()):
> 
> above and its a tiny bit confusing why you have to search for _SHORT_MSG_RE
> twice.
> 
> Could you do something along the lines of what I wrote below. (EDIT: now that
I
> wrote this out, idk if its actually any clearer  :/)
> 
> output_lines = self._results['results']['output'].splitlines()
> short_error_messages  = [l for l in output_lines if _SHORT_MSG_RE.search(l)]
> long_error_messages  = [l for l in output_lines if _LONG_MSG_RE.search(l)]
> 
> if short_error_messages:
>   self._LogLogcat()
>   # If there is a long message, use that instead.
>   if long_messages:
>     result_message = long_error_messages[0].split('=')[1]
>   else:
>     result_message = short_error_messages[0].split('=')[1]
>   results.AddResult(result_message, base_test_result.ResultType.CRASH))

Lets let John chime in and see which he likes better, or if there is a third way
he likes more.

[jbudorick, 2015-09-08 18:21:02]

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
File build/android/pylib/remote/device/remote_device_test_run.py (right):

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
build/android/pylib/remote/device/remote_device_test_run.py:373: if
_SHORT_MSG_RE.search(line):
On 2015/08/24 at 20:32:51, rnephew1 wrote:
> On 2015/08/24 19:53:32, mikecase wrote:
> > My one comment is that you have the line ...
> > 
> >       if any(_SHORT_MSG_RE.search(l)
> >           for l in self._results['results']['output'].splitlines()):
> > 
> > above and its a tiny bit confusing why you have to search for _SHORT_MSG_RE
> > twice.
> > 
> > Could you do something along the lines of what I wrote below. (EDIT: now
that I
> > wrote this out, idk if its actually any clearer  :/)
> > 
> > output_lines = self._results['results']['output'].splitlines()
> > short_error_messages  = [l for l in output_lines if _SHORT_MSG_RE.search(l)]
> > long_error_messages  = [l for l in output_lines if _LONG_MSG_RE.search(l)]
> > 
> > if short_error_messages:
> >   self._LogLogcat()
> >   # If there is a long message, use that instead.
> >   if long_messages:
> >     result_message = long_error_messages[0].split('=')[1]
> >   else:
> >     result_message = short_error_messages[0].split('=')[1]
> >   results.AddResult(result_message, base_test_result.ResultType.CRASH))
> 
> Lets let John chime in and see which he likes better, or if there is a third
way he likes more.

I think both are doing too much looping & regex searching and would prefer
something like:

_LONG_MSG_RE = re.compile(r'longMsg=(.*)$')
_SHORT_MSG_RE = re.compile(r'shortMsg=(.*)$')

crash_msg = None
for line in self._results['results']['output'].splitlines():
  m = _LONG_MSG_RE.search(line):
  if m:
    crash_msg = m.group(1)
    break

  m = _SHORT_MSG_RE.search(line)
  if m:
    crash_msg = m.group(1)

if crash_msg:
  self._LogLogcat()
  results.AddResult(...)
elif self._DidDeviceGoOffline():
  ...
else:
  ...

[rnephew (Reviews Here), 2015-09-08 19:05:17]

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
File build/android/pylib/remote/device/remote_device_test_run.py (right):

https://codereview.chromium.org/1311783005/diff/1/build/android/pylib/remote/...
build/android/pylib/remote/device/remote_device_test_run.py:373: if
_SHORT_MSG_RE.search(line):
On 2015/09/08 18:21:02, jbudorick wrote:
> On 2015/08/24 at 20:32:51, rnephew1 wrote:
> > On 2015/08/24 19:53:32, mikecase wrote:
> > > My one comment is that you have the line ...
> > > 
> > >       if any(_SHORT_MSG_RE.search(l)
> > >           for l in self._results['results']['output'].splitlines()):
> > > 
> > > above and its a tiny bit confusing why you have to search for
_SHORT_MSG_RE
> > > twice.
> > > 
> > > Could you do something along the lines of what I wrote below. (EDIT: now
> that I
> > > wrote this out, idk if its actually any clearer  :/)
> > > 
> > > output_lines = self._results['results']['output'].splitlines()
> > > short_error_messages  = [l for l in output_lines if
_SHORT_MSG_RE.search(l)]
> > > long_error_messages  = [l for l in output_lines if _LONG_MSG_RE.search(l)]
> > > 
> > > if short_error_messages:
> > >   self._LogLogcat()
> > >   # If there is a long message, use that instead.
> > >   if long_messages:
> > >     result_message = long_error_messages[0].split('=')[1]
> > >   else:
> > >     result_message = short_error_messages[0].split('=')[1]
> > >   results.AddResult(result_message, base_test_result.ResultType.CRASH))
> > 
> > Lets let John chime in and see which he likes better, or if there is a third
> way he likes more.
> 
> I think both are doing too much looping & regex searching and would prefer
> something like:
> 
> _LONG_MSG_RE = re.compile(r'longMsg=(.*)$')
> _SHORT_MSG_RE = re.compile(r'shortMsg=(.*)$')
> 
> crash_msg = None
> for line in self._results['results']['output'].splitlines():
>   m = _LONG_MSG_RE.search(line):
>   if m:
>     crash_msg = m.group(1)
>     break
> 
>   m = _SHORT_MSG_RE.search(line)
>   if m:
>     crash_msg = m.group(1)
> 
> if crash_msg:
>   self._LogLogcat()
>   results.AddResult(...)
> elif self._DidDeviceGoOffline():
>   ...
> else:
>   ...

Done.

[jbudorick, 2015-09-08 19:38:28]

lgtm

[rnephew (Reviews Here), 2015-09-08 19:39:36]

The CQ bit was checked by rnephew@chromium.org

[commit-bot: I haz the power, 2015-09-08 19:39:58]

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

[commit-bot: I haz the power, 2015-09-08 20:51:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-08 20:52:16]

Patchset 2 (id:??) landed as
https://crrev.com/2347b0e01194f15e2c4fecaaea4b2e84a0ba058a
Cr-Commit-Position: refs/heads/master@{#347785}