gaia-ui-test crashes don't pass a valid test name to mozcrash.check_for_crashes()

RESOLVED DUPLICATE of bug 1059248

Status

defect
RESOLVED DUPLICATE of bug 1059248
5 years ago
5 years ago

People

(Reporter: emorley, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [affects=b2g])

Attachments

(1 obsolete attachment)

In other test jobs, the harness keeps track of the last test seen, so when there is a crash, it can pass this to mozcrash, so that the resultant error-line-with-top-frame-of-stack includes the test name.

However for crashes during gaia-ui tests, the test name is 'base.py' or 'runner.py', rather than the test name.

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42022963&tree=B2g-Inbound#error0
22:09:45     INFO -  TEST-START test_keyboard.py
22:10:02     INFO -  test_keyboard_basic (test_keyboard.TestKeyboard) ... mozcrash INFO | Downloading symbols from: https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/b2g-inbound-macosx64_gecko/1403151453/en-US/b2g-33.0a1.en-US.mac64.crashreporter-symbols.zip
22:10:17    ERROR -  PROCESS-CRASH | base.py | application crashed [@ mozilla::gl::GLBlitTextureImageHelper::BlitTextureImage(...)]

Where the expected output would be:
22:10:17    ERROR -  PROCESS-CRASH | test_keyboard.py | application crashed [@ mozilla::gl::GLBlitTextureImageHelper::BlitTextureImage(...)]
The way we call mozcrash elsewhere [1]:
> mozcrash.check_for_crashes(local_dump_dir, symbolsPath, test_name=self.lastTestSeen)

Whereas:
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/b2ginstance.py#63
63                 if mozcrash.check_for_crashes(local_dump_dir, self.symbols_path):

IMO we should get rid of the footgun in mozcrash and make the test name mandatory, but I'll file a separate bug for that.

Andrew, would you mind modifying the call here to pass the test name? Guessing we can just use http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#30 similar to how is done for the TEST-UNEXPECTED-FAILS via getInfo():
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#226

[1] http://mxr.mozilla.org/mozilla-central/search?string=mozcrash.check_for_crashes&tree=mozilla-central
Blocks: 930025
Component: Gaia::UI Tests → Marionette
Flags: needinfo?(ahalberstadt)
Product: Firefox OS → Testing
Summary: gaia-ui-test doesn't set testLastSeen correctly for crashes (instead uses 'base.py') → b2ginstance.py doesn't pass the test name when calling mozcrash.check_for_crashes()
Version: unspecified → Trunk
I'm about to land a patch that deletes b2ginstance.py.. so I vote we WONTFIX this :). The new patch should do the right thing.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(ahalberstadt)
Resolution: --- → WONTFIX
Great :-)
Resolution: WONTFIX → DUPLICATE
Duplicate of bug: 997244
Bug 997244 unfortunately didn't fix this:

https://tbpl.mozilla.org/php/getParsedLog.php?id=42123951&tree=Mozilla-Inbound
> PROCESS-CRASH | runner.py | ...

https://tbpl.mozilla.org/php/getParsedLog.php?id=42128624&tree=Mozilla-Inbound
> PROCESS-CRASH | runner.py | ...

Whilst the mozcrash.check_for_crashes() call is passes test_name:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozrunner/mozrunner/base/runner.py#182

test_name must still be None, since we get the summaries above.

Afaict the entry points to runner.py's check_for_crashes() are:

http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/b2g_desktop.py#91
91             self.runner.check_for_crashes(test_name=self.last_test)

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#695
695             if self.runner.check_for_crashes():

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#184
184                 self.runner.check_for_crashes()

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozrunner/mozrunner/base/device.py#107
107         BaseRunner.check_for_crashes(self, dump_directory=dump_dir, test_name=self.last_test)

The marionette.py#695 and runtestsb2g.py#184 instances don't pass test_name.

I think we need to:
1) Fix those to pass test name.
2) Remove the footgun from runner.py's check_for_crashes() by making the test name mandatory
3) Also make the test name mandatory in mozcrash, but I'll file another bug for that.

May you take a look at #1 and #2? :-)
Blocks: 997244
Status: RESOLVED → REOPENED
Flags: needinfo?(ahalberstadt)
Resolution: DUPLICATE → ---
Summary: b2ginstance.py doesn't pass the test name when calling mozcrash.check_for_crashes() → gaia-ui-test crashes don't pass a valid test name to mozcrash.check_for_crashes()
Yes, I can update those places. Though I'm kind of on the fence about making it mandatory. In theory mozrunner can be used for non-testing purposes and we may not have a test_name to pass in. But I guess in practice it is mostly for testing :p

I propose we make it mandatory, but call it something else. I also propose we do #2 in the same bug as #3.
Flags: needinfo?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Yes, I can update those places.

Thank you :-)

> I propose we make it mandatory, but call it something else. I also propose
> we do #2 in the same bug as #3.

Agree to both; and re the name, I thought the same in bug 1028129 - will give us greater flexibility :-)
Chris, I'm sorry to add more log parsing in marionette while you are in the middle of trying to get rid of it. Depending on how close you are to finishing, we can either block on your patch, or land this for now and update it later.. up to you.
Assignee: nobody → ahalberstadt
Status: REOPENED → ASSIGNED
Attachment #8443469 - Flags: review?(cmanchester)
Comment on attachment 8443469 [details] [diff] [review]
always pass in test_name in mozrunner.check_for_crashes

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

I say land it. This additional parsing will not break with my patch in bug 956739, but will once we have mozharness reading a raw log from stdout. When this happens, and bug 1023371 lands, associating a crash with the last test seen will be the purview of the structured log formatter.
Attachment #8443469 - Flags: review?(cmanchester) → review+
Thanks! I agree, that sounds like the best way forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/154690942f4d
Backed out for now, since the new test name was 'automation' rather than the actual test name, or the previous 'base.py' (which caused fewer false positives).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7692bed181
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2bac65d5860b
s/base.py/runner.py/
Whiteboard: [affects=b2g]
I don't suppose you know when you might be able to take another look at this? :-)
Dave, Can you add this to your queue ... IIRC your working on this space (or very near this space)
Flags: needinfo?(dave.hunt)
Pretty sure bug 1059248 will fix this.
Depends on: 1059248
Flags: needinfo?(dave.hunt)
Ed, is this still a problem? Or did bug 1059248 fix it..
Assignee: ahalberstadt → nobody
Flags: needinfo?(emorley)
Appears fixed :-)

https://bugzilla.mozilla.org/show_bug.cgi?id=1056047#c6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
No longer depends on: 1059248
Flags: needinfo?(emorley)
Resolution: --- → DUPLICATE
Duplicate of bug: 1059248
Attachment #8443469 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.