garden-o-matic should be able to determine which revisions caused a given failure

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

This will help the gardener determine the root cause of a test failure.

[Adam Barth]

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.

[Adam Roben (:aroben)]

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

[Adam Barth]

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

[Adam Roben (:aroben)]

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

[Adam Barth]

Created attachment 100179 [details]
Work in progres

[Adam Barth]

Created attachment 100188 [details]
Patch, now with tests

[Ojan Vafai]

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.

[Adam Barth]

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.

[Eric Seidel (no email)]

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.

[Adam Barth]

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

[Adam Roben (:aroben)]

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

[Adam Barth]

Created attachment 100336 [details]
Patch

[Adam Roben (:aroben)]

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.

[Adam Barth]

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

[Adam Barth]

Created attachment 100337 [details]
Patch

[Adam Roben (:aroben)]

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.

[Adam Barth]

Committed r90771: <http://trac.webkit.org/changeset/90771>