Dashboard cleanup: remove usage of global g_builders.
authorjparent@chromium.org <jparent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Dec 2012 02:02:51 +0000 (02:02 +0000)
committerjparent@chromium.org <jparent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Dec 2012 02:02:51 +0000 (02:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104941

Reviewed by Dirk Pranke.

The dashboards use a lot of global state, which makes hacking on them
complicated. This change removes the use of one such global: g_builders.
In most cases, we can just use currentBuilderGroup().builders instead,
which is now currentBuilders().
Surprisingly, the most changes were required to the unit tests, since
they were even bigger offenders of bad hygiene, relying on global state
set by other tests, randomly clobbering global variables in ways the
real code doesn't, etc.

* TestResultServer/static-dashboards/builders.js:
(BuilderGroup.prototype.setup):
* TestResultServer/static-dashboards/dashboard_base.js:
(.switch.return):
(htmlForTestTypeSwitcher):
* TestResultServer/static-dashboards/flakiness_dashboard.js:
(generatePage):
(getAllTestsTrie):
(processTestRunsForAllBuilders):
(showPopupForBuild):
(generatePageForExpectationsUpdate):
(loadExpectationsLayoutTests):
* TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:
(resetGlobals):
(stubResultsByBuilder):
(test):
* TestResultServer/static-dashboards/loader.js:
(.):
* TestResultServer/static-dashboards/loader_unittests.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@137693 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/TestResultServer/static-dashboards/aggregate_results.html
Tools/TestResultServer/static-dashboards/builders.js
Tools/TestResultServer/static-dashboards/dashboard_base.js
Tools/TestResultServer/static-dashboards/flakiness_dashboard.js
Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js
Tools/TestResultServer/static-dashboards/loader.js
Tools/TestResultServer/static-dashboards/loader_unittests.js
Tools/TestResultServer/static-dashboards/timeline_explorer.html
Tools/TestResultServer/static-dashboards/treemap.html

index 70c7e1d948944e63384d0d5f8b8fe584b8c5bba3..60c11392bc9fdd0fcf4551d548d03ecf25223a9c 100644 (file)
@@ -1,3 +1,39 @@
+2012-12-13  Julie Parent  <jparent@chromium.org>
+
+        Dashboard cleanup: remove usage of global g_builders.
+        https://bugs.webkit.org/show_bug.cgi?id=104941
+
+        Reviewed by Dirk Pranke.
+
+        The dashboards use a lot of global state, which makes hacking on them
+        complicated. This change removes the use of one such global: g_builders.
+        In most cases, we can just use currentBuilderGroup().builders instead,
+        which is now currentBuilders().
+        Surprisingly, the most changes were required to the unit tests, since
+        they were even bigger offenders of bad hygiene, relying on global state
+        set by other tests, randomly clobbering global variables in ways the
+        real code doesn't, etc.
+
+        * TestResultServer/static-dashboards/builders.js:
+        (BuilderGroup.prototype.setup):
+        * TestResultServer/static-dashboards/dashboard_base.js:
+        (.switch.return):
+        (htmlForTestTypeSwitcher):
+        * TestResultServer/static-dashboards/flakiness_dashboard.js:
+        (generatePage):
+        (getAllTestsTrie):
+        (processTestRunsForAllBuilders):
+        (showPopupForBuild):
+        (generatePageForExpectationsUpdate):
+        (loadExpectationsLayoutTests):
+        * TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:
+        (resetGlobals):
+        (stubResultsByBuilder):
+        (test):
+        * TestResultServer/static-dashboards/loader.js:
+        (.):
+        * TestResultServer/static-dashboards/loader_unittests.js:
+
 2012-12-13  Eric Seidel  <eric@webkit.org>
 
         Callers should not have to stringify args before calling Executive run_command/popen
index fa033a54d4fdab2eb563835ff436414f791ba418..a3805a5472806c2e9c02a48f69d5188aaf6047d8 100644 (file)
@@ -68,7 +68,7 @@ img {
 function generatePage()
 {
     var html = htmlForTestTypeSwitcher(true) + '<br>';
-    for (var builder in g_builders)
+    for (var builder in currentBuilders())
         html += htmlForBuilder(builder);
     document.body.innerHTML = html;
 }
index 97dfe27297a25a0dd2ceb8444bd5f5a96344f969..32731ea168b5b3d224a02b7bfcc6576878869ac7 100644 (file)
@@ -127,7 +127,6 @@ BuilderGroup.prototype.setup = function()
     // FIXME: instead of copying these to globals, it would be better if
     // the rest of the code read things from the BuilderGroup instance directly
     g_defaultBuilderName = this._defaultBuilder();
-    g_builders = this.builders;
 };
 
 BuilderGroup.prototype._defaultBuilder = function()
index 6eee1e7bdee5381d21908d72d7132d45158187b5..e51768a8a26225dd58c18a3b235b27222cf71a78 100644 (file)
@@ -188,7 +188,7 @@ function handleValidHashParameterWrapper(key, value)
     // FIXME: This should probably be stored on g_crossDashboardState like everything else in this function.
     case 'builder':
         validateParameter(g_currentState, key, value,
-            function() { return value in g_builders; });
+            function() { return value in currentBuilders(); });
         return true;
 
     case 'useTestData':
@@ -416,12 +416,17 @@ function currentBuilderGroup()
     return currentBuilderGroupCategory()[g_crossDashboardState.group]
 }
 
+function currentBuilders()
+{
+    return currentBuilderGroup().builders;
+}
+
 function isTipOfTreeWebKitBuilder()
 {
     return currentBuilderGroup().isToTWebKit;
 }
 
-var g_defaultBuilderName, g_builders;
+var g_defaultBuilderName;
 function initBuilders()
 {
     currentBuilderGroup().setup();
@@ -694,7 +699,7 @@ function htmlForTestTypeSwitcher(opt_noBuilderMenu, opt_extraHtml, opt_includeNo
     html += selectHTML('Test type', 'testType', TEST_TYPES);
 
     if (!opt_noBuilderMenu) {
-        var buildersForMenu = Object.keys(g_builders);
+        var buildersForMenu = Object.keys(currentBuilders());
         if (opt_includeNoneBuilder)
             buildersForMenu.unshift('--------------');
         html += selectHTML('Builder', 'builder', buildersForMenu);
index d44e3e58e64e2a930dee51868e5f9376adc9fe1b..c37711b3cd6698d73f874c069fd37d5a1966f692 100644 (file)
@@ -155,7 +155,7 @@ function generatePage()
     else
         generatePageForBuilder(g_currentState.builder);
 
-    for (var builder in g_builders)
+    for (var builder in currentBuilders())
         processTestResultsForBuilderAsync(builder);
 
     postHeightChangedMessage();
@@ -186,7 +186,7 @@ function handleValidHashParameter(key, value)
     case 'builder':
         validateParameter(g_currentState, key, value,
             function() {
-                return value in g_builders;
+                return value in currentBuilders();
             });
         return true;
 
@@ -479,7 +479,7 @@ var g_allTestsTrie;
 function getAllTestsTrie()
 {
     if (!g_allTestsTrie)
-        g_allTestsTrie = new TestTrie(g_builders, g_resultsByBuilder);
+        g_allTestsTrie = new TestTrie(currentBuilders(), g_resultsByBuilder);
 
     return g_allTestsTrie;
 }
@@ -610,7 +610,7 @@ function allTestsWithSamePlatformAndBuildType(platform, buildType)
 {
     if (!g_allTestsByPlatformAndBuildType[platform][buildType]) {
         var tests = {};
-        for (var thisBuilder in g_builders) {
+        for (var thisBuilder in currentBuilders()) {
             var thisBuilderBuildInfo = platformAndBuildType(thisBuilder);
             if (thisBuilderBuildInfo.buildType == buildType && thisBuilderBuildInfo.platform == platform) {
                 addTestsForBuilder(thisBuilder, tests);
@@ -919,7 +919,7 @@ function processTestResultsForBuilderAsync(builder)
 
 function processTestRunsForAllBuilders()
 {
-    for (var builder in g_builders)
+    for (var builder in currentBuilders())
         processTestRunsForBuilder(builder);
 }
 
@@ -1236,7 +1236,7 @@ function showPopupForBuild(e, builder, index, opt_testName)
 
         var chromeRevision = g_resultsByBuilder[builder].chromeRevision[index];
         if (chromeRevision && isLayoutTestResults()) {
-            html += '<li><a href="' + TEST_RESULTS_BASE_PATH + g_builders[builder] +
+            html += '<li><a href="' + TEST_RESULTS_BASE_PATH + currentBuilders()[builder] +
                 '/' + chromeRevision + '/layout-test-results.zip">layout-test-results.zip</a></li>';
         }
     }
@@ -1626,7 +1626,7 @@ function generatePageForExpectationsUpdate()
         }
     }
 
-    for (var builder in g_builders) {
+    for (var builder in currentBuilders()) {
         var tests = g_perBuilderWithExpectationsButNoFailures[builder]
         for (var i = 0; i < tests.length; i++) {
             // Anything extra in this case is what is listed in expectations
@@ -1793,7 +1793,7 @@ function htmlForIndividualTestOnAllBuilders(test)
     }
 
     var skippedBuilders = []
-    for (builder in currentBuilderGroup().builders) {
+    for (builder in currentBuilders()) {
         if (shownBuilders.indexOf(builder) == -1)
             skippedBuilders.push(builder);
     }
@@ -2255,7 +2255,7 @@ function loadExpectationsLayoutTests(test, expectationsContainer)
     var revisionContainer = document.createElement('div');
     revisionContainer.textContent = "Showing results for: "
     expectationsContainer.appendChild(revisionContainer);
-    for (var builder in g_builders) {
+    for (var builder in currentBuilders()) {
         if (builderMaster(builder).name == WEBKIT_BUILDER_MASTER) {
             var latestRevision = g_currentState.revision || g_resultsByBuilder[builder].webkitRevision[0];
             var buildInfo = buildInfoForRevision(builder, latestRevision);
@@ -2272,7 +2272,7 @@ function loadExpectationsLayoutTests(test, expectationsContainer)
     var testWithoutSuffix = test.substring(0, test.lastIndexOf('.'));
     var actualResultSuffixes = ['-actual.txt', '-actual.png', '-crash-log.txt', '-diff.txt', '-wdiff.html', '-diff.png'];
 
-    for (var builder in g_builders) {
+    for (var builder in currentBuilders()) {
         var actualResultsBase;
         if (builderMaster(builder).name == WEBKIT_BUILDER_MASTER) {
             var latestRevision = g_currentState.revision || g_resultsByBuilder[builder].webkitRevision[0];
@@ -2280,7 +2280,7 @@ function loadExpectationsLayoutTests(test, expectationsContainer)
             actualResultsBase = 'http://build.webkit.org/results/' + builder +
                 '/r' + buildInfo.revisionStart + ' (' + buildInfo.buildNumber + ')/';
         } else
-            actualResultsBase = TEST_RESULTS_BASE_PATH + g_builders[builder] + '/results/layout-test-results/';
+            actualResultsBase = TEST_RESULTS_BASE_PATH + currentBuilders()[builder] + '/results/layout-test-results/';
 
         for (var i = 0; i < actualResultSuffixes.length; i++) {
             addExpectationItem(expectationsContainers, expectationsContainer, null,
index ac264b407e9f3f3497698d394902641e25c5b3c2..2da6341bb78afabc6dbb780e1a982deea5b14a96 100644 (file)
@@ -32,7 +32,6 @@ function resetGlobals()
     allTests = null;
     g_expectationsByPlatform = {};
     g_resultsByBuilder = {};
-    g_builders = {};
     g_allExpectations = null;
     g_allTestsTrie = null;
     g_currentState = {};
@@ -45,26 +44,35 @@ function resetGlobals()
     LOAD_BUILDBOT_DATA([{
         name: 'ChromiumWebkit',
         url: 'dummyurl', 
-        tests: {'layout-tests': {'builders': ['WebKit Linux', 'WebKit Linux (dbg)', 'WebKit Mac10.7', 'WebKit Win']}}
+        tests: {'layout-tests': {'builders': ['WebKit Linux', 'WebKit Linux (dbg)', 'WebKit Mac10.7', 'WebKit Win', 'WebKit Win (dbg)']}}
     },
     {
         name: 'webkit.org',
         url: 'dummyurl',
         tests: {'layout-tests': {'builders': ['Apple SnowLeopard Tests', 'Qt Linux Tests', 'Chromium Mac10.7 Tests', 'GTK Win']}}
     }]);
     for (var group in LAYOUT_TESTS_BUILDER_GROUPS)
         LAYOUT_TESTS_BUILDER_GROUPS[group] = null;
 }
 
-function runExpectationsTest(builder, test, expectations, modifiers)
+function stubResultsByBuilder(data)
 {
-    g_builders[builder] = true;
+    for (var builder in currentBuilders())
+    {
+        g_resultsByBuilder[builder] = data[builder] || {'tests': []};
+    };
+}
 
+function runExpectationsTest(builder, test, expectations, modifiers)
+{
     // Put in some dummy results. processExpectations expects the test to be
     // there.
     var tests = {};
     tests[test] = {'results': [[100, 'F']], 'times': [[100, 0]]};
-    g_resultsByBuilder[builder] = {'tests': tests};
+    var results = {};
+    results[builder] = {'tests': tests};
+    stubResultsByBuilder(results);
 
     processExpectations();
     var resultsForTest = createResultsObjectForTest(test, builder);
@@ -94,6 +102,8 @@ test('flattenTrie', 1, function() {
 
 test('releaseFail', 2, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
+
     var builder = 'WebKit Win';
     var test = 'foo/1.html';
     var expectationsArray = [
@@ -105,6 +115,7 @@ test('releaseFail', 2, function() {
 
 test('releaseFailDebugCrashReleaseBuilder', 2, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
     var builder = 'WebKit Win';
     var test = 'foo/1.html';
     var expectationsArray = [
@@ -118,6 +129,7 @@ test('releaseFailDebugCrashReleaseBuilder', 2, function() {
 
 test('releaseFailDebugCrashDebugBuilder', 2, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
     var builder = 'WebKit Win (dbg)';
     var test = 'foo/1.html';
     var expectationsArray = [
@@ -131,6 +143,7 @@ test('releaseFailDebugCrashDebugBuilder', 2, function() {
 
 test('overrideJustBuildType', 12, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
     var test = 'bar/1.html';
     g_expectationsByPlatform['CHROMIUM'] = getParsedExpectations('bar [ WontFix Failure Pass Timeout ]\n' +
         '[ Mac ] ' + test + ' [ WontFix Failure ]\n' +
@@ -227,9 +240,10 @@ test('filterBugs',4, function() {
 
 test('getExpectations', 16, function() {
     resetGlobals();
-    g_builders['WebKit Win'] = true;
-    g_resultsByBuilder = {
-        'WebKit Win': {
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
+    stubResultsByBuilder({
+        'WebKit Win' : {
             'tests': {
                 'foo/test1.html': {'results': [[100, 'F']], 'times': [[100, 0]]},
                 'foo/test2.html': {'results': [[100, 'F']], 'times': [[100, 0]]},
@@ -237,7 +251,7 @@ test('getExpectations', 16, function() {
                 'test1.html': {'results': [[100, 'F']], 'times': [[100, 0]]}
             }
         }
-    }
+    });
 
     g_expectationsByPlatform['CHROMIUM'] = getParsedExpectations('Bug(123) foo [ Failure Pass Crash ]\n' +
         'Bug(Foo) [ Release ] foo/test1.html [ Failure ]\n' +
@@ -316,6 +330,7 @@ test('substringList', 2, function() {
 });
 
 test('htmlForTestsWithExpectationsButNoFailures', 4, function() {
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
     var builder = 'WebKit Win';
     g_perBuilderWithExpectationsButNoFailures[builder] = ['passing-test1.html', 'passing-test2.html'];
     g_perBuilderSkippedPaths[builder] = ['skipped-test1.html'];
@@ -368,11 +383,13 @@ test('htmlForTestTypeSwitcherGroup', 6, function() {
 
 test('htmlForIndividualTestOnAllBuilders', 1, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
     equal(htmlForIndividualTestOnAllBuilders('foo/nonexistant.html'), '<div class="not-found">Test not found. Either it does not exist, is skipped or passes on all platforms.</div>');
 });
 
 test('htmlForIndividualTestOnAllBuildersWithResultsLinksNonexistant', 1, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
     equal(htmlForIndividualTestOnAllBuildersWithResultsLinks('foo/nonexistant.html'),
         '<div class="not-found">Test not found. Either it does not exist, is skipped or passes on all platforms.</div>' +
         '<div class=expectations test=foo/nonexistant.html>' +
@@ -405,7 +422,7 @@ test('htmlForIndividualTestOnAllBuildersWithResultsLinks', 1, function() {
         '</table>' +
         '<div>The following builders either don\'t run this test (e.g. it\'s skipped) or all runs passed:</div>' +
         '<div class=skipped-builder-list>' +
-            '<div class=skipped-builder>WebKit Linux (dbg)</div><div class=skipped-builder>WebKit Mac10.7</div><div class=skipped-builder>WebKit Win</div>' +
+            '<div class=skipped-builder>WebKit Linux (dbg)</div><div class=skipped-builder>WebKit Mac10.7</div><div class=skipped-builder>WebKit Win</div><div class=skipped-builder>WebKit Win (dbg)</div>' +
         '</div>' +
         '<div class=expectations test=dummytest.html>' +
             '<div><span class=link onclick="setQueryParameter(\'showExpectations\', true)">Show results</span> | ' +
@@ -449,6 +466,7 @@ test('htmlForIndividualTestOnAllBuildersWithResultsLinksWebkitMaster', 1, functi
 
 test('htmlForIndividualTests', 4, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
     var test1 = 'foo/nonexistant.html';
     var test2 = 'bar/nonexistant.html';
 
index c748901617b8e47857cb68f43a55f7050e99ee81..a6d8543060892e2048badc3e731005ae53013122 100644 (file)
@@ -90,7 +90,7 @@ loader.Loader.prototype = {
     {
         parseParameters();
 
-        for (var builderName in g_builders)
+        for (var builderName in currentBuilders())
             this._loadResultsFileForBuilder(builderName);
     },
     _loadResultsFileForBuilder: function(builderName)
@@ -167,12 +167,12 @@ loader.Loader.prototype = {
 
         // Remove this builder from builders, so we don't try to use the
         // data that isn't there.
-        delete g_builders[builderName];
+        delete currentBuilders()[builderName];
 
         // Change the default builder name if it has been deleted.
         if (g_defaultBuilderName == builderName) {
             g_defaultBuilderName = null;
-            for (var availableBuilderName in g_builders) {
+            for (var availableBuilderName in currentBuilders()) {
                 g_defaultBuilderName = availableBuilderName;
                 g_defaultDashboardSpecificStateValues.builder = availableBuilderName;
                 break;
@@ -194,7 +194,7 @@ loader.Loader.prototype = {
     },
     _haveResultsFilesLoaded: function()
     {
-        for (var builder in g_builders) {
+        for (var builder in currentBuilders()) {
             if (!g_resultsByBuilder[builder])
                 return false;
         }
index 8f60bb63e190c7a7eca1c73813f292b6bd27ec90..4c1e76ca0bb96f0c520a11d586f358bb9823d897 100644 (file)
@@ -54,9 +54,11 @@ test('loading steps', 1, function() {
     }
 });
 
-test('results files loading', 5, function() {
+// Total number of assertions is 1 for the deepEqual of the builder lists
+// and then 2 per builder (one for ok, one for deepEqual of tests).
+test('results files loading', 11, function() {
     resetGlobals();
-    var expectedLoadedBuilders = ["WebKit Linux", "WebKit Win"];
+    var expectedLoadedBuilders =  ['WebKit Linux', 'WebKit Linux (dbg)', 'WebKit Mac10.7', 'WebKit Win', 'WebKit Win (dbg)'];
     var loadedBuilders = [];
     var resourceLoader = new loader.Loader();
     resourceLoader._loadNext = function() {
@@ -69,16 +71,13 @@ test('results files loading', 5, function() {
 
     var requestFunction = loader.request;
     loader.request = function(url, successCallback, errorCallback) {
-        var builderName = /builder=([\w ]+)&/.exec(url)[1];
+        var builderName = /builder=([\w ().]+)&/.exec(url)[1];
         loadedBuilders.push(builderName);
         successCallback({responseText: '{"version": 4, "' + builderName + '": {"secondsSinceEpoch": [' + Date.now() + '], "tests": {}}}'});
     }
 
-    g_builders = {"WebKit Linux": true, "WebKit Win": true};
-
-    builders.masters['ChromiumWebkit'] = {'tests': {'layout-tests': {builders: ["WebKit Linux", "WebKit Win"]}}};
     loadBuildersList('@ToT - chromium.org', 'layout-tests');
-
     try {
         resourceLoader._loadResultsFiles();
     } finally {
@@ -112,6 +111,8 @@ test('expectations files loading', 1, function() {
 
 test('results file failing to load', 2, function() {
     resetGlobals();
+    loadBuildersList('@ToT - chromium.org', 'layout-tests');
+    
     // FIXME: loader shouldn't depend on state defined in dashboard_base.js.
     g_buildersThatFailedToLoad = [];
 
@@ -122,14 +123,14 @@ test('results file failing to load', 2, function() {
     }
 
     var builder1 = 'builder1';
-    g_builders[builder1] = true;
+    currentBuilders()[builder1] = true;
     resourceLoader._handleResultsFileLoadError(builder1);
 
     var builder2 = 'builder2';
-    g_builders[builder2] = true;
+    currentBuilders()[builder2] = true;
     resourceLoader._handleResultsFileLoadError(builder2);
 
     deepEqual(g_buildersThatFailedToLoad, [builder1, builder2]);
     equal(resourceLoadCount, 2);
 
-})
+});
index 4cdd9a86b49583008cd47a57bbabb51afe90ea78..d4aa8e858b35f70e5982761a22e4c06d04e83d35 100644 (file)
@@ -291,7 +291,7 @@ function updateBuildInspector(results, builder, dygraph, index)
             ' (' + results[BUILD_NUMBERS_KEY][index] + ')';
     } else {
         var resultsUrl = 'http://build.chromium.org/f/chromium/layout_test_results/' +
-            g_builders[builder] + '/' + results[CHROME_REVISIONS_KEY][index];
+            currentBuilders()[builder] + '/' + results[CHROME_REVISIONS_KEY][index];
     }
 
     addRow('Build:', '<a href="' + buildUrl + '" target="_blank">' + buildNumber + '</a> (<a href="' + resultsUrl + '" target="_blank">results</a>)');
index aa7ae437ef7c1e41243e7e157939863d2ae90971..da1550531f63f5a83c3ec74500e076ccabd9e2a9 100644 (file)
@@ -274,7 +274,7 @@ function handleValidHashParameter(key, value)
     switch(key) {
     case 'builder':
         validateParameter(g_currentState, key, value,
-            function() { return value in g_builders; });
+            function() { return value in currentBuilders(); });
         return true;
 
     case 'treemapfocus':