Bug 150056 – run-webkit-tests does not copy all crash logs for layout test failures on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

When run-webkit-tests runs layout tests on the iOS Simualtor, it does not collect all crash logs when generating the layout test summary page. The issue is that the process ID for DumpRenderTree.app and WebKitTestRunner.app is known to LayoutTestRelay, but not run-webkit-tests.

A better approach may be to collect all crash logs created between the start and end time for the run, then associate them with layout test crashes based on the "Application Specific Information" section which contains the name of the crashing test:

Application Specific Information:
CRASHING TEST: fast/events/keydown-leftright-keys.html

Note that run-webkit-tests may retry crashing tests multiple times, so there may be a one-to-many relationship of a layout test to multiple crash logs.

A separate section should be added in the results.html file to indicate these crashes.

[Aakash Jain]

also see <rdar://problem/22239750>

[Aakash Jain]

Created attachment 262955 [details]
Proposed patch

[Dana Burkart]

Unofficial r=me (I don't have reviewer status).

[Alexey Proskuryakov]

Comment on attachment 262955 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262955&action=review

> Tools/Scripts/webkitpy/common/system/crashlogs.py:122
> +            return  basename.endswith(".crash")

Why are there two spaces here?

> Tools/Scripts/webkitpy/common/system/crashlogs.py:146
> +                    if test_name == "":
> +                        test_name = process_name + "-" + str(pid)

This is not great naming - it's not "test name" in this case.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:310
> +                if not (any(process[0] == test for process in crashed_processes)):

Are the outer parentheses necessary?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:45
> +(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, TIMEOUT, CRASH, CRASH_OTHER, SKIP, WONTFIX,

Don't we need to add support for this new result everywhere? Bot watcher's dashboard and flakiness dashboard come to mind first. Maybe EWS too.

But since these crashes are already in a separate section of the JSON, I don't see why we need a new result.

[David Kilzer (:ddkilzer)]

Comment on attachment 262955 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262955&action=review

r=me, but please review comments.

>> Tools/Scripts/webkitpy/common/system/crashlogs.py:122
>> +            return  basename.endswith(".crash")
> 
> Why are there two spaces here?

Please remove the double space before landing.

>> Tools/Scripts/webkitpy/common/system/crashlogs.py:146
>> +                        test_name = process_name + "-" + str(pid)
> 
> This is not great naming - it's not "test name" in this case.

test_or_process_name might be a better variable name.

This change is not required to land the patch, but please consider changing it.

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:310
>> +                if not (any(process[0] == test for process in crashed_processes)):
> 
> Are the outer parentheses necessary?

Please remove outer parenthesis if not necessary.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:45
>> +(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, TIMEOUT, CRASH, CRASH_OTHER, SKIP, WONTFIX,
> 
> Don't we need to add support for this new result everywhere? Bot watcher's dashboard and flakiness dashboard come to mind first. Maybe EWS too.
> 
> But since these crashes are already in a separate section of the JSON, I don't see why we need a new result.

I'm not sure what Alexey's concern is here.  As long as this generates new results.html files without causing the Dashboards to fail in some way, the patch seems fine.

Please follow-up with Alexey before landing this.

[Aakash Jain]

Created attachment 263525 [details]
Proposed patch

Incorporated the review comments. Modified how full_results.json store other_crashes data(in a separate section). The crash name in the Other Crashes section will be always consistent as: processname-pid.

[Alexey Proskuryakov]

Comment on attachment 263525 [details]
Proposed patch

The patch looks good, but it breaks fast/harness/results.html test.

[Build Bot]

Comment on attachment 263525 [details]
Proposed patch

Attachment 263525 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/309355

New failing tests:
fast/harness/results.html

[Build Bot]

Created attachment 263530 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 263525 [details]
Proposed patch

Attachment 263525 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/309370

New failing tests:
fast/harness/results.html

[Build Bot]

Created attachment 263531 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Aakash Jain]

Created attachment 263542 [details]
Proposed patch

[Alexey Proskuryakov]

Comment on attachment 263542 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263542&action=review

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:46
> -(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, TIMEOUT, CRASH, SKIP, WONTFIX,
> - SLOW, REBASELINE, MISSING, FLAKY, NOW, NONE) = range(16)
> +(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, TIMEOUT, CRASH, CRASH_OTHER, SKIP, WONTFIX,
> + SLOW, REBASELINE, MISSING, FLAKY, NOW, NONE) = range(17)

Sorry, I missed this while glancing over the patch yesterday. I still don't understand why we need to add a new expectation, given that it's a super fragile thing.

[Aakash Jain]

Created attachment 263628 [details]
Updated patch

We needed a new expectation earlier, since for the newly found crashes, we were creating TestResult and adding to the list of TestResults and needed a way to differentiate between both type of crashes (so as to create separate sections in html page). However since we re-structed the json file and both type of crashes are stored in separate sections of json, we can do away with adding a new expectation. We just need some flag to differentiate test crashes from these new crashes (so as to add to correct section in json file). I have now used TestResult.is_other_crash flag for the same. Please review.

[Alexey Proskuryakov]

Comment on attachment 263628 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263628&action=review

> Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:172
> +            crash_dict['report'] = 'REGRESSION'
> +            crash_dict['expected'] = expected
> +            crash_dict['actual'] = "".join(actual)

Are these used anywhere? I think that claiming that other crashes are regressions is misleading, and also there aren't any actual or expected results, there are only crash logs.

[Aakash Jain]

Created attachment 263646 [details]
Updated patch

Removed actual, expected and report from crash_dict. I need to keep an (empty) dictionary so that rest of javascript code can easily parse it.

[WebKit Commit Bot]

Comment on attachment 263646 [details]
Updated patch

Clearing flags on attachment: 263646

Committed r191374: <http://trac.webkit.org/changeset/191374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.