Bug 88260 – Make a builder group support+expect multiple loads.

• Component: New Bugs
• Status: RESOLVED FIXED

[Chase Phillips]

Make a builder group support+expect multiple loads.

[Chase Phillips]

Created attachment 145634 [details]
Patch

[Ojan Vafai]

Comment on attachment 145634 [details]
Patch

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

Thanks for following up on this. I completely forgot about it. :( This looks good. Just a few minor things to cleanup. FYI, when you next upload it, if you use --request-commit in the webkit-patch command it will set cq? indicating that you'd like the reviewer to put the patch in the commit queue once they approve it.

> Tools/ChangeLog:7
> +

This could use a little bit more description (a couple short sentences) about the changes being made, e.g. something like:
Each BuilderGroup now keeps track of how many sub-groups it still expects to load. We only consider the builder group loaded once all the sub-groups have loaded for failed to load.

> Tools/TestResultServer/static-dashboards/builders.js:51
> +CHROMIUM_LINUX_BUILDER_MASTER = new BuilderMaster('ChromiumMac', 'http://build.chromium.org/p/chromium.linux/');

s/ChromiumMac/ChromiumLinux

> Tools/TestResultServer/static-dashboards/builders.js:74
> +    this.groups = 0;
> +    this.expectedGroups = expectedGroups;

Can we kill the expectedGroups argument? Instead, in requestBuilderList you increment expectedGroups. Then the rest of the code just works without needing to hard-code the number of expected groups.

> Tools/TestResultServer/static-dashboards/builders.js:75
> +    this.append(builders);

Do we ever pass in a non-empty builders to the constructor? Can we just remove the argument entirely? Then we also wouldn't need the length check at the beginning of append.

> Tools/TestResultServer/static-dashboards/builders.js:81
> +    if (flags & BuilderGroup.DEFAULT_BUILDER)
> +        this.defaultBuilder = builder;

While you're modifying this code, would you mind putting a FIXME here to remove this? AFAIK, we don't actually use DEFAULT_BUILDER in any meaningful way anymore (we always just default to the first builder in alphabetical order).

> Tools/TestResultServer/static-dashboards/builders.js:100
> +    if (this.groups >= this.expectedGroups)
> +        return true;
> +    return false;

Nit: I'd just merge this all into one line:
    return this.groups >= this.expectedGroups;

> Tools/TestResultServer/static-dashboards/builders.js:127
> +      builderGroups[groupName] = builderGroup;

Needs 4 space indent.

> Tools/TestResultServer/static-dashboards/builders.js:210
> +      g_handleBuildersListLoaded();

Needs 4 space indent.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:648
> +        '@ToT - dummy.org': new BuilderGroup(BuilderGroup.TOT_WEBKIT, [], 3),

This will obviously need to change given my request to remove expectedGroups from the constructor, but I'm OK with the tests just setting builderGroup.expectedGroups = 3. Somehow the hard-coding bothers me less in a test. :)

> Tools/TestResultServer/static-dashboards/run-unittests.html:57
> +LAYOUT_TESTS_BUILDER_GROUPS[groupName] = new BuilderGroup(BuilderGroup.TOT_WEBKIT, [], 4);

Ditto here. This can set LAYOUT_TESTS_BUILDER_GROUPS[groupName].expectedGroups = 4.

[Chase Phillips]

Comment on attachment 145634 [details]
Patch

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

>> Tools/ChangeLog:7
>> +
> 
> This could use a little bit more description (a couple short sentences) about the changes being made, e.g. something like:
> Each BuilderGroup now keeps track of how many sub-groups it still expects to load. We only consider the builder group loaded once all the sub-groups have loaded for failed to load.

Done.

>> Tools/TestResultServer/static-dashboards/builders.js:51
>> +CHROMIUM_LINUX_BUILDER_MASTER = new BuilderMaster('ChromiumMac', 'http://build.chromium.org/p/chromium.linux/');
> 
> s/ChromiumMac/ChromiumLinux

Done.

>> Tools/TestResultServer/static-dashboards/builders.js:74
>> +    this.expectedGroups = expectedGroups;
> 
> Can we kill the expectedGroups argument? Instead, in requestBuilderList you increment expectedGroups. Then the rest of the code just works without needing to hard-code the number of expected groups.

Done.

>> Tools/TestResultServer/static-dashboards/builders.js:75
>> +    this.append(builders);
> 
> Do we ever pass in a non-empty builders to the constructor? Can we just remove the argument entirely? Then we also wouldn't need the length check at the beginning of append.

I didn't find any cases where builders was non-empty, so I removed it and then updated all of the places where an empty list is passed in so the argument is removed.  I removed the length check at the beginning of append.

>> Tools/TestResultServer/static-dashboards/builders.js:81
>> +        this.defaultBuilder = builder;
> 
> While you're modifying this code, would you mind putting a FIXME here to remove this? AFAIK, we don't actually use DEFAULT_BUILDER in any meaningful way anymore (we always just default to the first builder in alphabetical order).

Done.

>> Tools/TestResultServer/static-dashboards/builders.js:100
>> +    return false;
> 
> Nit: I'd just merge this all into one line:
>     return this.groups >= this.expectedGroups;

Done.

>> Tools/TestResultServer/static-dashboards/builders.js:127
>> +      builderGroups[groupName] = builderGroup;
> 
> Needs 4 space indent.

Done.

>> Tools/TestResultServer/static-dashboards/builders.js:210
>> +      g_handleBuildersListLoaded();
> 
> Needs 4 space indent.

Done.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:634
> +        '@DEPS - dummy.org': new BuilderGroup(BuilderGroup.DEPS_WEBKIT, [], 1),

I also changed these lines and set .expectedGroups directly after line 635.

>> Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:648
>> +        '@ToT - dummy.org': new BuilderGroup(BuilderGroup.TOT_WEBKIT, [], 3),
> 
> This will obviously need to change given my request to remove expectedGroups from the constructor, but I'm OK with the tests just setting builderGroup.expectedGroups = 3. Somehow the hard-coding bothers me less in a test. :)

Done.

>> Tools/TestResultServer/static-dashboards/run-unittests.html:57
>> +LAYOUT_TESTS_BUILDER_GROUPS[groupName] = new BuilderGroup(BuilderGroup.TOT_WEBKIT, [], 4);
> 
> Ditto here. This can set LAYOUT_TESTS_BUILDER_GROUPS[groupName].expectedGroups = 4.

Done.

[Chase Phillips]

Created attachment 145648 [details]
Patch

[Ojan Vafai]

Comment on attachment 145648 [details]
Patch

Looks great. Thanks for fixing this! I assume you want this in the commit queue?

Once this lands, you can push it following these instructions: https://sites.google.com/a/chromium.org/dev/developers/testing/changedashboard. I don't mind pushing it if you'd rather not take the time.

[WebKit Review Bot]

Comment on attachment 145648 [details]
Patch

Clearing flags on attachment: 145648

Committed r119463: <http://trac.webkit.org/changeset/119463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.