Closed Bug 1263425 Opened 8 years ago Closed 8 years ago

Crashes in Marionette still finish green (?!)

Categories

(Remote Protocol :: Marionette, defect)

Version 3
defect
Not set
critical

Tracking

(firefox47 fixed, firefox48 fixed, firefox-esr45 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox-esr45 --- fixed

People

(Reporter: RyanVM, Assigned: impossibus)

Details

Attachments

(1 file)

Take a look at this log, which demonstrates a clear startup crash. Yet the run shows up as green on Treeherder. That seems Really Really Bad.

http://archive.mozilla.org/pub/firefox/try-builds/ryanvm@gmail.com-d0996368bb81e116b817887b2582772b44aa317f/try-win32/try_win7-all_test-marionette-e10s-bm111-tests1-windows-build151.txt.gz
Flags: needinfo?(dburns)
Here's what happened. The mozharness script reported 'success' based on data provided by the Marionette runner: the number of failures/errors recorded (0), the numbers of passes (1) and the return code (0 because no exceptions were thrown). 

In other words, it seems to me that although Marionette runner checks for crashes at several different occasions, it never actually records them as errors or anything (?!). I guess it's counting on an exception being raised by marionette_client after a crash?

I will probably have a patch ready tomorrow -- just need to add some unit tests for this aspect of Marionette runner first.
Assignee: nobody → mjzffr
Status: NEW → ASSIGNED
Cool, it works: http://archive.mozilla.org/pub/firefox/try-builds/mjzffr@gmail.com-3dd4ef738c6759ef6f76fccf4f934f0c1dd5f0c9/try-win32/try_win7-all_test-marionette-bm109-tests1-windows-build236.txt.gz

The above log shows the same scenario as Ryan's original report: a crash happens mid-test, but the test is reported as a PASS. However, the crash detection increments the error count, so runner.failed is non-zero, yay!
Flags: needinfo?(dburns)
This addresses the case where there is a crash immediately after a passing test,
in which case the test suite stops and no failure is recorded.

Review commit: https://reviewboard.mozilla.org/r/47219/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47219/
Attachment #8742475 - Flags: review?(dburns)
Comment on attachment 8742475 [details]
MozReview Request: Bug 1263425 - Include crashes in Marionette runner's error/failure count; r?AutomatedTester

https://reviewboard.mozilla.org/r/47219/#review43883
Attachment #8742475 - Flags: review?(dburns) → review+
https://hg.mozilla.org/mozilla-central/rev/8b071443fc72
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
David and Maja, we should consider this fix for backporting to beta, release, and esr45. Ryan agreed on that via IRC. David will you do that?
Flags: needinfo?(dburns)
Flags: needinfo?(dburns)
Whiteboard: checkin-needed-esr45
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.