[Webkit-unassigned] [Bug 88260] Make a builder group support+expect multiple loads.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 4 16:10:34 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=88260
--- Comment #3 from Chase Phillips <cmp at chromium.org> 2012-06-04 16:10:34 PST ---
(From update of attachment 145634)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list