garden-o-matic should show test results

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

garden-o-matic should show test results

[Adam Barth]

Created attachment 100645 [details]
Patch

[Adam Roben (:aroben)]

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?

[Adam Barth]

(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!

[Adam Barth]

Committed r90923: <http://trac.webkit.org/changeset/90923>