Bug 55907

Summary: new-run-webkit-tests should upload crash logs
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: Tools / TestsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, eric, lforschler, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 63468    
Bug Blocks: 34984    
Attachments:
Description Flags
Patch eric: review+

Dirk Pranke
Reported 2011-03-07 14:45:24 PST
old-run-webkit-tests now uploads crash logs; we need to modify new-run-webkit-tests to do the same. Quoting from an email to aroben@: On Mar 2, 2011, at 3:44 PM, Dirk Pranke wrote: > What did you need to change to make this happen? What do I need to add > to new-run-webkit-tests? There (thankfully) isn't very much code in old-run-webkit-tests to support this. The functions captureSavedCrashLogs and setUpWindowsCrashLogSaving and their callers/callees, plus the END block, are all that is required.
Attachments
Patch (6.41 KB, patch)
2011-06-27 14:59 PDT, Adam Barth 		eric: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2011-03-07 15:48:46 PST
If we wrote the code out nicely in webkitpy, we could easily expose commands (as part of webkit-patch or a separate tool) which ORWT could call, so that we could share a single python implementation instead of having two copies. :)
Adam Barth
Comment 2 2011-06-27 14:59:01 PDT
Created attachment 98793 [details] Patch
Dirk Pranke
Comment 3 2011-06-27 15:07:46 PDT
Comment on attachment 98793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98793&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:181 > + fs.write_text_file(filename, log if log else error) I just looked at the crashlogs.py module for the first time; does this actually work if you are running multiple DRTs at once? How do you know which pid generated the log? It looks like there is a timestamp and maybe some other number in the crash log filename; is that naming convention configurable, or is there some other way to get a pid into the filename?
Eric Seidel (no email)
Comment 4 2011-06-27 15:15:25 PDT
The pid is in the file. We could parse it out if necessary. I'm not sure we need to associate pid->crashlog yet though.
Dirk Pranke
Comment 5 2011-06-27 15:18:31 PDT
(In reply to comment #4) > The pid is in the file. We could parse it out if necessary. I'm not sure we need to associate pid->crashlog yet though. Well, since there are multiple workers running in parallel, the risk is that you'll end up associating stacks with the wrong tests. I've certainly seen cases where we've had lots (or all) tests crashing at once, but maybe the break is so obvious that you only really need to see one stack to fix the problem? If the pid is in the crash file, it doesn't seem like it would be that much harder to pull it out ...
Eric Seidel (no email)
Comment 6 2011-06-27 15:21:56 PDT
Comment on attachment 98793 [details] Patch LGTM. I suspect we'll iterate from here.
Adam Barth
Comment 7 2011-06-27 15:23:18 PDT
There's a slight problem in that ReportCrash takes a very long time to run and the master gets bored and kills it. :(
Adam Barth
Comment 8 2011-06-27 16:05:42 PDT
Committed r89873: <http://trac.webkit.org/changeset/89873>
Adam Barth
Comment 9 2011-06-27 16:27:00 PDT
Comment on attachment 98793 [details] Patch I'm going to leave this bug closed, but there's going to be some follow up work because ReportCrash is super, duper slow and tricks us into think the test timed out.
Adam Roben (:aroben)
Comment 10 2011-06-28 08:19:53 PDT
Have other bugs been filed to cover the remaining work? (Waiting appropriately for ReportCrash, dealing with multiple concurrent crashes, making this work on Windows…) It seems wrong to close this bug if not.
Note You need to log in before you can comment on or make changes to this bug.