Bug 64141 – Teach garden-o-matic how to display test results

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Teach garden-o-matic how to display test results

[Adam Barth]

Created attachment 100057 [details]
Work in progress

[Adam Barth]

Created attachment 100062 [details]
Patch

[Ojan Vafai]

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.

[Adam Barth]

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.

[Adam Barth]

> > 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.

[Ojan Vafai]

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

[Adam Barth]

Created attachment 100066 [details]
Patch

[Ojan Vafai]

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/

[Adam Barth]

Committed r90652: <http://trac.webkit.org/changeset/90652>