Summary: | Teach garden-o-matic how to display test results | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, eric, ojan | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 64188 | ||||||||||
Attachments: |
|
Description
Adam Barth
2011-07-07 18:09:40 PDT
Created attachment 100057 [details]
Work in progress
Created attachment 100062 [details]
Patch
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. 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. > > 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.
(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. Created attachment 100066 [details]
Patch
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/ Committed r90652: <http://trac.webkit.org/changeset/90652> |