Attachment 339134 Details for Bug 185125 – Fixes the bug

[patch] Fixes the bug

bug-185125-20180430121013.patch (text/plain), 14.15 KB, created by Ryosuke Niwa on 2018-04-30 12:10:15 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-04-30 12:10:15 PDT

Size: 14.15 KB

patch

obsolete

>Subversion Revision: 231153
>diff --git a/Websites/perf.webkit.org/ChangeLog b/Websites/perf.webkit.org/ChangeLog
>index b399bcb18c52f1bd37319eabc85e853c23806c78..064036b39d3168956850c60c6e30a193a05afbee 100644
>--- a/Websites/perf.webkit.org/ChangeLog
>+++ b/Websites/perf.webkit.org/ChangeLog
>@@ -1,3 +1,25 @@
>+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
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The bug was caused by mock-remote-api.js always loading PrivilegedAPI using require, which doesn't work in a browser.
>+        Fixed the bug by explicitly requiring the right kind of PrivilegedAPI in each unit test instead.
>+
>+        * unit-tests/analysis-task-tests.js:
>+        * unit-tests/buildbot-syncer-tests.js:
>+        * unit-tests/commit-log-tests.js:
>+        * unit-tests/commit-set-range-bisector-tests.js:
>+        * unit-tests/commit-set-tests.js:
>+        * unit-tests/measurement-set-tests.js:
>+        * unit-tests/privileged-api-tests.js:
>+        * unit-tests/resources/mock-remote-api.js:
>+        (MockRemoteAPI.inject): Take PrivilegedAPI instead of the type string. Also fixed a bug that _token wasn't unset
>+        after each unit test, and superfluous initializations of originalRemoteAPI and originalPrivilegedAPI.
>+        * unit-tests/test-groups-tests.js:
>+
> 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/unit-tests/analysis-task-tests.js b/Websites/perf.webkit.org/unit-tests/analysis-task-tests.js
>index 5d499ab3597f8155c257b49d1bae9b8a580f6515..7fc154200157023ed936a279c58d965244677b07 100644
>--- a/Websites/perf.webkit.org/unit-tests/analysis-task-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/analysis-task-tests.js
>@@ -3,8 +3,10 @@
> const assert = require('assert');
> 
> require('../tools/js/v3-models.js');
>-let MockModels = require('./resources/mock-v3-models.js').MockModels;
>-let MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
>+const MockModels = require('./resources/mock-v3-models.js').MockModels;
>+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()
> {
>@@ -129,7 +131,7 @@ describe('AnalysisTask', () => {
>     }
> 
>     describe('fetchAll', () => {
>-        const requests = MockRemoteAPI.inject();
>+        const requests = MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
>         it('should request all analysis tasks', () => {
>             let callCount = 0;
>             AnalysisTask.fetchAll().then(() => { callCount++; });
>@@ -245,7 +247,7 @@ describe('AnalysisTask', () => {
>     }
> 
>     describe('create with browser privilege api', () => {
>-        const requests = MockRemoteAPI.inject();
>+        const requests = MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
> 
>         it('should create analysis task with confirming repetition count zero as default with browser privilege api', async () => {
>             const [startPoint, endPoint] = mockStartAndEndPoints();
>@@ -287,7 +289,7 @@ describe('AnalysisTask', () => {
>     });
> 
>     describe('create with node privilege api', () => {
>-        const requests = MockRemoteAPI.inject(null, 'node');
>+        const requests = MockRemoteAPI.inject(null, NodePrivilegedAPI);
>         beforeEach(() => {
>             PrivilegedAPI.configure('worker', 'password');
>         });
>diff --git a/Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js b/Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js
>index 2e2c3b4498ee41bff4fff177a87ad955f282736d..66cafc6b45cc06a98dd1cffdb4c326af4ac9ef8a 100644
>--- a/Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js
>@@ -3,11 +3,13 @@
> let assert = require('assert');
> 
> require('../tools/js/v3-models.js');
>-let MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
>-let MockModels = require('./resources/mock-v3-models.js').MockModels;
>+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
> 
>-let BuildbotBuildEntry = require('../tools/js/buildbot-syncer.js').BuildbotBuildEntry;
>-let BuildbotSyncer = require('../tools/js/buildbot-syncer.js').BuildbotSyncer;
>+const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
>+const MockModels = require('./resources/mock-v3-models.js').MockModels;
>+
>+const BuildbotBuildEntry = require('../tools/js/buildbot-syncer.js').BuildbotBuildEntry;
>+const BuildbotSyncer = require('../tools/js/buildbot-syncer.js').BuildbotSyncer;
> 
> function sampleiOSConfig()
> {
>@@ -331,7 +333,7 @@ function sampleFinishedBuild(buildRequestId, workerName, builderName)
> 
> describe('BuildbotSyncer', () => {
>     MockModels.inject();
>-    let requests = MockRemoteAPI.inject('http://build.webkit.org');
>+    const requests = MockRemoteAPI.inject('http://build.webkit.org', BrowserPrivilegedAPI);
> 
>     describe('_loadConfig', () => {
> 
>diff --git a/Websites/perf.webkit.org/unit-tests/commit-log-tests.js b/Websites/perf.webkit.org/unit-tests/commit-log-tests.js
>index cc2dbbf126ca1a8471fcd6f0897eb1423f601414..2ed1d802ba12fd9bf0276c25d8c82d2df3338b60 100644
>--- a/Websites/perf.webkit.org/unit-tests/commit-log-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/commit-log-tests.js
>@@ -3,6 +3,7 @@
> const assert = require('assert');
> 
> require('../tools/js/v3-models.js');
>+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
> const MockModels = require('./resources/mock-v3-models.js').MockModels;
> const MockRemoteAPI = require('../unit-tests/resources/mock-remote-api.js').MockRemoteAPI;
> 
>@@ -257,7 +258,7 @@ describe('CommitLog', function () {
> 
>     describe('fetchOwnedCommits', () => {
>         beforeEach(() => {
>-            MockRemoteAPI.inject();
>+            MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
>         });
> 
>         it('should reject if repository of the commit does not own other repositories', () => {
>diff --git a/Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js b/Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js
>index f10cc1c8e2370cd0bed9de6cfe0c88f9d3f4d7b7..b5ae2020e50e4617c491d05df6aa022093cf41d8 100644
>--- a/Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js
>@@ -3,6 +3,7 @@
> const assert = require('assert');
> 
> require('../tools/js/v3-models.js');
>+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
> const MockModels = require('./resources/mock-v3-models.js').MockModels;
> const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> 
>@@ -271,7 +272,7 @@ describe('CommitSetRangeBisector', () => {
> 
>     describe('commitSetClosestToMiddleOfAllCommits', () => {
>         MockModels.inject();
>-        const requests = MockRemoteAPI.inject();
>+        const requests = MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
> 
>         it('should return "null" if no common repository found', async () => {
>             const middleCommitSet = await CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits(commitSetsWithNoCommonRepository().splice(0, 2));
>diff --git a/Websites/perf.webkit.org/unit-tests/commit-set-tests.js b/Websites/perf.webkit.org/unit-tests/commit-set-tests.js
>index a8f345fa8e034629cd5b8d01efcd2da057b40ee9..7e39ab70cdedbd3374a45a7ba79cc6f06da24d06 100644
>--- a/Websites/perf.webkit.org/unit-tests/commit-set-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/commit-set-tests.js
>@@ -2,6 +2,7 @@
> 
> const assert = require('assert');
> require('../tools/js/v3-models.js');
>+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
> const MockModels = require('./resources/mock-v3-models.js').MockModels;
> const MockRemoteAPI = require('../unit-tests/resources/mock-remote-api.js').MockRemoteAPI;
> 
>@@ -161,7 +162,7 @@ function anotherCommitWithGitRevision()
> }
> 
> describe('CommitSet', () => {
>-    MockRemoteAPI.inject();
>+    MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
>     MockModels.inject();
> 
>     function oneCommitSet()
>@@ -399,7 +400,7 @@ describe('CommitSet', () => {
> });
> 
> describe('IntermediateCommitSet', () => {
>-    MockRemoteAPI.inject();
>+    MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
>     MockModels.inject();
> 
>     describe('setCommitForRepository', () => {
>diff --git a/Websites/perf.webkit.org/unit-tests/measurement-set-tests.js b/Websites/perf.webkit.org/unit-tests/measurement-set-tests.js
>index 47393e087bc4e9b4cb2abfe2467dbc7710aab384..226e2526d5d014197aa94b6072a4dfdcf32a398b 100644
>--- a/Websites/perf.webkit.org/unit-tests/measurement-set-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/measurement-set-tests.js
>@@ -3,14 +3,14 @@
> const assert = require('assert');
> if (!assert.almostEqual)
>     assert.almostEqual = require('./resources/almost-equal.js');
>-
>-const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> require('../tools/js/v3-models.js');
>+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
>+const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> const MockModels = require('./resources/mock-v3-models.js').MockModels;
> 
> describe('MeasurementSet', () => {
>     MockModels.inject();
>-    let requests = MockRemoteAPI.inject();
>+    const requests = MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
> 
>     beforeEach(() => {
>         MeasurementSet._set = null;
>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..eb425df2626fff3e32aac89f393e3fbbd4ea1082 100644
>--- a/Websites/perf.webkit.org/unit-tests/privileged-api-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/privileged-api-tests.js
>@@ -1,12 +1,13 @@
> 'use strict';
> 
> const assert = require('assert');
>-
>-const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> require('../tools/js/v3-models.js');
>+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
>+const NodePrivilegedAPI = require('../tools/js/privileged-api').PrivilegedAPI;
>+const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> 
> describe('BrowserPrivilegedAPI', () => {
>-    let requests = MockRemoteAPI.inject();
>+    const requests = MockRemoteAPI.inject(null, BrowserPrivilegedAPI);
> 
>     beforeEach(() => {
>         PrivilegedAPI._token = null;
>@@ -169,7 +170,7 @@ describe('BrowserPrivilegedAPI', () => {
> });
> 
> describe('NodePrivilegedAPI', () => {
>-    let requests = MockRemoteAPI.inject(null, 'node');
>+    let requests = MockRemoteAPI.inject(null, NodePrivilegedAPI);
>     beforeEach(() => {
>         PrivilegedAPI.configure('slave_name', 'password');
>     });
>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..be43feebbba735cf8547c1b2685e36fc1d957660 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
>@@ -1,5 +1,3 @@
>-const BrowserPrivilegedAPI = require('../../public/v3/privileged-api.js').PrivilegedAPI;
>-const NodePrivilegedAPI = require('../../tools/js/privileged-api').PrivilegedAPI;
> 
> var MockRemoteAPI = {
>     url: function (path)
>@@ -60,27 +58,26 @@ var MockRemoteAPI = {
>         }
>         return this._waitingPromise;
>     },
>-    inject: function (urlPrefix, privilegedAPIType='browser')
>+    inject: function (urlPrefix, privilegedAPI)
>     {
>-        console.assert(privilegedAPIType === 'browser' || privilegedAPIType === 'node');
>-        let originalRemoteAPI = global.RemoteAPI;
>-        let originalPrivilegedAPI = global.PrivilegedAPI;
>-        const useNodePrivilegedAPI = privilegedAPIType === 'node';
>-        const PrivilegedAPI = useNodePrivilegedAPI ? NodePrivilegedAPI: BrowserPrivilegedAPI;
>+        let originalRemoteAPI;
>+        let originalPrivilegedAPI;
> 
>         beforeEach(() => {
>             MockRemoteAPI.reset(urlPrefix);
>             originalRemoteAPI = global.RemoteAPI;
>             global.RemoteAPI = MockRemoteAPI;
>             originalPrivilegedAPI = global.PrivilegedAPI;
>-            global.PrivilegedAPI = PrivilegedAPI;
>-            if (!useNodePrivilegedAPI)
>-                PrivilegedAPI._token = null;
>+            global.PrivilegedAPI = privilegedAPI;
>+            if (privilegedAPI._token)
>+                privilegedAPI._token = null;
>         });
> 
>         afterEach(() => {
>             global.RemoteAPI = originalRemoteAPI;
>             global.PrivilegedAPI = originalPrivilegedAPI;
>+            if (privilegedAPI._token)
>+                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..4891e3d2547b1fcdf2794e329d6e8afef11d5c2d 100644
>--- a/Websites/perf.webkit.org/unit-tests/test-groups-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/test-groups-tests.js
>@@ -1,8 +1,8 @@
> 'use strict';
> 
> const assert = require('assert');
>-
> require('../tools/js/v3-models.js');
>+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
> const MockModels = require('./resources/mock-v3-models.js').MockModels;
> const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
> 
>@@ -108,7 +108,7 @@ describe('TestGroup', function () {
>     MockModels.inject();
> 
>     describe('fetchForTask', () => {
>-        const requests = MockRemoteAPI.inject('https://perf.webkit.org');
>+        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);

Actions: View | Formatted Diff | Diff

Attachments on bug 185125: 339113 | 339132 | 339134