Associated commits don't immediately show up on an analysis task page

https://bugs.webkit.org/show_bug.cgi?id=155692

Reviewed by Darin Adler.

The bug was caused by resolveCommits in AnalysisTask._constructAnalysisTasksFromRawData not being
able to find the matching commit log if the commit log had been created by the charts which don't
set the remote identifiers on each CommitLog objects.

Fixed the bug by modifying /api/measurement-set to include the commit ID, and making CommitLog
use the real database ID as its ID instead of a fake ID we create from repository and revision.

Also added a bunch of Mocha unit tests for AnalysisTask.fetchAll.

* public/api/measurement-set.php:
(MeasurementSetFetcher::execute_query): Fetch commit_id.
(MeasurementSetFetcher::format_run): Use pass-by-reference to avoid making a copy of the row.
(MeasurementSetFetcher::parse_revisions_array): Include commit_id as the first item in the result.
* public/v3/instrumentation.js:
* public/v3/models/analysis-task.js:
(AnalysisTask): Fixed a bug that _buildRequestCount and _finishedBuildRequestCount could be kept
as strings and hasPendingRequests() could return a wrong result because it would perform string
inequality instead of numerical inequality.
(AnalysisTask.prototype.updateSingleton): Ditto.
(AnalysisTask.prototype.dissociateCommit):
(AnalysisTask._constructAnalysisTasksFromRawData):
(AnalysisTask._constructAnalysisTasksFromRawData.resolveCommits): Use findById now that CommitLog
objects all use the same id as the database id.
* public/v3/models/commit-log.js:
(CommitLog):
(CommitLog.prototype.remoteId): Deleted since we no longer create a fake id for commit logs for
measurement sets.
(CommitLog.findByRemoteId): Deleted.
(CommitLog.ensureSingleton): Deleted.
(CommitLog.fetchBetweenRevisions):

* public/v3/models/data-model.js:
(DataModelObject.clearStaticMap): Added to aid unit testing.
(DataModelObject.ensureNamedStaticMap): Fixed a typo. Each map is a dictionary, not an array.
* public/v3/models/metric.js:
* public/v3/models/platform.js:
* public/v3/models/root-set.js:
(RootSet): Updated per the interface change in CommitLog.ensureSingleton.
(MeasurementRootSet): Updated per /api/measurement-set change. Use the first value as the id.
* public/v3/models/test.js:
* unit-tests/analysis-task-tests.js: Added.
(sampleAnalysisTask):
(measurementCluster):
* unit-tests/checkconfig.js: Added some assertion message to help aid diagnosing the failure.
* unit-tests/measurement-adaptor-tests.js: Updated the sample data per the API change in
/api/measurement-set and also added assertions for commit log ids.
* unit-tests/measurement-set-tests.js:
(beforeEach):
* unit-tests/resources: Added.
* unit-tests/resources/mock-remote-api.js: Added. Extracted from measurement-set-tests.js to be
used in analysis-task-tests.js.
(assert.notReached.assert.notReached):
(global.RemoteAPI.getJSON):
(global.RemoteAPI.getJSONWithStatus):
(beforeEach):
* unit-tests/resources/v3-models.js: Added. Extracted from measurement-set-tests.js to be used in
analysis-task-tests.js and added more imports as needed.
(importFromV3):
(beforeEach):


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

Author: [email protected]

Websites/perf.webkit.org/ChangeLog

@@ -1,3 +1,70 @@
+2016-03-19  Ryosuke Niwa  <[email protected]>
+
+        Associated commits don't immediately show up on an analysis task page
+        https://bugs.webkit.org/show_bug.cgi?id=155692
+
+        Reviewed by Darin Adler.
+
+        The bug was caused by resolveCommits in AnalysisTask._constructAnalysisTasksFromRawData not being
+        able to find the matching commit log if the commit log had been created by the charts which don't
+        set the remote identifiers on each CommitLog objects.
+
+        Fixed the bug by modifying /api/measurement-set to include the commit ID, and making CommitLog
+        use the real database ID as its ID instead of a fake ID we create from repository and revision.
+
+        Also added a bunch of Mocha unit tests for AnalysisTask.fetchAll.
+
+        * public/api/measurement-set.php:
+        (MeasurementSetFetcher::execute_query): Fetch commit_id.
+        (MeasurementSetFetcher::format_run): Use pass-by-reference to avoid making a copy of the row.
+        (MeasurementSetFetcher::parse_revisions_array): Include commit_id as the first item in the result.
+        * public/v3/instrumentation.js:
+        * public/v3/models/analysis-task.js:
+        (AnalysisTask): Fixed a bug that _buildRequestCount and _finishedBuildRequestCount could be kept
+        as strings and hasPendingRequests() could return a wrong result because it would perform string
+        inequality instead of numerical inequality.
+        (AnalysisTask.prototype.updateSingleton): Ditto.
+        (AnalysisTask.prototype.dissociateCommit):
+        (AnalysisTask._constructAnalysisTasksFromRawData):
+        (AnalysisTask._constructAnalysisTasksFromRawData.resolveCommits): Use findById now that CommitLog
+        objects all use the same id as the database id.
+        * public/v3/models/commit-log.js:
+        (CommitLog):
+        (CommitLog.prototype.remoteId): Deleted since we no longer create a fake id for commit logs for
+        measurement sets.
+        (CommitLog.findByRemoteId): Deleted.
+        (CommitLog.ensureSingleton): Deleted.
+        (CommitLog.fetchBetweenRevisions):
+
+        * public/v3/models/data-model.js:
+        (DataModelObject.clearStaticMap): Added to aid unit testing.
+        (DataModelObject.ensureNamedStaticMap): Fixed a typo. Each map is a dictionary, not an array.
+        * public/v3/models/metric.js:
+        * public/v3/models/platform.js:
+        * public/v3/models/root-set.js:
+        (RootSet): Updated per the interface change in CommitLog.ensureSingleton.
+        (MeasurementRootSet): Updated per /api/measurement-set change. Use the first value as the id.
+        * public/v3/models/test.js:
+        * unit-tests/analysis-task-tests.js: Added.
+        (sampleAnalysisTask):
+        (measurementCluster):
+        * unit-tests/checkconfig.js: Added some assertion message to help aid diagnosing the failure.
+        * unit-tests/measurement-adaptor-tests.js: Updated the sample data per the API change in
+        /api/measurement-set and also added assertions for commit log ids.
+        * unit-tests/measurement-set-tests.js:
+        (beforeEach):
+        * unit-tests/resources: Added.
+        * unit-tests/resources/mock-remote-api.js: Added. Extracted from measurement-set-tests.js to be
+        used in analysis-task-tests.js.
+        (assert.notReached.assert.notReached):
+        (global.RemoteAPI.getJSON):
+        (global.RemoteAPI.getJSONWithStatus):
+        (beforeEach):
+        * unit-tests/resources/v3-models.js: Added. Extracted from measurement-set-tests.js to be used in
+        analysis-task-tests.js and added more imports as needed.
+        (importFromV3):
+        (beforeEach):
+
 2016-03-18  Ryosuke Niwa  <[email protected]>
 
         Build fix after r198464.

Websites/perf.webkit.org/public/api/measurement-set.php

@@ -165,7 +165,7 @@ function fetch_next_cluster() {
     function execute_query($config_id) {
         return $this->db->query('
             SELECT test_runs.*, builds.*,
-            array_agg((commit_repository, commit_revision, commit_time)) AS revisions,
+            array_agg((commit_id, commit_repository, commit_revision, commit_time)) AS revisions,
             max(commit_time) AS revision_time, max(commit_order) AS revision_order
                 FROM builds
                     LEFT OUTER JOIN build_commits ON commit_build = build_id
@@ -180,7 +180,7 @@ static function format_map()
             'commitTime', 'build', 'buildTime', 'buildNumber', 'builder');
     }
 
-    private static function format_run($run, &$commit_time) {
+    private static function format_run(&$run, &$commit_time) {
         $commit_time = Database::to_js_time($run['revision_time']);
         $build_time = Database::to_js_time($run['build_time']);
         if (!$commit_time)
@@ -200,16 +200,19 @@ private static function format_run($run, &$commit_time) {
             intval($run['build_builder']));
     }
 
-    private static function parse_revisions_array($postgres_array) {
-        // e.g. {"(WebKit,131456,\"2012-10-16 14:53:00\")","(Chromium,162004,)"}
+    private static function parse_revisions_array(&$postgres_array) {
+        // e.g. {"(<commit-id>,<repository-id>,<revision>,\"2012-10-16 14:53:00\")","(<commit-id>,<repository-id>,<revision>,)"}
         $outer_array = json_decode('[' . trim($postgres_array, '{}') . ']');
         $revisions = array();
         foreach ($outer_array as $item) {
             $name_and_revision = explode(',', trim($item, '()'));
             if (!$name_and_revision[0])
                 continue;
-            $time = Database::to_js_time(trim($name_and_revision[2], '"'));
-            array_push($revisions, array(intval(trim($name_and_revision[0], '"')), trim($name_and_revision[1], '"'), $time));
+            $commit_id = intval(trim($name_and_revision[0], '"'));
+            $repository_id = intval(trim($name_and_revision[1], '"'));
+            $revision = trim($name_and_revision[2], '"');
+            $time = Database::to_js_time(trim($name_and_revision[3], '"'));
+            array_push($revisions, array($commit_id, $repository_id, $revision, $time));
         }
         return $revisions;
     }

Websites/perf.webkit.org/public/v3/instrumentation.js

@@ -1,3 +1,5 @@
+'use strict';
+
 class Instrumentation {
 
     static startMeasuringTime(domain, label)
@@ -53,3 +55,6 @@ class Instrumentation {
     }
 
 }
+
+if (typeof module != 'undefined')
+    module.exports.Instrumentation = Instrumentation;

Websites/perf.webkit.org/public/v3/models/analysis-task.js

@@ -1,3 +1,4 @@
+'use strict';
 
 class AnalysisTask extends LabeledObject {
     constructor(id, object)
@@ -22,8 +23,8 @@ class AnalysisTask extends LabeledObject {
         this._bugs = object.bugs || [];
         this._causes = object.causes || [];
         this._fixes = object.fixes || [];
-        this._buildRequestCount = object.buildRequestCount;
-        this._finishedBuildRequestCount = object.finishedBuildRequestCount;
+        this._buildRequestCount = +object.buildRequestCount;
+        this._finishedBuildRequestCount = +object.finishedBuildRequestCount;
     }
 
     static findByPlatformAndMetric(platformId, metricId)
@@ -50,8 +51,8 @@ class AnalysisTask extends LabeledObject {
         this._bugs = object.bugs || [];
         this._causes = object.causes || [];
         this._fixes = object.fixes || [];
-        this._buildRequestCount = object.buildRequestCount;
-        this._finishedBuildRequestCount = object.finishedBuildRequestCount;
+        this._buildRequestCount = +object.buildRequestCount;
+        this._finishedBuildRequestCount = +object.finishedBuildRequestCount;
     }
 
     hasResults() { return this._finishedBuildRequestCount; }
@@ -138,7 +139,7 @@ class AnalysisTask extends LabeledObject {
         var id = this.id();
         return PrivilegedAPI.sendRequest('associate-commit', {
             task: id,
-            commit: commit.remoteId(),
+            commit: commit.id(),
         }).then(function (data) {
             return AnalysisTask.cachedFetch('../api/analysis-tasks', {id: id}, true)
                 .then(AnalysisTask._constructAnalysisTasksFromRawData.bind(AnalysisTask));
@@ -232,11 +233,11 @@ class AnalysisTask extends LabeledObject {
             rawData.repository = Repository.findById(rawData.repository);
             if (!rawData.repository)
                 continue;
-            CommitLog.ensureSingleton(rawData.repository, rawData);
+            CommitLog.ensureSingleton(rawData.id, rawData);
         }
 
         function resolveCommits(commits) {
-            return commits.map(function (id) { return CommitLog.findByRemoteId(id); }).filter(function (commit) { return !!commit; });
+            return commits.map(function (id) { return CommitLog.findById(id); }).filter(function (commit) { return !!commit; });
         }
 
         var results = [];
@@ -266,3 +267,6 @@ class AnalysisTask extends LabeledObject {
         });
     }
 }
+
+if (typeof module != 'undefined')
+    module.exports.AnalysisTask = AnalysisTask;

Websites/perf.webkit.org/public/v3/models/commit-log.js

@@ -5,27 +5,13 @@ class CommitLog extends DataModelObject {
     {
         super(id);
         this._repository = rawData.repository;
+        console.assert(this._repository instanceof Repository);
         this._rawData = rawData;
         this._remoteId = rawData.id;
         if (this._remoteId)
             this.ensureNamedStaticMap('remoteId')[this._remoteId] = this;
     }
 
-    // FIXME: All this non-sense should go away once measurement-set start returning real commit id.
-    remoteId() { return this._remoteId; }
-    static findByRemoteId(id)
-    {
-        var remoteIdMap = super.namedStaticMap('remoteId');
-        return remoteIdMap ? remoteIdMap[id] : null;
-    }
-
-    static ensureSingleton(repository, rawData)
-    {
-        var id = repository.id() + '-' + rawData['revision'];
-        rawData.repository = repository;
-        return super.ensureSingleton(id, rawData);
-    }
-
     updateSingleton(rawData)
     {
         super.updateSingleton(rawData);
@@ -99,7 +85,10 @@ class CommitLog extends DataModelObject {
 
         var self = this;
         return RemoteAPI.getJSONWithStatus(url).then(function (data) {
-            var commits = data['commits'].map(function (rawData) { return CommitLog.ensureSingleton(repository, rawData); });
+            var commits = data['commits'].map(function (rawData) {
+                rawData.repository = repository;
+                return CommitLog.ensureSingleton(rawData.id, rawData);
+            });
             self._cacheCommitLogs(repository, from, to, commits);
             return commits;
         });

Websites/perf.webkit.org/public/v3/models/data-model.js

@@ -20,6 +20,8 @@ class DataModelObject {
 
     updateSingleton(object) { }
 
+    static clearStaticMap() { this[DataModelObject.StaticMapSymbol] = null; }
+
     static namedStaticMap(name)
     {
         var staticMap = this[DataModelObject.StaticMapSymbol];
@@ -32,7 +34,7 @@ class DataModelObject {
             this[DataModelObject.StaticMapSymbol] = {};
         var staticMap = this[DataModelObject.StaticMapSymbol];
         if (!staticMap[name])
-            staticMap[name] = [];
+            staticMap[name] = {};
         return staticMap[name];
     }
 

Websites/perf.webkit.org/public/v3/models/metric.js

@@ -1,3 +1,4 @@
+'use strict';
 
 class Metric extends LabeledObject {
     constructor(id, object)
@@ -86,3 +87,6 @@ class Metric extends LabeledObject {
         }
     };
 }
+
+if (typeof module != 'undefined')
+    module.exports.Metric = Metric;

Websites/perf.webkit.org/public/v3/models/platform.js

@@ -1,3 +1,4 @@
+'use strict';
 
 class Platform extends LabeledObject {
     constructor(id, object)
@@ -34,3 +35,6 @@ class Platform extends LabeledObject {
         return this._lastModifiedByMetric[metric.id()];
     }
 }
+
+if (typeof module != 'undefined')
+    module.exports.Platform = Platform;

Websites/perf.webkit.org/public/v3/models/root-set.js

@@ -14,11 +14,11 @@ class RootSet extends DataModelObject {
 
         for (var row of object.roots) {
             var repositoryId = row.repository;
-            var repository = Repository.findById(repositoryId);
+            row.repository = Repository.findById(repositoryId);
 
             console.assert(!this._repositoryToCommitMap[repositoryId]);
-            this._repositoryToCommitMap[repositoryId] = CommitLog.ensureSingleton(repository, row);
-            this._repositories.push(repository);
+            this._repositoryToCommitMap[repositoryId] = CommitLog.ensureSingleton(row.id, row);
+            this._repositories.push(row.repository);
         }
     }
 
@@ -74,12 +74,16 @@ class MeasurementRootSet extends RootSet {
     {
         super(id, null);
         for (var values of revisionList) {
-            var repositoryId = values[0];
+            // [<commit-id>, <repository-id>, <revision>, <time>]
+            var commitId = values[0];
+            var repositoryId = values[1];
+            var revision = values[2];
+            var time = values[3];
             var repository = Repository.findById(repositoryId);
             if (!repository)
                 continue;
 
-            this._repositoryToCommitMap[repositoryId] = CommitLog.ensureSingleton(repository, {revision: values[1], time: values[2]});
+            this._repositoryToCommitMap[repositoryId] = CommitLog.ensureSingleton(commitId, {repository: repository, revision: revision, time: time});
             this._repositories.push(repository);
         }
     }

Websites/perf.webkit.org/public/v3/models/test.js

@@ -1,3 +1,4 @@
+'use strict';
 
 class Test extends LabeledObject {
     constructor(id, object, isTopLevel)
@@ -42,3 +43,6 @@ class Test extends LabeledObject {
     addChildTest(test) { this._childTests.push(test); }
     addMetric(metric) { this._metrics.push(metric); }
 }
+
+if (typeof module != 'undefined')
+    module.exports.Test = Test;