Opening an analysis task from the queue page is broken
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 May 2017 23:54:43 +0000 (23:54 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 May 2017 23:54:43 +0000 (23:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172559
<rdar://problem/32389708>

Rubber-stamped by Chris Dumez.

Fix the bug that opening the analysis task page from the queue page results in multiple assertion failures
as well as the list of test groups in the analysis task page not getting updated.

* public/v3/models/build-request.js:
(BuildRequest.prototype.updateSingleton): Because /api/build-requests/ do not include test groups, it's
possible for testGroup to be dynamically updated upon loading an analysis task page. Update _testGroup in
such instances instead of asserting that it doesn't happen.

* public/v3/models/data-model.js:
(DataModelObject.cachedFetch): Because various code to create model objects from the result of a JSON API
modify the fetched content in irreversible manner, e.g. `object.platform = Platform.findById(object.platform)`
we must return a fresh new content each time even if the result had been cached.

* public/v3/models/test-group.js:
(TestGroup.prototype.platform): Return this._platform as that's not available.

* public/v3/pages/analysis-task-page.js:
(AnalysisTaskPage):
(AnalysisTaskPage.prototype._resetVariables): Extracted from the constructor.
(AnalysisTaskPage.prototype.updateFromSerializedState): Reset all instance variables when opening a new
analysis task page. This would avoid showing the stale result even when fetching new test groups had failed.

* unit-tests/test-groups-tests.js: Added a test case for fetching the same test group twice. This used to hit
a problem in BuildRequest.constructBuildRequestsFromData which overrode platform property of each raw content
with a Platform model object because in the case of a cached fetch, we end up trying to look up the platform
again using the result of stringifying the Platform object instead of the platform ID included in the original
fetched content.
(sampleTestGroup): Added "platform" as included in the JSON API's response now.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/v3/models/build-request.js
Websites/perf.webkit.org/public/v3/models/data-model.js
Websites/perf.webkit.org/public/v3/models/test-group.js
Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js
Websites/perf.webkit.org/unit-tests/test-groups-tests.js

index e707417..9087956 100644 (file)
@@ -1,5 +1,42 @@
 2017-05-24  Ryosuke Niwa  <rniwa@webkit.org>
 
+        Opening an analysis task from the queue page is broken
+        https://bugs.webkit.org/show_bug.cgi?id=172559
+        <rdar://problem/32389708>
+
+        Rubber-stamped by Chris Dumez.
+
+        Fix the bug that opening the analysis task page from the queue page results in multiple assertion failures
+        as well as the list of test groups in the analysis task page not getting updated.
+
+        * public/v3/models/build-request.js:
+        (BuildRequest.prototype.updateSingleton): Because /api/build-requests/ do not include test groups, it's
+        possible for testGroup to be dynamically updated upon loading an analysis task page. Update _testGroup in
+        such instances instead of asserting that it doesn't happen.
+
+        * public/v3/models/data-model.js:
+        (DataModelObject.cachedFetch): Because various code to create model objects from the result of a JSON API
+        modify the fetched content in irreversible manner, e.g. `object.platform = Platform.findById(object.platform)`
+        we must return a fresh new content each time even if the result had been cached.
+
+        * public/v3/models/test-group.js:
+        (TestGroup.prototype.platform): Return this._platform as that's not available.
+
+        * public/v3/pages/analysis-task-page.js:
+        (AnalysisTaskPage):
+        (AnalysisTaskPage.prototype._resetVariables): Extracted from the constructor.
+        (AnalysisTaskPage.prototype.updateFromSerializedState): Reset all instance variables when opening a new
+        analysis task page. This would avoid showing the stale result even when fetching new test groups had failed.
+
+        * unit-tests/test-groups-tests.js: Added a test case for fetching the same test group twice. This used to hit
+        a problem in BuildRequest.constructBuildRequestsFromData which overrode platform property of each raw content
+        with a Platform model object because in the case of a cached fetch, we end up trying to look up the platform
+        again using the result of stringifying the Platform object instead of the platform ID included in the original
+        fetched content.
+        (sampleTestGroup): Added "platform" as included in the JSON API's response now.
+
+2017-05-24  Ryosuke Niwa  <rniwa@webkit.org>
+
         The commit log viewer can overlap the analysis results viewer
         https://bugs.webkit.org/show_bug.cgi?id=172534
 
index 32ad431..819b63c 100644 (file)
@@ -30,9 +30,15 @@ class BuildRequest extends DataModelObject {
 
     updateSingleton(object)
     {
-        console.assert(this._testGroup == object.testGroup);
         console.assert(this._order == object.order);
         console.assert(this._commitSet == object.commitSet);
+
+        const testGroup = object.testGroup;
+        console.assert(!this._testGroup || this._testGroup == testGroup);
+        if (!this._testGroup && testGroup)
+            testGroup.addBuildRequest(this);
+
+        this._testGroup = testGroup;
         this._status = object.status;
         this._statusUrl = object.url;
         this._buildId = object.build;
index 63170f7..f189298 100644 (file)
@@ -77,7 +77,9 @@ class DataModelObject {
         if (!cacheMap[path])
             cacheMap[path] = RemoteAPI.getJSONWithStatus(path);
 
-        return cacheMap[path];
+        return cacheMap[path].then((content) => {
+            return JSON.parse(JSON.stringify(content));
+        });
     }
 
 }
index cb9f9a3..b6db583 100644 (file)
@@ -49,11 +49,7 @@ class TestGroup extends LabeledObject {
         return request ? request.test() : null;
     }
 
-    platform()
-    {
-        const request = this._lastRequest();
-        return request ? request.platform() : null;
-    }
+    platform() { return this._platform; }
 
     _lastRequest()
     {
index b6ea89e..d580d53 100644 (file)
@@ -447,6 +447,17 @@ class AnalysisTaskPage extends PageWithHeading {
     constructor()
     {
         super('Analysis Task');
+        this._renderTaskNameAndStatusLazily = new LazilyEvaluatedFunction(this._renderTaskNameAndStatus.bind(this));
+        this._renderCauseAndFixesLazily = new LazilyEvaluatedFunction(this._renderCauseAndFixes.bind(this));
+        this._renderRelatedTasksLazily = new LazilyEvaluatedFunction(this._renderRelatedTasks.bind(this));
+        this._resetVariables();
+    }
+
+    title() { return this._task ? this._task.label() : 'Analysis Task'; }
+    routeName() { return 'analysis/task'; }
+
+    _resetVariables()
+    {
         this._task = null;
         this._metric = null;
         this._triggerable = null;
@@ -461,17 +472,11 @@ class AnalysisTaskPage extends PageWithHeading {
         this._currentTestGroup = null;
         this._filteredTestGroups = null;
         this._showHiddenTestGroups = false;
-
-        this._renderTaskNameAndStatusLazily = new LazilyEvaluatedFunction(this._renderTaskNameAndStatus.bind(this));
-        this._renderCauseAndFixesLazily = new LazilyEvaluatedFunction(this._renderCauseAndFixes.bind(this));
-        this._renderRelatedTasksLazily = new LazilyEvaluatedFunction(this._renderRelatedTasks.bind(this));
     }
 
-    title() { return this._task ? this._task.label() : 'Analysis Task'; }
-    routeName() { return 'analysis/task'; }
-
     updateFromSerializedState(state)
     {
+        this._resetVariables();
         if (state.remainingRoute) {
             const taskId = parseInt(state.remainingRoute);
             AnalysisTask.fetchById(taskId).then(this._didFetchTask.bind(this)).then(() => {
index 8941b64..f3e833c 100644 (file)
@@ -1,15 +1,17 @@
 'use strict';
 
-var assert = require('assert');
+const assert = require('assert');
 
 require('../tools/js/v3-models.js');
-let MockModels = require('./resources/mock-v3-models.js').MockModels;
+const MockModels = require('./resources/mock-v3-models.js').MockModels;
+const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
 
 function sampleTestGroup() {
     return {
         "testGroups": [{
             "id": "2128",
             "task": "1376",
+            "platform": "31",
             "name": "Confirm",
             "author": "rniwa",
             "createdAt": 1458688514000,
@@ -105,6 +107,37 @@ function sampleTestGroup() {
 describe('TestGroup', function () {
     MockModels.inject();
 
+    describe('fetchForTask', () => {
+        const requests = MockRemoteAPI.inject('https://perf.webkit.org');
+
+        it('should be able to fetch the list of test groups for the same task twice using cache', () => {
+            const fetchPromise = TestGroup.fetchForTask(1376);
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].method, 'GET');
+            assert.equal(requests[0].url, '/api/test-groups?task=1376');
+            requests[0].resolve(sampleTestGroup());
+
+            let assertTestGroups = (testGroups) => {
+                assert.equal(testGroups.length, 1);
+                assert.equal(testGroups[0].platform(), MockModels.elCapitan);
+                const buildRequests = testGroups[0].buildRequests();
+                assert.equal(buildRequests.length, 4);
+                for (let request of buildRequests) {
+                    assert.equal(buildRequests[0].triggerable(), MockModels.triggerable);
+                    assert.equal(buildRequests[0].test(), MockModels.plt);
+                    assert.equal(buildRequests[0].platform(), MockModels.elCapitan);
+                }
+            }
+
+            return fetchPromise.then((testGroups) => {
+                assertTestGroups(testGroups);
+                return TestGroup.fetchForTask(1376);
+            }).then((testGroups) => {
+                assertTestGroups(testGroups);
+            });
+        });
+    });
+
     describe('_createModelsFromFetchedTestGroups', function () {
         it('should create test groups', function () {
             var groups = TestGroup._createModelsFromFetchedTestGroups(sampleTestGroup());