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

Status: RESOLVED DUPLICATE of bug 1059248

(Remote Protocol :: Marionette, defect)

Description

[Ed Morley [:emorley]]

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(...)]

Comment 1

[Ed Morley [:emorley]]

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

Comment 2

[Andrew Halberstadt [:ahal]]

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.

Comment 3

[Ed Morley [:emorley]]

Great :-)

Comment 4

[Ed Morley [:emorley]]

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? :-)

Comment 5

[Andrew Halberstadt [:ahal]]

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.

Comment 6

[Ed Morley [:emorley]]

(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 :-)

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: always pass in test_name in mozrunner.check_for_crashes

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.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 9

[Andrew Halberstadt [:ahal]]

Thanks! I agree, that sounds like the best way forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/154690942f4d

Comment 10

[Andrew Halberstadt [:ahal]]

Add missing 'self':
https://hg.mozilla.org/integration/mozilla-inbound/rev/3399263d2702

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

[Ed Morley [:emorley]]

s/base.py/runner.py/

Comment 13

[Ed Morley [:emorley]]

I don't suppose you know when you might be able to take another look at this? :-)

Comment 14

[James Lal [:lightsofapollo]]

Dave, Can you add this to your queue ... IIRC your working on this space (or very near this space)

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Pretty sure bug 1059248 will fix this.

Comment 16

[Andrew Halberstadt [:ahal]]

Ed, is this still a problem? Or did bug 1059248 fix it..

Comment 17

[Ed Morley [:emorley]]

Appears fixed :-)

https://bugzilla.mozilla.org/show_bug.cgi?id=1056047#c6