Bug 64141 - Teach garden-o-matic how to display test results
Summary: Teach garden-o-matic how to display test results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 64188
  Show dependency treegraph
 
Reported: 2011-07-07 18:09 PDT by Adam Barth
Modified: 2011-07-08 12:26 PDT (History)
3 users (show)

See Also:


Attachments
Work in progress (9.93 KB, patch)
2011-07-07 18:10 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (17.07 KB, patch)
2011-07-07 18:48 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (18.28 KB, patch)
2011-07-07 19:26 PDT, Adam Barth
ojan: review+
ojan: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-07-07 18:09:40 PDT
Teach garden-o-matic how to display test results
Comment 1 Adam Barth 2011-07-07 18:10:23 PDT
Created attachment 100057 [details]
Work in progress
Comment 2 Adam Barth 2011-07-07 18:48:45 PDT
Created attachment 100062 [details]
Patch
Comment 3 Ojan Vafai 2011-07-07 19:10:50 PDT
Comment on attachment 100062 [details]
Patch

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

Yay for tests!

> Tools/ChangeLog:7
> +

Wouldn't hurt to give a bit more description here, e.g. as to how this all works. The code isn't especially hard for me to understand given it's ancestry, but the way it works at a high-level is maybe not immediately obvious.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:50
> +base.probe = function(url, options)

s/options/handlers? and maybe change success->onsuccess and error->onerror?

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:55
> +        if ($.isFunction(options.success))

checking that it's a function seems excessive to me. bulks up the code. I think it should be sufficient to just check "if (options.success)"

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:56
> +            options.success.call();

Can't you just call options.success()?

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:60
> +        if ($.isFunction(options.error))

ditto: if (options.error)

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:61
> +            options.error.call();

options.error()?

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/index.html:54
> +.results iframe {
> +  width: 400px;
> +  height: 300px;
> +}
> +.results img {
> +  width: 400px;
> +  height: 300px;
> +}

.results iframe,
.results img {
...
}

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:13
> +var kPossibleSuffixList = [

Because of reftests, there are -expected.html and -expected-mismatch.html and for audio tests you'll need -expected.wav and -actual.wav.

You might want to look at http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/results.html as a reference for some of this stuff.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:132
> +results.resultKind = function(url)
> +{
> +    if (/-actual\.[a-z]+$/.test(url))
> +        return 'actual';
> +    else if (/-expected\.[a-z]+$/.test(url))
> +        return 'expected';
> +    else if (/diff\.[a-z]+$/.test(url))
> +        return 'diff';
> +    return 'unknown';
> +}
> +
> +results.resultType = function(url)
> +{
> +    if (/\.png$/.test(url))
> +        return 'image';
> +    return 'text';

Would prefer for these strings to be constants.

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:135
> +function sortResultURLs(urls)

I'm confused by this name. This looks more like it filters than sorts.
Comment 4 Adam Barth 2011-07-07 19:16:09 PDT
Thanks for the review.

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:56
> > +            options.success.call();
> 
> Can't you just call options.success()?

We can, but then the |this| value will be bound to |options|, which leaks an implementation detail.

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:13
> > +var kPossibleSuffixList = [
> 
> Because of reftests, there are -expected.html and -expected-mismatch.html and for audio tests you'll need -expected.wav and -actual.wav.

I'm not going to worry about that stuff yet.  The truth is that there are only four reftests total in the whole project.  Basic stuff first.  Complicated things later.

> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:135
> > +function sortResultURLs(urls)
> 
> I'm confused by this name. This looks more like it filters than sorts.

It sorts.  I should add an ASSERT that we end up returning all the URLs we started with.
Comment 5 Adam Barth 2011-07-07 19:19:33 PDT
> > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:50
> > +base.probe = function(url, options)
> 
> s/options/handlers? and maybe change success->onsuccess and error->onerror?

I'm just matching jQuery style here.
Comment 6 Ojan Vafai 2011-07-07 19:21:12 PDT
(In reply to comment #4)
> Thanks for the review.
> 
> > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/base.js:56
> > > +            options.success.call();
> > 
> > Can't you just call options.success()?
> 
> We can, but then the |this| value will be bound to |options|, which leaks an implementation detail.

Fair enough.

> > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:13
> > > +var kPossibleSuffixList = [
> > 
> > Because of reftests, there are -expected.html and -expected-mismatch.html and for audio tests you'll need -expected.wav and -actual.wav.
> 
> I'm not going to worry about that stuff yet.  The truth is that there are only four reftests total in the whole project.  Basic stuff first.  Complicated things later.

I'm fine with that, but it'd be good add them to the FIXME. Now that nearly everyone is on NRWT, I think we can start writing reftests, so I expect that number to change quickly.

> > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:135
> > > +function sortResultURLs(urls)
> > 
> > I'm confused by this name. This looks more like it filters than sorts.
> 
> It sorts.  I should add an ASSERT that we end up returning all the URLs we started with.

Oh, I see. It sorts by suffix type? Could use a better name. I would expect sort to sort them lexicographically.
Comment 7 Adam Barth 2011-07-07 19:26:56 PDT
Created attachment 100066 [details]
Patch
Comment 8 Ojan Vafai 2011-07-08 09:47:26 PDT
Comment on attachment 100066 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/ui.js:30
> +        var fragment = type == 'image' ? '<img>' : '<iframe></iframe>';

s/'image'/kImageType/
Comment 9 Adam Barth 2011-07-08 12:26:37 PDT
Committed r90652: <http://trac.webkit.org/changeset/90652>