Bug 104941 - Dashboard cleanup: remove usage of global g_builders.
Summary: Dashboard cleanup: remove usage of global g_builders.
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: Julie Parent
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-13 12:55 PST by Julie Parent
Modified: 2012-12-13 18:03 PST (History)
4 users (show)

See Also:


Attachments
Patch (19.51 KB, patch)
2012-12-13 13:05 PST, Julie Parent
no flags Details | Formatted Diff | Diff
Patch (18.93 KB, patch)
2012-12-13 13:50 PST, Julie Parent
no flags Details | Formatted Diff | Diff
Patch (22.17 KB, patch)
2012-12-13 14:40 PST, Julie Parent
no flags Details | Formatted Diff | Diff
Patch for landing (22.17 KB, patch)
2012-12-13 14:41 PST, Julie Parent
no flags Details | Formatted Diff | Diff
Patch (22.16 KB, patch)
2012-12-13 14:49 PST, Julie Parent
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julie Parent 2012-12-13 12:55:14 PST
Dashboard cleanup: remove usage of global g_builders.
Comment 1 Julie Parent 2012-12-13 13:05:09 PST
Created attachment 179320 [details]
Patch
Comment 2 Julie Parent 2012-12-13 13:10:35 PST
Comment on attachment 179320 [details]
Patch

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

> Tools/TestResultServer/static-dashboards/loader_unittests.js:57
> +test('results files loading', 11, function() {

Total exceptions is 1 + 2 x numBuilders.  So before it was 1 + 2 * 2 = 5, now 1 + 2 * 5 = 11, since we use an expanded builder list.

> Tools/TestResultServer/static-dashboards/loader_unittests.js:72
> +        var builderName = /builder=([\w ().]+)&/.exec(url)[1];

Now that we use real builder names, they can contain more than just alphanumerics, aka, "WebKit Linux (dbg)" has '(' and ')', and "WebKit Mac10.7" has '.'
Comment 3 Dirk Pranke 2012-12-13 13:16:09 PST
Comment on attachment 179320 [details]
Patch

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

> Tools/ChangeLog:12
> +        they were even bigger offenders of bad hygine, relying on global state

typo: hygiene.

> Tools/TestResultServer/static-dashboards/dashboard_base.js:191
> +            function() { return value in currentBuilderGroup().builders; });

I would probably add a currentBuilders() function that returns currentBuilderGroup().builders and use that below.

>> Tools/TestResultServer/static-dashboards/loader_unittests.js:57
>> +test('results files loading', 11, function() {
> 
> Total exceptions is 1 + 2 x numBuilders.  So before it was 1 + 2 * 2 = 5, now 1 + 2 * 5 = 11, since we use an expanded builder list.

It might be useful to note this in a comment.
Comment 4 Dirk Pranke 2012-12-13 13:16:20 PST
(looks fine otherwise).
Comment 5 Dirk Pranke 2012-12-13 13:16:44 PST
R+ if you agree w/ those changes.
Comment 6 Julie Parent 2012-12-13 13:50:42 PST
Created attachment 179324 [details]
Patch
Comment 7 Julie Parent 2012-12-13 13:51:14 PST
Addressed all comments.
Comment 8 Julie Parent 2012-12-13 13:54:00 PST
Just saw that some files were excluded from my change, fixing ...
Comment 9 Dirk Pranke 2012-12-13 13:54:27 PST
Comment on attachment 179324 [details]
Patch

thanks!
Comment 10 Julie Parent 2012-12-13 14:40:05 PST
Created attachment 179340 [details]
Patch
Comment 11 Julie Parent 2012-12-13 14:41:29 PST
Created attachment 179342 [details]
Patch for landing
Comment 12 WebKit Review Bot 2012-12-13 14:43:31 PST
Comment on attachment 179342 [details]
Patch for landing

Rejecting attachment 179342 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

/mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15309423
Comment 13 Julie Parent 2012-12-13 14:49:18 PST
Created attachment 179345 [details]
Patch
Comment 14 WebKit Review Bot 2012-12-13 18:03:53 PST
Comment on attachment 179345 [details]
Patch

Clearing flags on attachment: 179345

Committed r137693: <http://trac.webkit.org/changeset/137693>
Comment 15 WebKit Review Bot 2012-12-13 18:03:57 PST
All reviewed patches have been landed.  Closing bug.