Bug 83716 – Show flakiness dashboard data in garden-o-matic

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ojan Vafai]

Created attachment 136742 [details]
screenshot

I have a patch that inlines the flakiness dashboard data into garden-o-matic. Curious what you all think. I have found it very difficult to do rebaselines of expected failures without this data (e.g. flaky tests shouldn't be rebaselined).

Does this work? Is it too much information? Should it be behind a toggle of some kind?

[Adam Barth]

It's certainly more colorful and a lot more information than we had before.  I'm inclined to say we should try it and iterate as needed.

[Ojan Vafai]

Created attachment 136811 [details]
Patch

[Adam Barth]

Comment on attachment 136811 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/garden-o-matic.js:39
> -function update()
> +function update(opt_onUpdateComplete)

I would have just called opt_onUpdateComplete something like "callback", but I understand that this is a more advanced convention.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:50
> -    var testsParameter = testNameList.join(',');
> +    var testsParameter = opt_testNameList ? opt_testNameList.join(',') : '';
>      return 'http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=' + encodeURIComponent(testsParameter);

Should we really be adding the "tests=" parameter if opt_testNameList is falsy?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:87
> +                document.body.setAttribute('selected-tab-name', this._tabNames[ui.index]);

selected-tab-name => data-selected-tab-name

Also, can you make "data-selected-tab-name" a constant somewhere?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:212
> +        window.addEventListener('scroll', this._syncIframeToPlaceHolder.bind(this));

We really need a scroll listener?  That seems very unfortunate.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:220
> +        var intervalId = setInterval(function() {

Crazy.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:268
> +                var parsedData = JSON.parse(event.data);

Shouldn't we validate that this message actually came from the right origin?  Message events can come from anywhere.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:274
> +            if (!parsedData.height)
> +                console.log('parsedData did not have a height property: ' + event.data);

Why don't we just use a seamless iframe?  I guess it won't work anyway because this iframe is cross-origin.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:276
> +            var newHeight = (parsedData.height + 20) + 'px';

Why + 20 ?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:306
> +        // Update the position and size of the iframe for 5 seconds after loading to give
> +        // enough time for the contents to render and for jquery to finish the accordion animation.
> +        var deadline = Date.now() + 5000;

This is crazy!  5 seconds just seems like a random amount of time.

[Adam Barth]

This patch seems really crazy.  Is there a less nutty way to do this?

[Ojan Vafai]

Comment on attachment 136811 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/garden-o-matic.js:39
>> +function update(opt_onUpdateComplete)
> 
> I would have just called opt_onUpdateComplete something like "callback", but I understand that this is a more advanced convention.

I'm fine either way.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:50
>      return 'http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=' + encodeURIComponent(testsParameter);

The dashboard just ignores empty parameters. So it's fine.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:87
>> +                document.body.setAttribute('selected-tab-name', this._tabNames[ui.index]);
> 
> selected-tab-name => data-selected-tab-name
> 
> Also, can you make "data-selected-tab-name" a constant somewhere?

Much better.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:212
>> +        window.addEventListener('scroll', this._syncIframeToPlaceHolder.bind(this));
> 
> We really need a scroll listener?  That seems very unfortunate.

Why unfortunate?

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:268
>> +                var parsedData = JSON.parse(event.data);
> 
> Shouldn't we validate that this message actually came from the right origin?  Message events can come from anywhere.

Whoops. yes! :)

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:274
>> +                console.log('parsedData did not have a height property: ' + event.data);
> 
> Why don't we just use a seamless iframe?  I guess it won't work anyway because this iframe is cross-origin.

You answered your own question. This is a perfect example of why I want seamless+cors to work.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:276
>> +            var newHeight = (parsedData.height + 20) + 'px';
> 
> Why + 20 ?

Whoops. Forgot to fix this. Stupid web platform. As best I can tell, there's no way to grab the size of the iframe's content that includes the horizontal scrollbar. Will move this into a properly named variable and add a comment explaining.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:306
>> +        var deadline = Date.now() + 5000;
> 
> This is crazy!  5 seconds just seems like a random amount of time.

In practice, this polling is cheap (uses <1% of my cpu). So we can just pick a time that's long enough to work >99.9% of the time. So, I picked 5 seconds as that seemed like more than enough.

I listed in the FIXME above what we'd need to do in order to get rid of this. In practice, this turns out to be good enough I think. Maybe jQuery gives a way to determine when the animation is done, but I can't find it.

[Ojan Vafai]

+arv in case he has any great ideas.

(In reply to comment #4)
> This patch seems really crazy.  Is there a less nutty way to do this?

I agree it's a little crazy. :)

I did give it a good amount of thought and couldn't come up with anything better. I listed in the FIXME's the things that would need to change to make this not nutty.

If we just added a "keepalive" or something to the platform so that iframes don't reload when being reparented, then all this nuttieness and 90% of this code would go away. I think that makes sense for the platform regardless, so I'll post a suggestion to whatwg.

Until the platform changes though, the only other solution I can think of is to not use an iframe at all and instead pull the flakiness dashboard code into garden-o-matic directly. That would be a good deal of work. It's not an insane amount of work, but it's certainly more work than this patch.

[Adam Barth]

Why can't we just make the frame load faster?  Is everything it's currently doing necessary to draw the green and red boxes?

[Adam Barth]

Comment on attachment 136811 [details]
Patch

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

I'm marking this as R-.  I'm hoping we can find a way to add this feature that doesn't involve as much fighting the platform.

>>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:212
>>> +        window.addEventListener('scroll', this._syncIframeToPlaceHolder.bind(this));
>> 
>> We really need a scroll listener?  That seems very unfortunate.
> 
> Why unfortunate?

Scroll listeners make scrolling slow and janky.  They're also as sign that we're doing things wrong and fighting the platform rather than working with it.

>>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:306
>>> +        var deadline = Date.now() + 5000;
>> 
>> This is crazy!  5 seconds just seems like a random amount of time.
> 
> In practice, this polling is cheap (uses <1% of my cpu). So we can just pick a time that's long enough to work >99.9% of the time. So, I picked 5 seconds as that seemed like more than enough.
> 
> I listed in the FIXME above what we'd need to do in order to get rid of this. In practice, this turns out to be good enough I think. Maybe jQuery gives a way to determine when the animation is done, but I can't find it.

Yeah, you can get a callback when the animation is done.  If we can't get that working, I'd rather just remove the animation than having a 100ms poll loop.

[Erik Arvidsson]

Comment on attachment 136811 [details]
Patch

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

I don't really have any suggestions. This is a known issue with the web platform and the polling the content size is the only solution I know (unless you want to wrap every single DOM access inside your iframe).

>>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:268
>>> +                var parsedData = JSON.parse(event.data);
>> 
>> Shouldn't we validate that this message actually came from the right origin?  Message events can come from anywhere.
> 
> Whoops. yes! :)

Do we need to stringify/parse. Didn't we end up implementing posting structured data?

>>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:276
>>> +            var newHeight = (parsedData.height + 20) + 'px';
>> 
>> Why + 20 ?
> 
> Whoops. Forgot to fix this. Stupid web platform. As best I can tell, there's no way to grab the size of the iframe's content that includes the horizontal scrollbar. Will move this into a properly named variable and add a comment explaining.

You should be able to do this...

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js:366
> +        $(this).bind('accordionchangestart', function(event, ui) {

oh jquery

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results_unittests.js:45
> +    window.postMessage(JSON.stringify({height:15}), '*');

{height: 15}

[Ojan Vafai]

Created attachment 138730 [details]
Patch

[Ojan Vafai]

Ok. This is a much much smaller patch after all the refactoring. Should be an easy review. :)

[Erik Arvidsson]

Comment on attachment 138730 [details]
Patch

Looks good to me

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:56
> +}

I see that other statements are missing semicolon too.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results_unittests.js:28
> +module("ui.results");

inconsistent use of quotes

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results_unittests.js:31
> +    dashboard = new ui.results.FlakinessData();

missing var?

[Ojan Vafai]

Comment on attachment 138730 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:56
>> +}
> 
> I see that other statements are missing semicolon too.

Yeah, I'm matching the code around it, which seems like the right thing to do.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results_unittests.js:28
>> +module("ui.results");
> 
> inconsistent use of quotes

The whole codebase is inconsistent here. More code seems to use single-quotes though, so I'll switch this over.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results_unittests.js:31
>> +    dashboard = new ui.results.FlakinessData();
> 
> missing var?

lol, whoops. I changed that at one point for easier debugging and forgot to fix it. :)

[Ojan Vafai]

Created attachment 139033 [details]
Patch

[Ojan Vafai]

Committed r115337: <http://trac.webkit.org/changeset/115337>