Attachment 339101 Details for Bug 184641 – Fixes the bug

[patch] Fixes the bug

bug-184641-20180429164224.patch (text/plain), 13.37 KB, created by Ryosuke Niwa on 2018-04-29 16:42:25 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-04-29 16:42:25 PDT

Size: 13.37 KB

patch

obsolete

>Subversion Revision: 231153
>diff --git a/Websites/perf.webkit.org/ChangeLog b/Websites/perf.webkit.org/ChangeLog
>index b399bcb18c52f1bd37319eabc85e853c23806c78..9ed29897db579bda1f7633e7e86effcd4352b1c4 100644
>--- a/Websites/perf.webkit.org/ChangeLog
>+++ b/Websites/perf.webkit.org/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-04-15  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 NOBODY (OOPS!).
>+
>+        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/privileged-api-tests.js: No longer clears PrivilegedAPI._token here as this is now taken care
>+        by MockRemoteAPI.inject.
>+        * unit-tests/resources/mock-remote-api.js:
>+        (MockRemoteAPI.inject): Clear PrivilegedAPI._token automatically. Otherwise, PrivilegedAPI sometimes request
>+        a CSRF token and sometimes doesn't depending on the inter-test dependency and the time taken to run each test.
>+        * unit-tests/test-groups-tests.js: Added a test case for TestGroup.createWithTask
>+
> 2018-04-26  Dewei Zhu  <dewei_zhu@apple.com>
> 
>         Extend create-analysis-test API to be able to create with confirming test group.
>diff --git a/Websites/perf.webkit.org/public/v3/models/analysis-task.js b/Websites/perf.webkit.org/public/v3/models/analysis-task.js
>index 83cc3cd1e5b85f1f4a832321f0a966977c847fb4..159a25e7c056cc95975740f34298c8edc7026446 100644
>--- a/Websites/perf.webkit.org/public/v3/models/analysis-task.js
>+++ b/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));
>     }
>diff --git a/Websites/perf.webkit.org/public/v3/models/test-group.js b/Websites/perf.webkit.org/public/v3/models/test-group.js
>index e09f142fcdd5d5928deba28e37b6c9c1a35fa8c3..0317e05a979ea72cec06a0d820fd7e81ab4226c4 100644
>--- a/Websites/perf.webkit.org/public/v3/models/test-group.js
>+++ b/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);
>         });
>diff --git a/Websites/perf.webkit.org/unit-tests/analysis-task-tests.js b/Websites/perf.webkit.org/unit-tests/analysis-task-tests.js
>index 5d499ab3597f8155c257b49d1bae9b8a580f6515..d56704acc4277998c59ac0133e1c1e060070e860 100644
>--- a/Websites/perf.webkit.org/unit-tests/analysis-task-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/analysis-task-tests.js
>@@ -6,7 +6,7 @@ require('../tools/js/v3-models.js');
> let MockModels = require('./resources/mock-v3-models.js').MockModels;
> let MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> 
>-function sampleAnalysisTask()
>+function sampleAnalysisTasks()
> {
>     return {
>         'analysisTasks': [
>@@ -121,6 +121,8 @@ function measurementCluster()
> 
> describe('AnalysisTask', () => {
>     MockModels.inject();
>+    const requests = MockRemoteAPI.inject();
>+
>     function makeMockPoints(id, commitSet) {
>         return {
>             id,
>@@ -128,8 +130,42 @@ describe('AnalysisTask', () => {
>         }
>     }
> 
>+    describe('fetchById', () => {
>+        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();
>         it('should request all analysis tasks', () => {
>             let callCount = 0;
>             AnalysisTask.fetchAll().then(() => { callCount++; });
>@@ -157,7 +193,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(() => {
>@@ -172,7 +208,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);
>@@ -194,7 +230,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);
>@@ -216,7 +252,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);
>diff --git a/Websites/perf.webkit.org/unit-tests/privileged-api-tests.js b/Websites/perf.webkit.org/unit-tests/privileged-api-tests.js
>index 9da4b89827e707499f0e098586227bd6ee03b3a4..e0e01f85adc4d8515d1d88025bb64df932352c82 100644
>--- a/Websites/perf.webkit.org/unit-tests/privileged-api-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/privileged-api-tests.js
>@@ -6,11 +6,7 @@ const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> require('../tools/js/v3-models.js');
> 
> describe('BrowserPrivilegedAPI', () => {
>-    let requests = MockRemoteAPI.inject();
>-
>-    beforeEach(() => {
>-        PrivilegedAPI._token = null;
>-    });
>+    const requests = MockRemoteAPI.inject();
> 
>     describe('requestCSRFToken', () => {
>         it('should generate a new token', () => {
>diff --git a/Websites/perf.webkit.org/unit-tests/resources/mock-remote-api.js b/Websites/perf.webkit.org/unit-tests/resources/mock-remote-api.js
>index 6bc10fb355605e5a1e7727f7d14212b5c4118b19..9f15353aedc3ddbb9506d9fc4c54d781306a85c0 100644
>--- a/Websites/perf.webkit.org/unit-tests/resources/mock-remote-api.js
>+++ b/Websites/perf.webkit.org/unit-tests/resources/mock-remote-api.js
>@@ -63,8 +63,8 @@ var MockRemoteAPI = {
>     inject: function (urlPrefix, privilegedAPIType='browser')
>     {
>         console.assert(privilegedAPIType === 'browser' || privilegedAPIType === 'node');
>-        let originalRemoteAPI = global.RemoteAPI;
>-        let originalPrivilegedAPI = global.PrivilegedAPI;
>+        let originalRemoteAPI;
>+        let originalPrivilegedAPI;
>         const useNodePrivilegedAPI = privilegedAPIType === 'node';
>         const PrivilegedAPI = useNodePrivilegedAPI ? NodePrivilegedAPI: BrowserPrivilegedAPI;
> 
>@@ -81,6 +81,8 @@ var MockRemoteAPI = {
>         afterEach(() => {
>             global.RemoteAPI = originalRemoteAPI;
>             global.PrivilegedAPI = originalPrivilegedAPI;
>+            if (!useNodePrivilegedAPI)
>+                PrivilegedAPI._token = null;
>         });
> 
>         return MockRemoteAPI.requests;
>diff --git a/Websites/perf.webkit.org/unit-tests/test-groups-tests.js b/Websites/perf.webkit.org/unit-tests/test-groups-tests.js
>index f3e833c5440480c741219901e2dc0a90bf6bff4c..2a299239cedfe9dacbd87fcd4d99881426587151 100644
>--- a/Websites/perf.webkit.org/unit-tests/test-groups-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/test-groups-tests.js
>@@ -106,6 +106,7 @@ function sampleTestGroup() {
> 
> describe('TestGroup', function () {
>     MockModels.inject();
>+    const requests = MockRemoteAPI.inject();
> 
>     describe('fetchForTask', () => {
>         const requests = MockRemoteAPI.inject('https://perf.webkit.org');
>@@ -343,4 +344,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

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 184641: 337984 | 339101