Bug 81848 – Show image diffs for gpu_tests on flakiness dashboard

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

[Dave Tu]

Show image diffs for gpu_tests on flakiness dashboard

[Dave Tu]

*** This bug has been marked as a duplicate of bug 81847 ***

[Dave Tu]

Reopening to attach new patch.

[Dave Tu]

Created attachment 133151 [details]
Patch

[Dave Tu]

*** Bug 81847 has been marked as a duplicate of this bug. ***

[Dave Tu]

Created attachment 133153 [details]
Patch

[Ojan Vafai]

Comment on attachment 133153 [details]
Patch

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

Mostly just a bunch of nits...

> ChangeLog:10
> +
> +        * Tools/TestResultServer/static-dashboards/builders.js:
> +        * Tools/TestResultServer/static-dashboards/dashboard_base.js:
> +        * Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:

This needs more description. The bug title only covers part of what this patch does.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1843
> +        } else {
> +            html += ' | <b>Only shows actual results/diffs from the most recent *failure* on each bot.</b>';
> +        }

WebKit style is to not put the brackets around single line if/else.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2140
> +    var failure = indexesForFailures(builder, test)[0];

s/failure/failureIndex

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2158
> +    var failures = indexesForFailures(builder, test);

s/failures/failureIndexes. The old name of failures should have been buildNumbers. :)

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:-2156
> -    dummyNode.onerror = function() {
> -        container.parentNode.removeChild(childContainer);

Why did you remove this?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2184
> +        if (opt_width > 0)
> +            item.style.width = opt_width + 'px';

I'd rather you set a classname and set these widths in CSS. I'm not sure why we explicitly set the height on line 2182. Looks like it's configurable.

So, you could make itemClassName the third argument to appendNonWebKitResults and not optional.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2187
> +            var childContainer = document.createElement('span');

It would be better to make this a div with display:inline-block.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2189
> +            var title = document.createElement('b');

Make this a div.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2190
> +            title.innerText = opt_title;

Use textContent instead. innerText does weird things.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2192
> +            childContainer.appendChild(document.createElement('br'));

Don't need this anymore now that title is a div.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2196
> +            container.insertBefore(childContainer, dummyNode);
> +        } else {
> +            container.insertBefore(item, dummyNode);

How about calling replaceChild instead of insertBefore?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2197
> +        }

Single-line else doesn't have brackets in webkit style.

[Dave Tu]

Created attachment 133956 [details]
Patch

[Dave Tu]

Comment on attachment 133153 [details]
Patch

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

>> ChangeLog:10
>> +        * Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:
> 
> This needs more description. The bug title only covers part of what this patch does.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1843
>> +        }
> 
> WebKit style is to not put the brackets around single line if/else.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2140
>> +    var failure = indexesForFailures(builder, test)[0];
> 
> s/failure/failureIndex

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2158
>> +    var failures = indexesForFailures(builder, test);
> 
> s/failures/failureIndexes. The old name of failures should have been buildNumbers. :)

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:-2156
>> -        container.parentNode.removeChild(childContainer);
> 
> Why did you remove this?

childContainer is not defined in this context, this doesn't do anything besides throw an error. Looks like it should be container.removeChild(dummyNode);

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2184
>> +            item.style.width = opt_width + 'px';
> 
> I'd rather you set a classname and set these widths in CSS. I'm not sure why we explicitly set the height on line 2182. Looks like it's configurable.
> 
> So, you could make itemClassName the third argument to appendNonWebKitResults and not optional.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2187
>> +            var childContainer = document.createElement('span');
> 
> It would be better to make this a div with display:inline-block.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2189
>> +            var title = document.createElement('b');
> 
> Make this a div.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2190
>> +            title.innerText = opt_title;
> 
> Use textContent instead. innerText does weird things.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2192
>> +            childContainer.appendChild(document.createElement('br'));
> 
> Don't need this anymore now that title is a div.

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2196
>> +            container.insertBefore(item, dummyNode);
> 
> How about calling replaceChild instead of insertBefore?

Done.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2197
>> +        }
> 
> Single-line else doesn't have brackets in webkit style.

Done.

[Ojan Vafai]

Comment on attachment 133956 [details]
Patch

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

Please fix the nits and upload a new patch for the commit queue.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1846
> +        } else {
> +            html += ' | <b>Only shows actual results/diffs from the most recent *failure* on each bot.</b>';
> +        }

Single-line  else shouldn't have braces.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:2197
> +        } else {
> +            container.replaceChild(item, dummyNode);
> +        }

Single-line  else shouldn't have braces.

[Dave Tu]

Created attachment 134108 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134108 [details]
Patch

Clearing flags on attachment: 134108

Committed r112303: <http://trac.webkit.org/changeset/112303>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.