Bug 64743 – update the flakiness dashboard to understand the new platforms/formats in test_expectations

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

update the flakiness dashboard to understand the new platforms/formats in test_expectations

[Ojan Vafai]

Created attachment 101186 [details]
Patch

[Adam Barth]

Comment on attachment 101186 [details]
Patch

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

Who's the right person to review this patch?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:538
> +            if (stringContains(currentBuildUppercase, '10.5'))

What about the future version 10.10.5 ?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard_tests.js:147
> +function testPlatformAndBuildType()

Yay tests.

[Ojan Vafai]

Comment on attachment 101186 [details]
Patch

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

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:538
>> +            if (stringContains(currentBuildUppercase, '10.5'))
> 
> What about the future version 10.10.5 ?

Should that time come we can deal with it. That's a hypothetical at least a few years out. Doesn't seem worth complicating the code over.

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 101186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101186&action=review
> 
> Who's the right person to review this patch?

Not sure. There's not many people who know this code. Mihai knows the dashboard code the best after me, but he doesn't know this specific code.

[Adam Barth]

Comment on attachment 101186 [details]
Patch

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.

[Adam Barth]

Here's a diagram of how fallback actually works:

https://docs.google.com/drawings/d/1z65SKkWrD4Slm6jobIphHwwRADyUtjOAxwGBVKBY8Kc/edit?hl=en_US

[Eric Seidel (no email)]

The Port classes know how fallback works. :)  We should ask them.

[Adam Barth]

> The Port classes know how fallback works. :)  We should ask them.

The flakiness dashboard doesn't have any way to talk to webkitpy.

[Ojan Vafai]

Comment on attachment 101186 [details]
Patch

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

Thanks for the review. I know it's hard to review non-trivial code that you don't know at all.

>> Tools/TestResultServer/static-dashboards/dashboard_base.js:917
>> +    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.

K. I can rename it.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:341
>> +    'LUCID': 'LINUX'
> 
> These are not correct.

TL;DR version: This map can probably go away, but it needs some code to be redone that is best left for a followup patch.

This is a bit confusing calling it fallbacks, but haven't come up with a better name. When we process the expectations lines for a test, we build up a tree like:
{
  WIN: {DEBUG: {modifiers: 'XP BUG123', expectations,: 'TIMEOUT'}},
  XP: {DEBUG: {...}},
  LINUX: {ALL: {...}},
  ALL: {DEBUG: {...}}
}

When we're looking up the expectations for a given builder, we grab the platform (e.g. XP) and build type (e.g. DEBUG) and look it up in this data structure, if it's not there, then we grab the fallback for that platform from the PLATFORM_FALLBACKS object.

So, for an XP bot, we'd find XP and return that. For a Leopard bot, we wouldn't find that, so we'd look for a MAC entry. We would find that, so we'd look for an ALL entry.

This is a data structure for representing the way lines in test_expectations.txt work. For example,
WIN DEBUG : foo/bar.html = FAIL
foo/bar.html = TIMEOUT

On Win/Debug, foo/bar.html is expected to FAIL, but on all other platforms it's expected to TIMEOUT.

I suppose a much simpler design would be to just have the data structure list every platform/buildType explicitly. It would use a ton more memory, but the code would be simpler. Memory is not really something we're trying to optimize here. :)

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard_tests.js:80
>> +function assertEquals(actual, expected, message)
> 
> You really should just use qunit.  We have it in the tree now and it's quite nice.

I agree, I'll add a FIXME do so in a followup patch.

[Adam Barth]

To clarify my understanding, you're saying we have yet another platform inheritance strategy that's different than the one used to store expected results in the tree?

[Ojan Vafai]

Created attachment 101204 [details]
Patch

[Adam Barth]

Comment on attachment 101204 [details]
Patch

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

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:871
> +                              if (buildTypeKey == ALL)

This variable doesn't appear to be effected by this loop.  Should it be hoisted out of the look we can skip all this looping if we don't need to do anything?

[Ojan Vafai]

> To clarify my understanding, you're saying we have yet another platform inheritance strategy that's different than the one used to store expected results in the tree?

Yes and no. It's not platforms in the LayoutTests sense. It's OS'es really.

-WIN is syntactic sugar for XP/VISTA/WIN7.
-MAC is syntactic sugar for LEOPARD/SNOWLEOPARD.
-LINUX is syntactic sugar for LUCID. (and presumably MAVERICK once we support that)
-If you don't list a platform at all, it's sugar for the union of all of the above.

The fact that this code uses a fallback strategy to implement that logic is excessively complicated in retrospect and should go away.

[Ojan Vafai]

Comment on attachment 101204 [details]
Patch

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

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:871
>> +                              if (buildTypeKey == ALL)
> 
> This variable doesn't appear to be effected by this loop.  Should it be hoisted out of the look we can skip all this looping if we don't need to do anything?

Good point. This function will go away entirely in my followup patch that kills the concept of PLATFORM_FALLBACKS, so I'd rather not spend time optimizing this.

[Adam Barth]

Comment on attachment 101204 [details]
Patch

Ok.  The only other changes in this patch appear to be renaming bound variables.

[Ojan Vafai]

Committed r91212: <http://trac.webkit.org/changeset/91212>