Bug 94255 – Add AV perf layout tests to webkit flakiness dashboard

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

[Shadi]

Add AV perf layout tests to webkit flakiness dashboard

[Shadi]

Created attachment 158911 [details]
Patch

[Ojan Vafai]

Comment on attachment 158911 [details]
Patch

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

As far as the naming issue we discussed offline, I see two options:
1. Rename the builders so they say AV in the name.
2. Add a new group "@DEPS AV - chromium.org".

I have a slight preference for option 1, but option 2 is totally fine if you'd prefer that.

My intuition is that showing a different name than the builder name would be far too complicated as that assumption is woven throughout the dashboard code. You're welcome to try making such a patch, but I doubt it'd be a good use of your time. :)

> Tools/TestResultServer/static-dashboards/builders.js:59
>  var LEGACY_BUILDER_MASTERS_TO_GROUPS = {

You need to update this as well. Actually, now that I look at it, ChromiumWin, ChromiumMac and ChromiumLinux are missing from here as well. Mind adding them while you're modifying this?

> Tools/TestResultServer/static-dashboards/builders.js:197
> +    return builder.indexOf('Builder') == -1

Won't this exclude "Win7 BuilderTester"? Also, this line needs a semicolon at the end.

> Tools/TestResultServer/static-dashboards/builders.js:252
> +            requestBuilderList(LAYOUT_TESTS_BUILDER_GROUPS, isChromiumTipOfTreeAVTestRunner, CHROMIUM_PERF_AV_BUILDER_MASTER, groupName, BuilderGroup.TOT_WEBKIT, builderGroup);

These bots currently seem to be syncing to Chromium DEPS. Is that intended? If so, then they should be added to the "@Deps - chromium.org" group.

[Shadi]

Created attachment 158925 [details]
Patch

[Shadi]

Comment on attachment 158911 [details]
Patch

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

>> Tools/TestResultServer/static-dashboards/builders.js:59
>>  var LEGACY_BUILDER_MASTERS_TO_GROUPS = {
> 
> You need to update this as well. Actually, now that I look at it, ChromiumWin, ChromiumMac and ChromiumLinux are missing from here as well. Mind adding them while you're modifying this?

Added them all. I assume they are all @DEPS.

>> Tools/TestResultServer/static-dashboards/builders.js:197
>> +    return builder.indexOf('Builder') == -1
> 
> Won't this exclude "Win7 BuilderTester"? Also, this line needs a semicolon at the end.

I submitted this assuming the new bot names "AV Linux" and "AV Win7".

>> Tools/TestResultServer/static-dashboards/builders.js:252
>> +            requestBuilderList(LAYOUT_TESTS_BUILDER_GROUPS, isChromiumTipOfTreeAVTestRunner, CHROMIUM_PERF_AV_BUILDER_MASTER, groupName, BuilderGroup.TOT_WEBKIT, builderGroup);
> 
> These bots currently seem to be syncing to Chromium DEPS. Is that intended? If so, then they should be added to the "@Deps - chromium.org" group.

I had a different understanding for what @ToT chromium was. These are definitely @Deps.

[Ojan Vafai]

Comment on attachment 158925 [details]
Patch

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

Looks good. Please address the last couple comments and post a new patch with cq? marking it as requesting the commit queue. Any committer can change that to cq+ once you've gotten an r+.

> Tools/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

This needs a bit more of a detailed description. Specifically, you could mention that you're also adding the missing mappings to LEGACY_BUILDER_MASTERS_TO_GROUPS and that the builder name filter doesn't work for the current builder names, but that they're getting renamed.

> Tools/TestResultServer/static-dashboards/builders.js:266
> +            requestBuilderList(LAYOUT_TESTS_BUILDER_GROUPS, isChromiumTipOfTreeAVTestRunner, CHROMIUM_PERF_AV_BUILDER_MASTER, groupName, BuilderGroup.TOT_WEBKIT, builderGroup);

Need to change the BuilderGroup to DEPS_WEBKIT. Now that I look at it, that argument is plumbed through a bunch of calls, but not actually used. :( I suppose it doesn't matter, but may as well change it for consistency sake.

[Shadi]

Created attachment 159150 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159150 [details]
Patch

Clearing flags on attachment: 159150

Committed r125915: <http://trac.webkit.org/changeset/125915>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.