Creating a custom analysis task after fetching all analysis tasks fail

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

Reviewed by Saam Barati.

The bug was caused by AnalysisTask._fetchSubset not fetching the analysis task when all analysis tasks
had previously been fetched (AnlaysisTask._fetchAllPromise is set) even when noCache is set to true.
Fixed it by ignornig _fetchAllPromise when noCache is set to true.

This patch also adds noCache argument to AnalysisTask.fetchById and reverts the inadvertent change in
r226836 to always set noCache to true in this function.

* public/v3/models/analysis-task.js:
(AnalysisTask.fetchById): Added noCache argument instead of always specifying true, and modernized the code.
(AnalysisTask._fetchSubset): Fixed the bug. See above description.
* public/v3/models/test-group.js:
(TestGroup.createWithTask): Set noCache to true when calling AnalysisTask.fetchById here.
* unit-tests/analysis-task-tests.js: Added test cases for AnalysisTask.fetchById, including a test
to make sure it doesn't fetch the specified analysis task when noCache is set to false and all analysis
tasks had previously been fetched for the aforementioned revert of the inadvertent change in r226836.
(sampleAnalysisTasks): Renamed from sampleAnalysisTasks as the result contains multiple analysis tasks.
* unit-tests/test-groups-tests.js: Added a test case for TestGroup.createWithTask


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

Websites/perf.webkit.org/ChangeLog

@@ -1,3 +1,28 @@
+2018-04-30  Ryosuke Niwa  <[email protected]>
+
+        Creating a custom analysis task after fetching all analysis tasks fail
+        https://bugs.webkit.org/show_bug.cgi?id=184641
+
+        Reviewed by Saam Barati.
+
+        The bug was caused by AnalysisTask._fetchSubset not fetching the analysis task when all analysis tasks
+        had previously been fetched (AnlaysisTask._fetchAllPromise is set) even when noCache is set to true.
+        Fixed it by ignornig _fetchAllPromise when noCache is set to true.
+
+        This patch also adds noCache argument to AnalysisTask.fetchById and reverts the inadvertent change in
+        r226836 to always set noCache to true in this function.
+
+        * public/v3/models/analysis-task.js:
+        (AnalysisTask.fetchById): Added noCache argument instead of always specifying true, and modernized the code.
+        (AnalysisTask._fetchSubset): Fixed the bug. See above description.
+        * public/v3/models/test-group.js:
+        (TestGroup.createWithTask): Set noCache to true when calling AnalysisTask.fetchById here.
+        * unit-tests/analysis-task-tests.js: Added test cases for AnalysisTask.fetchById, including a test
+        to make sure it doesn't fetch the specified analysis task when noCache is set to false and all analysis
+        tasks had previously been fetched for the aforementioned revert of the inadvertent change in r226836.
+        (sampleAnalysisTasks): Renamed from sampleAnalysisTasks as the result contains multiple analysis tasks.
+        * unit-tests/test-groups-tests.js: Added a test case for TestGroup.createWithTask
+
 2018-04-30  Ryosuke Niwa  <[email protected]>
 
         REGRESSION(r230960): Browser tests under TimeSeriesChart fetchMeasurementSets all fail

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

@@ -202,9 +202,9 @@ class AnalysisTask extends LabeledObject {
         ];
     }
 
-    static fetchById(id)
+    static fetchById(id, noCache)
     {
-        return this._fetchSubset({id: id}, true).then(function (data) { return AnalysisTask.findById(id); });
+        return this._fetchSubset({id: id}, noCache).then((data) => AnalysisTask.findById(id));
     }
 
     static fetchByBuildRequestId(id)
@@ -248,7 +248,7 @@ class AnalysisTask extends LabeledObject {
 
     static _fetchSubset(params, noCache)
     {
-        if (this._fetchAllPromise)
+        if (this._fetchAllPromise && !noCache)
             return this._fetchAllPromise;
         return this.cachedFetch('/api/analysis-tasks', params, noCache).then(this._constructAnalysisTasksFromRawData.bind(this));
     }

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

@@ -191,7 +191,7 @@ class TestGroup extends LabeledObject {
         const revisionSets = CommitSet.revisionSetsFromCommitSets(commitSets);
         const params = {taskName, name: groupName, platform: platform.id(), test: test.id(), repetitionCount, revisionSets};
         return PrivilegedAPI.sendRequest('create-test-group', params).then((data) => {
-            return AnalysisTask.fetchById(data['taskId']);
+            return AnalysisTask.fetchById(data['taskId'], true);
         }).then((task) => {
             return this.fetchForTask(task.id()).then(() => task);
         });

Websites/perf.webkit.org/unit-tests/analysis-task-tests.js

@@ -8,7 +8,7 @@ const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
 const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
 const NodePrivilegedAPI = require('../tools/js/privileged-api').PrivilegedAPI;
 
-function sampleAnalysisTask()
+function sampleAnalysisTasks()
 {
     return {
         'analysisTasks': [
@@ -123,13 +123,50 @@ function measurementCluster()
 
 describe('AnalysisTask', () => {
     MockModels.inject();
+
     function makeMockPoints(id, commitSet) {
         return {
             id,
             commitSet: () => commitSet
         }
     }
 
+    describe('fetchById', () => {
+        const requests = MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
+        it('should fetch the specified analysis task', () => {
+            let callCount = 0;
+            AnalysisTask.fetchById(1).then(() => { callCount++; });
+            assert.equal(callCount, 0);
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '/api/analysis-tasks?id=1');
+        });
+
+        it('should not fetch the specified analysis task if all analysis task had been fetched', async () => {
+            const fetchAll = AnalysisTask.fetchAll();
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '/api/analysis-tasks');
+            requests[0].resolve(sampleAnalysisTasks());
+
+            await fetchAll;
+            AnalysisTask.fetchById(sampleAnalysisTasks().analysisTasks[0].id);
+            assert.equal(requests.length, 1);
+        });
+
+        it('should fetch the specified analysis task if all analysis task had been fetched but noCache is set to true', async () => {
+            const fetchAll = AnalysisTask.fetchAll();
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '/api/analysis-tasks');
+            requests[0].resolve(sampleAnalysisTasks());
+
+            await fetchAll;
+            const taskId = sampleAnalysisTasks().analysisTasks[0].id;
+            AnalysisTask.fetchById(taskId, true);
+            assert.equal(requests.length, 2);
+            assert.equal(requests[1].url, `/api/analysis-tasks?id=${taskId}`);
+        });
+
+    })
+
     describe('fetchAll', () => {
         const requests = MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
         it('should request all analysis tasks', () => {
@@ -159,7 +196,7 @@ describe('AnalysisTask', () => {
             assert.equal(requests.length, 1);
             assert.equal(requests[0].url, '/api/analysis-tasks');
 
-            requests[0].resolve(sampleAnalysisTask());
+            requests[0].resolve(sampleAnalysisTasks());
 
             let anotherCallCount = 0;
             return promise.then(() => {
@@ -174,7 +211,7 @@ describe('AnalysisTask', () => {
 
         it('should create AnalysisTask objects', () => {
             const promise = AnalysisTask.fetchAll();
-            requests[0].resolve(sampleAnalysisTask());
+            requests[0].resolve(sampleAnalysisTasks());
 
             return promise.then(() => {
                 assert.equal(AnalysisTask.all().length, 1);
@@ -196,7 +233,7 @@ describe('AnalysisTask', () => {
 
         it('should create CommitLog objects for `causes`', () => {
             const promise = AnalysisTask.fetchAll();
-            requests[0].resolve(sampleAnalysisTask());
+            requests[0].resolve(sampleAnalysisTasks());
 
             return promise.then(() => {
                 assert.equal(AnalysisTask.all().length, 1);
@@ -218,7 +255,7 @@ describe('AnalysisTask', () => {
             assert.equal(adaptedMeasurement.commitSet().commitForRepository(MockModels.webkit).revision(), '196051');
 
             const promise = AnalysisTask.fetchAll();
-            requests[0].resolve(sampleAnalysisTask());
+            requests[0].resolve(sampleAnalysisTasks());
 
             return promise.then(() => {
                 assert.equal(AnalysisTask.all().length, 1);

Websites/perf.webkit.org/unit-tests/test-groups-tests.js

@@ -106,10 +106,9 @@ function sampleTestGroup() {
 
 describe('TestGroup', function () {
     MockModels.inject();
+    const requests = MockRemoteAPI.inject('https://perf.webkit.org', BrowserPrivilegedAPI);
 
     describe('fetchForTask', () => {
-        const requests = MockRemoteAPI.inject('https://perf.webkit.org', BrowserPrivilegedAPI);
-
         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);
@@ -343,4 +342,45 @@ describe('TestGroup', function () {
         });
     });
 
+    describe('createWithTask', () => {
+        it('should fetch the newly created analysis task even when all analysis tasks had previously been fetched', async () => {
+            const allAnalysisTasks = AnalysisTask.fetchAll();
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '/api/analysis-tasks');
+            requests[0].resolve({
+                'analysisTasks': [],
+                'bugs': [],
+                'commits': [],
+                'status': 'OK'
+            });
+
+            const set1 = new CustomCommitSet;
+            set1.setRevisionForRepository(MockModels.webkit, '191622');
+            set1.setRevisionForRepository(MockModels.sharedRepository, '80229');
+            const set2 = new CustomCommitSet;
+            set2.setRevisionForRepository(MockModels.webkit, '191623');
+            set2.setRevisionForRepository(MockModels.sharedRepository, '80229');
+
+            const promise = TestGroup.createWithTask('some-task', MockModels.somePlatform, MockModels.someTest, 'some-group', 4, [set1, set2]);
+            assert.equal(requests.length, 2);
+            assert.equal(requests[1].url, '/privileged-api/generate-csrf-token');
+            requests[1].resolve({
+                token: 'abc',
+                expiration: Date.now() + 100 * 1000,
+            });
+            await MockRemoteAPI.waitForRequest();
+            assert.equal(requests.length, 3);
+            assert.equal(requests[2].method, 'POST');
+            assert.equal(requests[2].url, '/privileged-api/create-test-group');
+            requests[2].resolve({
+                taskId: 123,
+                testGroupId: 456,
+            });
+            await MockRemoteAPI.waitForRequest();
+            assert.equal(requests.length, 4);
+            assert.equal(requests[3].method, 'GET');
+            assert.equal(requests[3].url, '/api/analysis-tasks?id=123');
+        });
+    })
+
 });