Bug 64440

Summary: garden-o-matic should show test results
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, dglazkov, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 64188    
Attachments:
Description Flags
Patch aroben: review+, abarth: commit-queue?

Description Adam Barth 2011-07-13 02:14:28 PDT
garden-o-matic should show test results
Comment 1 Adam Barth 2011-07-13 02:16:59 PDT
Created attachment 100645 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-07-13 05:19:31 PDT
Comment on attachment 100645 [details]
Patch

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

> Tools/ChangeLog:14
> +        zoom in on elements of the details pain, and images likely aren't sized

"pain"? ;-)

> Tools/ChangeLog:25
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/config.js:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/index.html:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/main.css:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/main.js:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/results_unittests.js:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/run-unittests.html:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/ui.js:
> +        * Scripts/webkitpy/tool/servers/data/gardeningserver/ui_unittests.js:

It would be nice to have some lower-level explanations of what's going on in your patch.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/main.js:75
> +        var children = $(content).children();

I'm not that familiar with jQuery. But is the $() around content really needed here? You already got content from a jQuery function, so I'd assume it's already been properly jQuery-ized.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/main.js:81
> +                $(content).children().hide().fadeIn('fast', dismissButterbar);

Same question here.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:20
> -    '-expected.txt',
> -    '-actual.txt',
> +    // '-expected.txt',
> +    // '-actual.txt',

Why did you make this change?
Comment 3 Adam Barth 2011-07-13 09:13:25 PDT
(In reply to comment #2)
> (From update of attachment 100645 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100645&action=review
> 
> > Tools/ChangeLog:25
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/config.js:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/index.html:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/main.css:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/main.js:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/results_unittests.js:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/run-unittests.html:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/ui.js:
> > +        * Scripts/webkitpy/tool/servers/data/gardeningserver/ui_unittests.js:
> 
> It would be nice to have some lower-level explanations of what's going on in your patch.

Yeah, my ChangeLogs have been kind of sparse.

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/main.js:75
> > +        var children = $(content).children();
> 
> I'm not that familiar with jQuery. But is the $() around content really needed here? You already got content from a jQuery function, so I'd assume it's already been properly jQuery-ized.

It's not needed, but it's not harmful either.  I'll remove it.

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:20
> > -    '-expected.txt',
> > -    '-actual.txt',
> > +    // '-expected.txt',
> > +    // '-actual.txt',
> 
> Why did you make this change?

The problem is with IMAGE+TEXT failures.  There's too much to show.  My thought is that having the actual and expected images is more useful than having the actual and expected text.  These are all very reversible decisions.  I suspect we'll want to change this to one of the prettier diffs as well.

Thanks for the review!
Comment 4 Adam Barth 2011-07-13 09:57:19 PDT
Committed r90923: <http://trac.webkit.org/changeset/90923>