This will help the gardener determine the root cause of a test failure.
aroben: To fill you in on a discussion that dglazkov and I had this morning: 1) We think that being able to go back in time (like in TestFailures) and find the root cause of a test failure is important in order to figure out the best way to deal with a failure. 2) Having the tool hosted on a web site is also important so that the gardener can give links to folks. Both those thoughts push us more towards the TestFailures design point, which means we might want to integrate sooner rather than later. To access the local server (to preform backend actions), we're currently thinking about using XMLHttpRequest and CORS. If you have the local server running, you'll see the actions and if you don't have it running, you'll see the "read only" view.
(In reply to comment #1) > aroben: To fill you in on a discussion that dglazkov and I had this morning: > > 1) We think that being able to go back in time (like in TestFailures) and find the root cause of a test failure is important in order to figure out the best way to deal with a failure. > > 2) Having the tool hosted on a web site is also important so that the gardener can give links to folks. These both make sense to me. > Both those thoughts push us more towards the TestFailures design point, which means we might want to integrate sooner rather than later. OK. > To access the local server (to preform backend actions), we're currently thinking about using XMLHttpRequest and CORS. If you have the local server running, you'll see the actions and if you don't have it running, you'll see the "read only" view. So TestFailures would send an XHR to your local server? How would that work? I didn't think the local server was publicly-addressable. Maybe you meant the local server would XHR TestFailures, but then I don't know what "If you have the local server running, you'll see the actions" means.
> So TestFailures would send an XHR to your local server? Yes. > How would that work? Using CORS. > I didn't think the local server was publicly-addressable. It's not publicly addressable, but it is locally addressable (e.g., http://localhost:8570/blah/blah?efef If you view the hosted page, it can fire off an XHR to see if the local server is running on your computer, and if so, show the action buttons.
(In reply to comment #3) > > I didn't think the local server was publicly-addressable. > > It's not publicly addressable, but it is locally addressable (e.g., http://localhost:8570/blah/blah?efef > > If you view the hosted page, it can fire off an XHR to see if the local server is running on your computer, and if so, show the action buttons. Ah, of course! Client-side FTW!
Created attachment 100179 [details] Work in progres
Created attachment 100188 [details] Patch, now with tests
Comment on attachment 100188 [details] Patch, now with tests I didn't realize that this was what you wanted old full_results.json files for. Did you consider using results-small.json to find this information? It has all the data in a single request for all the failing tests. You can do everything in two requests. 1. Grab full_results.json to find the list of failing tests at tip of tree. 2. Grab (in parallel) results-small.json to find when each test started failing. Then you'll also have a lot more information and be better poised to add heuristics to determine if a test is flaky.
Yeah, I considered that. We might end up going that direction in the end. I didn't realize when I started the patch how much complexity the asynchronous results fetch would add. The main cost is becoming more reliant on test-results.appspot.com. In principles, we could just use buildbot directly, which would remove one moving piece. I don't have strong feelings either way. We might have to experiment now and optimize later.
Comment on attachment 100188 [details] Patch, now with tests View in context: https://bugs.webkit.org/attachment.cgi?id=100188&action=review I don't really feel like a very good reviewer for this patch. I think you'd do better with someone who spoke JS better than I do. > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:66 > + return kTestResultsServer + 'testfile' + > + '?builder=' + builderName + > + '&master=' + kMasterName + > + '&testtype=' + kTestType + > + '&name=' + name; It's sad that we don't have an easy way to build requests from a dictionary/object/map. I would expect such to exist in jquery. > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:76 > +function ResultsCache() { > + this.m_cache = {}; > +} style? > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:79 > +ResultsCache.prototype._fetch = function(key, callback) { > + var self = this; More style? At least your inconsistent with above.
> It's sad that we don't have an easy way to build requests from a dictionary/object/map. I would expect such to exist in jquery. It does exist in jquery. > style? Yeah, I screw up the { placement a lot. :(
(In reply to comment #10) > > style? > > Yeah, I screw up the { placement a lot. :( I think Eric was referring to using m_ prefixes as opposed to just _
Created attachment 100336 [details] Patch
Comment on attachment 100188 [details] Patch, now with tests View in context: https://bugs.webkit.org/attachment.cgi?id=100188&action=review >> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:66 >> + '&name=' + name; > > It's sad that we don't have an easy way to build requests from a dictionary/object/map. I would expect such to exist in jquery. Seems like you at least need to call encodeURIComponent. > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:72 > +function directoryOfResultsSummaryURL(builderName, name) > +{ > + return resultsSummaryURL(builderName, name) + '&dir=1'; > +} What is "name" naming here? Maybe it needs a better name. :-) >> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:79 >> + var self = this; > > More style? At least your inconsistent with above. I agree with Eric that it would be good to standardize on m_ vs _ > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:92 > + return callback(this.m_cache[key]); "return voidReturningFunction()" is not a pattern I've seen used much in WebKit, even in our JS code. > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:176 > + $.each(keyList, function(index, element) { > + self.m_keyList.push(element.key); > + }); You could use Array.prototype.map here. Or maybe the jQuery equivalent. > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:225 > + return true; > + } else { No need for "else" after "return". > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:239 > + var currentNode = resultsTree['tests']; Any particular reason why you're using ['tests'] instead of .tests? > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results_unittests.js:131 > + while (this.m_pendingCallbacks.length != 0) { No need for the != 0 here.
(In reply to comment #13) > (From update of attachment 100188 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100188&action=review > > >> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:66 > >> + '&name=' + name; > > > > It's sad that we don't have an easy way to build requests from a dictionary/object/map. I would expect such to exist in jquery. > > Seems like you at least need to call encodeURIComponent. This should be fixed now that I'm making jQuery do the hard work. > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:72 > > +function directoryOfResultsSummaryURL(builderName, name) > > +{ > > + return resultsSummaryURL(builderName, name) + '&dir=1'; > > +} > > What is "name" naming here? Maybe it needs a better name. :-) Fixed. > >> Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:79 > >> + var self = this; > > > > More style? At least your inconsistent with above. > > I agree with Eric that it would be good to standardize on m_ vs _ Ok. Done. > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:92 > > + return callback(this.m_cache[key]); > > "return voidReturningFunction()" is not a pattern I've seen used much in WebKit, even in our JS code. Fixed. > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:176 > > + $.each(keyList, function(index, element) { > > + self.m_keyList.push(element.key); > > + }); > > You could use Array.prototype.map here. Or maybe the jQuery equivalent. Done. > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:225 > > + return true; > > + } else { > > No need for "else" after "return". Done. > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:239 > > + var currentNode = resultsTree['tests']; > > Any particular reason why you're using ['tests'] instead of .tests? Mostly a hold-over from python where objects and dictionaries are different. In my mind resultsTree is a dictionary, not an object, but I'm not sure it matters. > > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results_unittests.js:131 > > + while (this.m_pendingCallbacks.length != 0) { > > No need for the != 0 here. Fixed.
Created attachment 100337 [details] Patch
Comment on attachment 100337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100337&action=review > Tools/Scripts/webkitpy/tool/servers/data/gardeningserver/results.js:203 > + ++this._indexOfNextKeyToFetch; Should use self instead of this.
Committed r90771: <http://trac.webkit.org/changeset/90771>