Creating a custom analysis task after fetching all analysis tasks fail
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 30 Apr 2018 23:56:21 +0000 (23:56 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 30 Apr 2018 23:56:21 +0000 (23:56 +0000)
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: https://svn.webkit.org/repository/webkit/trunk@231183 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/v3/models/analysis-task.js
Websites/perf.webkit.org/public/v3/models/test-group.js
Websites/perf.webkit.org/unit-tests/analysis-task-tests.js
Websites/perf.webkit.org/unit-tests/test-groups-tests.js

index f6a8e66..04442ec 100644 (file)
@@ -1,5 +1,30 @@
 2018-04-30  Ryosuke Niwa  <rniwa@webkit.org>
 
+        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  <rniwa@webkit.org>
+
         REGRESSION(r230960): Browser tests under TimeSeriesChart fetchMeasurementSets all fail
         https://bugs.webkit.org/show_bug.cgi?id=185125
 
index 83cc3cd..159a25e 100644 (file)
@@ -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));
     }
index e09f142..0317e05 100644 (file)
@@ -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);
         });
index 7fc1542..109aee8 100644 (file)
@@ -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,6 +123,7 @@ function measurementCluster()
 
 describe('AnalysisTask', () => {
     MockModels.inject();
+
     function makeMockPoints(id, commitSet) {
         return {
             id,
@@ -130,6 +131,42 @@ describe('AnalysisTask', () => {
         }
     }
 
+    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);
index 4891e3d..de59652 100644 (file)
@@ -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');
+        });
+    })
+
 });
\ No newline at end of file