[webkit-reviews] review denied: [Bug 64743] update the flakiness dashboard to understand the new platforms/formats in test_expectations : [Attachment 101186] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 64743: update the flakiness dashboard to understand the new
platforms/formats in test_expectations
https://bugs.webkit.org/show_bug.cgi?id=64743

Attachment 101186: Patch
https://bugs.webkit.org/attachment.cgi?id=101186&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101186&action=review


This change looks ok, except the PLATFORM_FALLBACKS rules, which are wrong.

> Tools/TestResultServer/static-dashboards/dashboard_base.js:917
> -function testResultsByBuild(builderResults)
> +function resultsByBuild(builderResults)
>  {
>      var builderTestResults = builderResults[TESTS_KEY];
>      var buildCount = builderResults[FIXABLE_COUNTS_KEY].length;
> -    var testResultsByBuild = new Array(buildCount);
> +    var resultsByBuild = new Array(buildCount);

Generally its a bad idea to have a local variable that shadows the name of the
function it's in.  It's just confusing.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:341
>  var PLATFORM_FALLBACKS = {
> -'WIN': 'ALL',
> -'WIN-XP': 'WIN',
> -'WIN-VISTA': 'WIN',
> -'LINUX': 'ALL',
> -'MAC': 'ALL'
> +    'WIN': 'ALL',
> +    'XP': 'WIN',
> +    'VISTA': 'WIN',
> +    'MAC': 'ALL',
> +    'SNOWLEOPARD': 'MAC',
> +    'LEOPARD': 'MAC',
> +    'LINUX': 'ALL',
> +    'LUCID': 'LINUX'

These are not correct.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard_tests.js:80
> -function assertEquals(resultsForTest, actual, expected)
> +function assertEquals(actual, expected, message)

You really should just use qunit.  We have it in the tree now and it's quite
nice.