Diff
Modified: trunk/Tools/ChangeLog (137692 => 137693)
--- trunk/Tools/ChangeLog 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/ChangeLog 2012-12-14 02:02:51 UTC (rev 137693)
@@ -1,3 +1,39 @@
+2012-12-13 Julie Parent <[email protected]>
+
+ 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 <[email protected]>
Callers should not have to stringify args before calling Executive run_command/popen
Modified: trunk/Tools/TestResultServer/static-dashboards/aggregate_results.html (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/aggregate_results.html 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/aggregate_results.html 2012-12-14 02:02:51 UTC (rev 137693)
@@ -68,7 +68,7 @@
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;
}
Modified: trunk/Tools/TestResultServer/static-dashboards/builders.js (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/builders.js 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/builders.js 2012-12-14 02:02:51 UTC (rev 137693)
@@ -127,7 +127,6 @@
// 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()
Modified: trunk/Tools/TestResultServer/static-dashboards/dashboard_base.js (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/dashboard_base.js 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/dashboard_base.js 2012-12-14 02:02:51 UTC (rev 137693)
@@ -188,7 +188,7 @@
// 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 @@
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 @@
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);
Modified: trunk/Tools/TestResultServer/static-dashboards/flakiness_dashboard.js (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/flakiness_dashboard.js 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/flakiness_dashboard.js 2012-12-14 02:02:51 UTC (rev 137693)
@@ -155,7 +155,7 @@
else
generatePageForBuilder(g_currentState.builder);
- for (var builder in g_builders)
+ for (var builder in currentBuilders())
processTestResultsForBuilderAsync(builder);
postHeightChangedMessage();
@@ -186,7 +186,7 @@
case 'builder':
validateParameter(g_currentState, key, value,
function() {
- return value in g_builders;
+ return value in currentBuilders();
});
return true;
@@ -479,7 +479,7 @@
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 @@
{
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 processTestRunsForAllBuilders()
{
- for (var builder in g_builders)
+ for (var builder in currentBuilders())
processTestRunsForBuilder(builder);
}
@@ -1236,7 +1236,7 @@
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 @@
}
}
- 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 @@
}
var skippedBuilders = []
- for (builder in currentBuilderGroup().builders) {
+ for (builder in currentBuilders()) {
if (shownBuilders.indexOf(builder) == -1)
skippedBuilders.push(builder);
}
@@ -2255,7 +2255,7 @@
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 @@
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 @@
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,
Modified: trunk/Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js 2012-12-14 02:02:51 UTC (rev 137693)
@@ -32,7 +32,6 @@
allTests = null;
g_expectationsByPlatform = {};
g_resultsByBuilder = {};
- g_builders = {};
g_allExpectations = null;
g_allTestsTrie = null;
g_currentState = {};
@@ -45,26 +44,35 @@
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 stubResultsByBuilder(data)
+{
+ for (var builder in currentBuilders())
+ {
+ g_resultsByBuilder[builder] = data[builder] || {'tests': []};
+ };
+}
+
function runExpectationsTest(builder, test, expectations, modifiers)
{
- g_builders[builder] = true;
-
// 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('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('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('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('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('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 @@
'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('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('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 @@
'</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('htmlForIndividualTests', 4, function() {
resetGlobals();
+ loadBuildersList('@ToT - chromium.org', 'layout-tests');
var test1 = 'foo/nonexistant.html';
var test2 = 'bar/nonexistant.html';
Modified: trunk/Tools/TestResultServer/static-dashboards/loader.js (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/loader.js 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/loader.js 2012-12-14 02:02:51 UTC (rev 137693)
@@ -90,7 +90,7 @@
{
parseParameters();
- for (var builderName in g_builders)
+ for (var builderName in currentBuilders())
this._loadResultsFileForBuilder(builderName);
},
_loadResultsFileForBuilder: function(builderName)
@@ -167,12 +167,12 @@
// 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 @@
},
_haveResultsFilesLoaded: function()
{
- for (var builder in g_builders) {
+ for (var builder in currentBuilders()) {
if (!g_resultsByBuilder[builder])
return false;
}
Modified: trunk/Tools/TestResultServer/static-dashboards/loader_unittests.js (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/loader_unittests.js 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/loader_unittests.js 2012-12-14 02:02:51 UTC (rev 137693)
@@ -54,9 +54,11 @@
}
});
-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 @@
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('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 @@
}
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);
-})
+});
Modified: trunk/Tools/TestResultServer/static-dashboards/timeline_explorer.html (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/timeline_explorer.html 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/timeline_explorer.html 2012-12-14 02:02:51 UTC (rev 137693)
@@ -291,7 +291,7 @@
' (' + 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>)');
Modified: trunk/Tools/TestResultServer/static-dashboards/treemap.html (137692 => 137693)
--- trunk/Tools/TestResultServer/static-dashboards/treemap.html 2012-12-14 01:58:05 UTC (rev 137692)
+++ trunk/Tools/TestResultServer/static-dashboards/treemap.html 2012-12-14 02:02:51 UTC (rev 137693)
@@ -274,7 +274,7 @@
switch(key) {
case 'builder':
validateParameter(g_currentState, key, value,
- function() { return value in g_builders; });
+ function() { return value in currentBuilders(); });
return true;
case 'treemapfocus':
