Last Comment Bug 757838 - remoteautomation.py doesn't set lastTestSeen, so Android crashes fall back to the generic "PROCESS-CRASH | automation.py | application crashed (minidump found)"
: remoteautomation.py doesn't set lastTestSeen, so Android crashes fall back to...
Status: RESOLVED FIXED
[sheriff-want]
:
Product: Testing
Classification: Components
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla19
Assigned To: Ed Morley [:emorley]
:
:
Mentors:
Depends on:
Blocks: 778688 691425 792873
  Show dependency treegraph
 
Reported: 2012-05-23 07:11 PDT by Ed Morley [:emorley]
Modified: 2012-11-04 03:04 PST (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed


Attachments
Part 1: Cleanup (2.74 KB, patch)
2012-11-02 08:18 PDT, Ed Morley [:emorley]
jmaher: review+
Details | Diff | Splinter Review
Part 2: Set lastTestSeen (4.74 KB, patch)
2012-11-02 09:17 PDT, Ed Morley [:emorley]
jmaher: review+
Details | Diff | Splinter Review
Part 3: B2G - Cleanup (2.01 KB, patch)
2012-11-02 10:39 PDT, Ed Morley [:emorley]
jgriffin: review+
Details | Diff | Splinter Review
Part 4: B2G - Set lastTestSeen (2.14 KB, patch)
2012-11-02 10:47 PDT, Ed Morley [:emorley]
jgriffin: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2012-05-23 07:11:33 PDT
Example:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cb27363cfba1&jobname=Android%20XUL%20Tegra%20250%20mozilla-inbound%20opt%20test%20reftest-2

TBPL only shows:
> PROCESS-CRASH | automation.py | application crashed (minidump found)
> Thread 4 (crashed)

Since the log contains:
{
REFTEST TEST-START | http://10.250.48.212:30104/tests/layout/reftests/font-inflation/threshold-select-combobox-contents-under-2.html | 1607 / 2542 (63%)
INFO | automation.py | Application ran for: 0:04:31.654848
INFO | automation.py | Reading PID log: /tmp/tmp41NaDUpidlog
getting files in '/mnt/sdcard/tests/reftest/profile/minidumps/'
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fx-team-android-xul/1337770945/fennec-15.0a1.en-US.android-arm.crashreporter-symbols.zip
PROCESS-CRASH | automation.py | application crashed (minidump found)
}

More useful would be something in the log similar to:
> PROCESS-CRASH | http://10.250.48.212:30104/tests/layout/reftests/font-inflation/threshold-select-combobox-contents-under-2.html | application crashed (minidump found)
Comment 1 Ed Morley [:emorley] 2012-07-25 05:28:41 PDT
Ok, so the "PROCESS-CRASH | automation.py | application crashed (minidump found)" comes from:
http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#170

...rather than automation.py.in (shows how little I know about the test harnesses lol), so I'm presuming is just the fallback when automation.py has failed miserably to output the crash message with testname.

Either way, having this fixed would make people much more likely to pay attention to Android crashes and actually star them.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-10-08 06:29:09 PDT
You were close! The testName that gets passed into that method comes from lastTestSeen in automation.py:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#882

I suspect this just isn't implemented in remoteautomation.py.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-10-08 06:29:47 PDT
Yeah:
http://mxr.mozilla.org/mozilla-central/source/build/mobile/remoteautomation.py#79

That doesn't seem to ever get set in remoteautomation.py.
Comment 4 Ed Morley [:emorley] 2012-10-08 08:37:37 PDT
Thank you for the guidance Ted :-)

(Also seems to affect b2gautomation.py)
Comment 5 Ed Morley [:emorley] 2012-11-02 08:18:33 PDT
Created attachment 677768 [details] [diff] [review]
Part 1: Cleanup

* Makes waitForFinish()'s parameter names consistent with those in automation.py.in & b2gautomation.py
* Moves the final |print proc.stdout| to RProcess::Wait so it reads more clearly (On my first read through of waitForFinish() I didn't realise we incrementally output stdout in RProcess::Wait as well, so thought that final print was the only print, which changed the required solution somewhat. This hopefully avoids anyone else having the same misunderstanding)
* Other hopefully self-explanatory changes.
Comment 6 Ed Morley [:emorley] 2012-11-02 09:17:03 PDT
Created attachment 677790 [details] [diff] [review]
Part 2: Set lastTestSeen

Since the new log content chunks can be fairly large (eg one of the ones I looked at was ~10,000 lines) I also tried a different variation on the regex to see how it compared:
> re1 = re.compile(r".*TEST-START \| ([^\s]*)", re.DOTALL)
> re1.match(newLogContent).group(1)
...but this one was about 7% slower.

Try run (with extra debugging output to confirm correct value of lastTestSeen):
https://tbpl.mozilla.org/?tree=Try&rev=bbc5cf55693a
Comment 7 Joel Maher ( :jmaher) 2012-11-02 09:23:58 PDT
Comment on attachment 677768 [details] [diff] [review]
Part 1: Cleanup

Review of attachment 677768 [details] [diff] [review]:
-----------------------------------------------------------------

I would want good try server coverage on this (reftest/mochitest/robocop) before calling this good.  From the changes here, I don't see any big problems.

::: build/mobile/remoteautomation.py
@@ +207,5 @@
>                  if (timer > timeout):
>                      break
>  
> +            # Flush anything added to stdout during the sleep
> +            print self.stdout

self.stdout or proc.stdout?
Comment 8 Ed Morley [:emorley] 2012-11-02 09:27:59 PDT
> I would want good try server coverage on this (reftest/mochitest/robocop)
> before calling this good.

Yeah agree, will send to try once I have the b2gautomation.py part done :-)

> > +            # Flush anything added to stdout during the sleep
> > +            print self.stdout
> 
> self.stdout or proc.stdout?

The 8 lines of context in this patch unfortunately doesn't show this clearly, but the lines being added are inside RProcess's wait(), so we need self.stdout.
Comment 9 Joel Maher ( :jmaher) 2012-11-02 09:28:53 PDT
Comment on attachment 677790 [details] [diff] [review]
Part 2: Set lastTestSeen

Review of attachment 677790 [details] [diff] [review]:
-----------------------------------------------------------------

nice!
Comment 10 Ed Morley [:emorley] 2012-11-02 10:39:54 PDT
Created attachment 677815 [details] [diff] [review]
Part 3: B2G - Cleanup

* Removes didTimeout, since it is never used (carry-over from automation.py.in).
* s/done/responseDueBy/g to make its purpose clearer.
Comment 11 Ed Morley [:emorley] 2012-11-02 10:47:18 PDT
Created attachment 677817 [details] [diff] [review]
Part 4: B2G - Set lastTestSeen

Same intent as the part 2 patch, but for b2gautomation.py.
Comment 13 Ed Morley [:emorley] 2012-11-02 12:00:43 PDT
Landed parts 1+2 for now, to ease the sheriffing pain:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10486f492c02
https://hg.mozilla.org/integration/mozilla-inbound/rev/415dc90118ab

Note You need to log in before you can comment on or make changes to this bug.