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 e7074171f0db43dad06c49034fe9d251013b9fb3..9087956dd88cd03151c9874e7ce8b611e303f185 100644 (file)
@@ -1,3 +1,40 @@
+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
index 32ad4312bdbc32575a9e6a18269dac06a14cc3e3..819b63c4fed77cceab324ed285a8a8a7ddc04728 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 63170f7a358498d06ebf4bce6534768a8c1e4536..f189298a47b458dcab7b7ba9f355feb9f4ec80ca 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 cb9f9a317310f63f7630539f5bddbae8b8bbaa95..b6db583fa7a4188e2d831f2080675af66fdee358 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 b6ea89e8826d28d27950d63f478524821b4c3436..d580d53331c29f073e2ee2f4fb4a53bcb99867e7 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 8941b64b50d85b49702673982adfc0ded3baea6c..f3e833c5440480c741219901e2dc0a90bf6bff4c 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());