Bug 36474 - Add support for revision -> build lookup in buildbot.py and layout test result parsing
Summary: Add support for revision -> build lookup in buildbot.py and layout test resul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-22 18:26 PDT by Eric Seidel (no email)
Modified: 2010-03-23 15:24 PDT (History)
2 users (show)

See Also:


Attachments
Patch (11.64 KB, patch)
2010-03-22 18:26 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (17.37 KB, patch)
2010-03-23 01:19 PDT, Eric Seidel (no email)
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-03-22 18:26:28 PDT
Add support for revision -> build lookup in buildbot.py and layout test result parsing
Comment 1 Eric Seidel (no email) 2010-03-22 18:26:52 PDT
Created attachment 51376 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-03-23 01:19:41 PDT
Ojan: With this added code it is now trivial to convert existing results.html files from the buildbot into the results.json files which the layout test dashboard supports.
Comment 3 Eric Seidel (no email) 2010-03-23 01:19:59 PDT
Created attachment 51401 [details]
Patch
Comment 4 Adam Barth 2010-03-23 13:43:31 PDT
We need to abstract out the calls to urllib2.urlopen.  We keep using the same idiom to surround them.  We could include the 404 handling and caching as optional parameters.

+ _fetch_layout_test_results

I'd prefer if all control paths had explicit returns.

+ _parse_twisted_file_row

This twisted stuff should be in a separate file.  It's not specific to buildbot.

- _longest_builder_name

Why did we get rid of this function?  It seemed helpful to document what that complex line is doing.
Comment 5 Eric Seidel (no email) 2010-03-23 14:22:07 PDT
(In reply to comment #4)
> + _fetch_layout_test_results
> 
> I'd prefer if all control paths had explicit returns.

Ludite! :)  I think it's actually more readable to not add the "return None" since that's implied in python.

> + _parse_twisted_file_row
> 
> This twisted stuff should be in a separate file.  It's not specific to
> buildbot.

Agreed.  The functions started out as specific to this task, but I realized they could be made general.  i'm not sure where we'd put any other "twisted" stuff, but I named them so that they would be easy to recognize as general.

> - _longest_builder_name
> 
> Why did we get rid of this function?  It seemed helpful to document what that
> complex line is doing.

I had to copy the code.  I realized it was a realatively simple one-liner and didn't have any good place to put that funciton, so I just inlined it in both places.

I think I'm just gonna land as is, but I'm happy to make further changes upon further discussion with you.
Comment 6 Eric Seidel (no email) 2010-03-23 15:24:52 PDT
Committed r56421: <http://trac.webkit.org/changeset/56421>