Make a builder group support+expect multiple loads.
Created attachment 145634 [details] Patch
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.
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.
Created attachment 145648 [details] Patch
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.
Comment on attachment 145648 [details] Patch Clearing flags on attachment: 145648 Committed r119463: <http://trac.webkit.org/changeset/119463>
All reviewed patches have been landed. Closing bug.