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.
also see <rdar://problem/22239750>
Created attachment 262955 [details] Proposed patch
Unofficial r=me (I don't have reviewer status).
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.
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.
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.
Comment on attachment 263525 [details] Proposed patch The patch looks good, but it breaks fast/harness/results.html test.
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
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
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
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
Created attachment 263542 [details] Proposed patch
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.
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.
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.
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.
Comment on attachment 263646 [details] Updated patch Clearing flags on attachment: 263646 Committed r191374: <http://trac.webkit.org/changeset/191374>
All reviewed patches have been landed. Closing bug.