Bug 64743 - update the flakiness dashboard to understand the new platforms/formats in test_expectations
Summary: update the flakiness dashboard to understand the new platforms/formats in tes...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-18 12:16 PDT by Ojan Vafai
Modified: 2011-07-18 14:51 PDT (History)
5 users (show)

See Also:


Attachments
Patch (18.10 KB, patch)
2011-07-18 12:22 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (19.33 KB, patch)
2011-07-18 14:25 PDT, Ojan Vafai
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-07-18 12:16:43 PDT
update the flakiness dashboard to understand the new platforms/formats in test_expectations
Comment 1 Ojan Vafai 2011-07-18 12:22:28 PDT
Created attachment 101186 [details]
Patch
Comment 2 Adam Barth 2011-07-18 13:07:25 PDT
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.
Comment 3 Ojan Vafai 2011-07-18 13:23:15 PDT
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.
Comment 4 Ojan Vafai 2011-07-18 13:25:10 PDT
(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.
Comment 5 Adam Barth 2011-07-18 13:39:03 PDT
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.
Comment 6 Adam Barth 2011-07-18 13:39:43 PDT
Here's a diagram of how fallback actually works:

https://docs.google.com/drawings/d/1z65SKkWrD4Slm6jobIphHwwRADyUtjOAxwGBVKBY8Kc/edit?hl=en_US
Comment 7 Eric Seidel (no email) 2011-07-18 13:53:00 PDT
The Port classes know how fallback works. :)  We should ask them.
Comment 8 Adam Barth 2011-07-18 14:03:57 PDT
> The Port classes know how fallback works. :)  We should ask them.

The flakiness dashboard doesn't have any way to talk to webkitpy.
Comment 9 Ojan Vafai 2011-07-18 14:15:04 PDT
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.
Comment 10 Adam Barth 2011-07-18 14:24:47 PDT
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?
Comment 11 Ojan Vafai 2011-07-18 14:25:09 PDT
Created attachment 101204 [details]
Patch
Comment 12 Adam Barth 2011-07-18 14:27:53 PDT
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?
Comment 13 Ojan Vafai 2011-07-18 14:29:28 PDT
> 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.
Comment 14 Ojan Vafai 2011-07-18 14:31:07 PDT
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.
Comment 15 Adam Barth 2011-07-18 14:33:11 PDT
Comment on attachment 101204 [details]
Patch

Ok.  The only other changes in this patch appear to be renaming bound variables.
Comment 16 Ojan Vafai 2011-07-18 14:51:28 PDT
Committed r91212: <http://trac.webkit.org/changeset/91212>