Attachment 339006 Details for Bug 184340 – Patch

[patch] Patch

bug-184340-20180427105840.patch (text/plain), 21.53 KB, created by dewei_zhu on 2018-04-27 10:58:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: dewei_zhu

Created: 2018-04-27 10:58:41 PDT

Size: 21.53 KB

patch

obsolete

>Subversion Revision: 231101
>diff --git a/Websites/perf.webkit.org/ChangeLog b/Websites/perf.webkit.org/ChangeLog
>index b399bcb18c52f1bd37319eabc85e853c23806c78..719bd662c13f5b944ee2e2c4e9b040e4f029da4b 100644
>--- a/Websites/perf.webkit.org/ChangeLog
>+++ b/Websites/perf.webkit.org/ChangeLog
>@@ -1,3 +1,32 @@
>+2018-04-27  Dewei Zhu  <dewei_zhu@apple.com>
>+
>+        Add a field to analysis_test_group table to indicate whether the its results has been sent to author.
>+        https://bugs.webkit.org/show_bug.cgi?id=184340
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added 'testgroup_notified_author' filed to 'analysis_test_group' table to indicate whether the author is notified.
>+        Updated /privileged-api/create-analysis-task.php to authenticate either by slave account or CSRF token.
>+        This field is default to false, however, for existing entries in the database, we should set it to true to avoid spaming.
>+        SQL query to update existing database is:
>+            'ALTER TABLE analysis_test_groups ADD COLUMN testgroup_notified_author boolean NOT NULL DEFAULT FALSE;'
>+            'UPDATE analysis_test_groups SET testgroup_notified_author = TRUE;'
>+
>+        * init-database.sql: Added 'testgroup_notified_author' field to 'analysis_test_group'.
>+        * public/api/test-groups.php: Updated API to return 'notifiedAuthor'.
>+        * public/privileged-api/update-test-group.php: Updated API to support setting 'testgroup_notified_author' field
>+        by authenticating through either slave account or csrf token.
>+        * public/v3/models/test-group.js:
>+        (TestGroup): Sets 'this._notifiedAuthor' in constructor.
>+        (TestGroup.prototype.updateSingleton): Updates 'this._notifiedAuthor' in this function.
>+        (TestGroup.prototype.notifiedAuthor): Returns wheter notification has been sent to author for this test group.
>+        (TestGroup.prototype.async.didNotifyAuthor): Provide a API to set 'testgroup_notified_author' to true in 'analysis_test_group' table.
>+        * server-tests/privileged-api-create-analysis-task-tests.js: Updated unit tests to use 'assertThrows' helper function.
>+        * server-tests/privileged-api-update-test-group-tests.js: Added unit test for 'update-test-group' API.
>+        * server-tests/resources/common-operations.js: Added a helper function that asserts an expected exception is raised.
>+        (async.assertThrows):
>+        * unit-tests/test-groups-tests.js: Added unit tests regarding this change.
>+
> 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/init-database.sql b/Websites/perf.webkit.org/init-database.sql
>index c4742e7b6dc3ea805b039e0d18775c52f2f7d067..8bd478665fd2fa94df184569d54421a5a48293eb 100644
>--- a/Websites/perf.webkit.org/init-database.sql
>+++ b/Websites/perf.webkit.org/init-database.sql
>@@ -279,6 +279,7 @@ CREATE TABLE analysis_test_groups (
>     testgroup_author varchar(256),
>     testgroup_created_at timestamp NOT NULL DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC'),
>     testgroup_hidden boolean NOT NULL DEFAULT FALSE,
>+    testgroup_notified_author boolean NOT NULL DEFAULT FALSE,
>     CONSTRAINT testgroup_name_must_be_unique_for_each_task UNIQUE(testgroup_task, testgroup_name));
> CREATE INDEX testgroup_task_index ON analysis_test_groups(testgroup_task);
> 
>diff --git a/Websites/perf.webkit.org/public/api/test-groups.php b/Websites/perf.webkit.org/public/api/test-groups.php
>index 42ddd3093509dd2f4ad630e84e4d3a05e7b0961c..ae0bf84c4d1a87f321accb72eea1622972324912 100644
>--- a/Websites/perf.webkit.org/public/api/test-groups.php
>+++ b/Websites/perf.webkit.org/public/api/test-groups.php
>@@ -65,6 +65,7 @@ function format_test_group($group_row) {
>         'author' => $group_row['testgroup_author'],
>         'createdAt' => strtotime($group_row['testgroup_created_at']) * 1000,
>         'hidden' => Database::is_true($group_row['testgroup_hidden']),
>+        'notifiedAuthor' => Database::is_true($group_row['testgroup_notified_author']),
>         'buildRequests' => array(),
>         'commitSets' => array(),
>     );
>diff --git a/Websites/perf.webkit.org/public/privileged-api/update-test-group.php b/Websites/perf.webkit.org/public/privileged-api/update-test-group.php
>index db1ed38392a57201cc076528e48a4968d20e9ee8..21237fd07c22c9b0f9bf242d577a68d6deab6733 100644
>--- a/Websites/perf.webkit.org/public/privileged-api/update-test-group.php
>+++ b/Websites/perf.webkit.org/public/privileged-api/update-test-group.php
>@@ -3,7 +3,8 @@
> require_once('../include/json-header.php');
> 
> function main() {
>-    $data = ensure_privileged_api_data_and_token();
>+    $db = connect();
>+    $data = ensure_privileged_api_data_and_token_or_slave($db);
> 
>     $test_group_id = array_get($data, 'group');
>     if (!$test_group_id)
>@@ -17,10 +18,11 @@ function main() {
>     if (array_key_exists('hidden', $data))
>         $values['hidden'] = Database::to_database_boolean($data['hidden']);
> 
>+    if (array_key_exists('notifiedAuthor', $data))
>+        $values['notified_author'] = Database::to_database_boolean($data['notifiedAuthor']);
>+
>     if (!$values)
>         exit_with_error('NothingToUpdate');
>-
>-    $db = connect();
>     $db->begin_transaction();
> 
>     if (!$db->update_row('analysis_test_groups', 'testgroup', array('id' => $test_group_id), $values)) {
>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..baa56e2f72951e1ef13850d358e616f7242a28a6 100644
>--- a/Websites/perf.webkit.org/public/v3/models/test-group.js
>+++ b/Websites/perf.webkit.org/public/v3/models/test-group.js
>@@ -9,6 +9,7 @@ class TestGroup extends LabeledObject {
>         this._authorName = object.author;
>         this._createdAt = new Date(object.createdAt);
>         this._isHidden = object.hidden;
>+        this._notifiedAuthor = object.notifiedAuthor;
>         this._buildRequests = [];
>         this._orderBuildRequestsLazily = new LazilyEvaluatedFunction((...buildRequests) => {
>             return buildRequests.sort((a, b) => a.order() - b.order());
>@@ -30,12 +31,14 @@ class TestGroup extends LabeledObject {
>         console.assert(this._platform == object.platform);
> 
>         this._isHidden = object.hidden;
>+        this._notifiedAuthor = object.notifiedAuthor;
>     }
> 
>     task() { return AnalysisTask.findById(this._taskId); }
>     createdAt() { return this._createdAt; }
>     isHidden() { return this._isHidden; }
>     buildRequests() { return this._buildRequests; }
>+    notifiedAuthor() { return this._notifiedAuthor; }
>     addBuildRequest(request)
>     {
>         this._buildRequests.push(request);
>@@ -185,6 +188,17 @@ class TestGroup extends LabeledObject {
>         });
>     }
> 
>+    async didNotifyAuthor()
>+    {
>+        const id = this.id();
>+        await PrivilegedAPI.sendRequest('update-test-group', {
>+            group: id,
>+            notifiedAuthor: true
>+        });
>+        const data = await TestGroup.cachedFetch(`/api/test-groups/${id}`, {}, true);
>+        return TestGroup._createModelsFromFetchedTestGroups(data);
>+    }
>+
>     static createWithTask(taskName, platform, test, groupName, repetitionCount, commitSets)
>     {
>         console.assert(commitSets.length == 2);
>diff --git a/Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js b/Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js
>index c7a6d1771f9b42c4a2fd07f672a23da24dc7cd89..68505c418ff1cfa78c226a7f61995d4415c90954 100644
>--- a/Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js
>+++ b/Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js
>@@ -6,6 +6,7 @@ let MockData = require('./resources/mock-data.js');
> let TestServer = require('./resources/test-server.js');
> const addBuilderForReport = require('./resources/common-operations.js').addBuilderForReport;
> const prepareServerTest = require('./resources/common-operations.js').prepareServerTest;
>+const assertThrows = require('./resources/common-operations.js').assertThrows;
> 
> const reportWithRevision = [{
>     "buildNumber": "124",
>@@ -353,17 +354,10 @@ describe('/privileged-api/create-analysis-task with browser privileged api', fun
>         const oneRevisionSet = {[webkitId]: {revision: '191622'}};
>         const anotherRevisionSet = {[webkitId]: {revision: '191623'}};
> 
>-        let raiseException = false;
>-
>-        try {
>-            await PrivilegedAPI.sendRequest('create-analysis-task', {name: 'confirm', repetitionCount: 1,
>+        await assertThrows('TriggerableNotFoundForTask', () =>
>+            PrivilegedAPI.sendRequest('create-analysis-task', {name: 'confirm', repetitionCount: 1,
>                 revisionSets: [oneRevisionSet, anotherRevisionSet],
>-                startRun: testRuns[0]['id'], endRun: testRuns[1]['id']});
>-        } catch (error) {
>-            assert.equal(error, 'TriggerableNotFoundForTask');
>-            raiseException = true;
>-        }
>-        assert.ok(raiseException);
>+                startRun: testRuns[0]['id'], endRun: testRuns[1]['id']}));
>     });
> 
>     it('should create an analysis task with no test group when repetition count is 0', async () => {
>@@ -513,17 +507,10 @@ describe('/privileged-api/create-analysis-task with node privileged api', functi
>         const oneRevisionSet = {[webkitId]: {revision: '191622'}};
>         const anotherRevisionSet = {[webkitId]: {revision: '191623'}};
> 
>-        let raiseException = false;
>-
>-        try {
>-            await PrivilegedAPI.sendRequest('create-analysis-task', {name: 'confirm', repetitionCount: 1,
>+        await assertThrows('SlaveNotFound', () =>
>+            PrivilegedAPI.sendRequest('create-analysis-task', {name: 'confirm', repetitionCount: 1,
>                 revisionSets: [oneRevisionSet, anotherRevisionSet],
>-                startRun: testRuns[0]['id'], endRun: testRuns[1]['id']});
>-        } catch (error) {
>-            assert.equal(error, 'SlaveNotFound');
>-            raiseException = true;
>-        }
>-        assert.ok(raiseException);
>+                startRun: testRuns[0]['id'], endRun: testRuns[1]['id']}));
> 
>         const allAnalysisTasks = await db.selectRows('analysis_tasks');
>         assert.ok(!allAnalysisTasks.length);
>diff --git a/Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js b/Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..3cc48d68ce4c61f63a5d30157c1db7fb0d267853
>--- /dev/null
>+++ b/Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js
>@@ -0,0 +1,165 @@
>+'use strict';
>+
>+const assert = require('assert');
>+
>+const MockData = require('./resources/mock-data.js');
>+const TestServer = require('./resources/test-server.js');
>+const addSlaveForReport = require('./resources/common-operations.js').addSlaveForReport;
>+const prepareServerTest = require('./resources/common-operations.js').prepareServerTest;
>+const assertThrows = require('./resources/common-operations.js').assertThrows;
>+
>+async function createAnalysisTask(name, webkitRevisions = ["191622", "191623"])
>+{
>+    const reportWithRevision = [{
>+        "buildNumber": "124",
>+        "buildTime": "2015-10-27T15:34:51",
>+        "revisions": {
>+            "WebKit": {
>+                "revision": webkitRevisions[0],
>+                "timestamp": '2015-10-27T11:36:56.878473Z',
>+            },
>+            "macOS": {
>+                "revision": "15A284",
>+            }
>+        },
>+        "builderName": "someBuilder",
>+        "slaveName": "someSlave",
>+        "slavePassword": "somePassword",
>+        "platform": "some platform",
>+        "tests": {
>+            "some test": {
>+                "metrics": {
>+                    "Time": ["Arithmetic"],
>+                },
>+                "tests": {
>+                    "test1": {
>+                        "metrics": {"Time": { "current": [11] }},
>+                    }
>+                }
>+            },
>+        }}];
>+
>+    const anotherReportWithRevision = [{
>+        "buildNumber": "125",
>+        "buildTime": "2015-10-27T17:27:41",
>+        "revisions": {
>+            "WebKit": {
>+                "revision": webkitRevisions[1],
>+                "timestamp": '2015-10-27T16:38:10.768995Z',
>+            },
>+            "macOS": {
>+                "revision": "15A284",
>+            }
>+        },
>+        "builderName": "someBuilder",
>+        "slaveName": "someSlave",
>+        "slavePassword": "somePassword",
>+        "platform": "some platform",
>+        "tests": {
>+            "some test": {
>+                "metrics": {
>+                    "Time": ["Arithmetic"],
>+                },
>+                "tests": {
>+                    "test1": {
>+                        "metrics": {"Time": { "current": [12] }},
>+                    }
>+                }
>+            },
>+        }}];
>+
>+    const db = TestServer.database();
>+    const remote = TestServer.remoteAPI();
>+    await addSlaveForReport(reportWithRevision[0]);
>+    await remote.postJSON('/api/report/', reportWithRevision);
>+    await remote.postJSON('/api/report/', anotherReportWithRevision);
>+    await Manifest.fetch();
>+    const test = Test.findByPath(['some test', 'test1']);
>+    const platform = Platform.findByName('some platform');
>+    const configRow = await db.selectFirstRow('test_configurations', {metric: test.metrics()[0].id(), platform: platform.id()});
>+    const testRuns = await db.selectRows('test_runs', {config: configRow['id']});
>+
>+    assert.equal(testRuns.length, 2);
>+    const content = await PrivilegedAPI.sendRequest('create-analysis-task', {
>+        name: name,
>+        startRun: testRuns[0]['id'],
>+        endRun: testRuns[1]['id'],
>+    });
>+    return content['taskId'];
>+}
>+
>+async function addTriggerableAndCreateTask(name, webkitRevisions)
>+{
>+    const report = {
>+        'slaveName': 'anotherSlave',
>+        'slavePassword': 'anotherPassword',
>+        'triggerable': 'build-webkit',
>+        'configurations': [
>+            {test: MockData.someTestId(), platform: MockData.somePlatformId()},
>+            {test: MockData.someTestId(), platform: MockData.otherPlatformId()},
>+        ],
>+        'repositoryGroups': [
>+            {name: 'os-only', acceptsRoot: true, repositories: [
>+                {repository: MockData.macosRepositoryId(), acceptsPatch: false},
>+            ]},
>+            {name: 'webkit-only', acceptsRoot: true, repositories: [
>+                {repository: MockData.webkitRepositoryId(), acceptsPatch: true},
>+            ]},
>+            {name: 'system-and-webkit', acceptsRoot: true, repositories: [
>+                {repository: MockData.macosRepositoryId(), acceptsPatch: false},
>+                {repository: MockData.webkitRepositoryId(), acceptsPatch: true}
>+            ]},
>+            {name: 'system-webkit-sjc', acceptsRoot: true, repositories: [
>+                {repository: MockData.macosRepositoryId(), acceptsPatch: false},
>+                {repository: MockData.jscRepositoryId(), acceptsPatch: false},
>+                {repository: MockData.webkitRepositoryId(), acceptsPatch: true}
>+            ]},
>+        ]
>+    };
>+    await MockData.addMockData(TestServer.database());
>+    await addSlaveForReport(report);
>+    await TestServer.remoteAPI().postJSON('/api/update-triggerable/', report);
>+    await createAnalysisTask(name, webkitRevisions);
>+}
>+
>+describe('/privileged-api/update-test-group', function(){
>+    prepareServerTest(this, 'node');
>+    beforeEach(() => {
>+        PrivilegedAPI.configure('test', 'password');
>+    });
>+
>+    it('should throw "SlaveNotFound" if invalid slave name and password combination is provided', async () => {
>+        await addTriggerableAndCreateTask('some task');
>+
>+        PrivilegedAPI.configure('test', 'wrongpassword');
>+        const webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0];
>+        const revisionSets = [{[webkit.id()]: {revision: '191622'}}, {[webkit.id()]: {revision: '191623'}}];
>+
>+        await assertThrows('SlaveNotFound', () =>
>+            PrivilegedAPI.sendRequest('create-test-group', {name: 'test', taskName: 'other task',
>+                platform: MockData.somePlatformId(), test: MockData.someTestId(), revisionSets}));
>+    });
>+
>+    it('should be able to update notified author flag', async () => {
>+        await addTriggerableAndCreateTask('some task');
>+        const webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0];
>+        const revisionSets = [{[webkit.id()]: {revision: '191622'}}, {[webkit.id()]: {revision: '191623'}}];
>+        let result = await PrivilegedAPI.sendRequest('create-test-group',
>+                {name: 'test', taskName: 'other task', platform: MockData.somePlatformId(), test: MockData.someTestId(), revisionSets});
>+        const insertedGroupId = result['testGroupId'];
>+        result = await Promise.all([AnalysisTask.fetchById(result['taskId']), TestGroup.fetchForTask(result['taskId'], true)]);
>+
>+        const [analysisTask, testGroups] = result;
>+
>+        assert.equal(analysisTask.name(), 'other task');
>+
>+        assert.equal(testGroups.length, 1);
>+        const group = testGroups[0];
>+        assert.equal(group.id(), insertedGroupId);
>+        assert.equal(group.repetitionCount(), 1);
>+        assert.equal(group.notifiedAuthor(), false);
>+        await group.didNotifyAuthor();
>+        const updatedGroup = TestGroup.findById(group.id());
>+        assert.equal(updatedGroup.notifiedAuthor(), true);
>+    });
>+});
>\ No newline at end of file
>diff --git a/Websites/perf.webkit.org/server-tests/resources/common-operations.js b/Websites/perf.webkit.org/server-tests/resources/common-operations.js
>index e1463cafbd18f94e866d459ca05b5d6e3cd985a8..91d26238c83637ca4f68050a25780bafdf295f6e 100644
>--- a/Websites/perf.webkit.org/server-tests/resources/common-operations.js
>+++ b/Websites/perf.webkit.org/server-tests/resources/common-operations.js
>@@ -1,4 +1,5 @@
> 
>+const assert = require('assert');
> const crypto = require('crypto');
> 
> function addBuilderForReport(report)
>@@ -49,9 +50,22 @@ function submitReport(report)
>     });
> }
> 
>+async function assertThrows(expectedError, testFunction)
>+{
>+    let thrownException = false;
>+    try {
>+        await testFunction()
>+    } catch(error) {
>+        thrownException = true;
>+        assert.equal(error, expectedError);
>+    }
>+    assert.ok(thrownException);
>+}
>+
> if (typeof module != 'undefined') {
>     module.exports.addBuilderForReport = addBuilderForReport;
>     module.exports.addSlaveForReport = addSlaveForReport;
>     module.exports.prepareServerTest = prepareServerTest;
>     module.exports.submitReport = submitReport;
>+    module.exports.assertThrows = assertThrows;
> }
>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..ed353ac9804586d01b1974f549db1b8f658f8d53 100644
>--- a/Websites/perf.webkit.org/unit-tests/test-groups-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/test-groups-tests.js
>@@ -16,6 +16,7 @@ function sampleTestGroup() {
>             "author": "rniwa",
>             "createdAt": 1458688514000,
>             "hidden": false,
>+            "notifiedAuthor": false,
>             "buildRequests": ["16985", "16986", "16987", "16988", "16989", "16990", "16991", "16992"],
>             "commitSets": ["4255", "4256"],
>         }],
>@@ -138,6 +139,41 @@ describe('TestGroup', function () {
>         });
>     });
> 
>+    describe('didNotifyAuthor', () => {
>+        const requests = MockRemoteAPI.inject('https://perf.webkit.org', 'node');
>+        beforeEach(() => {
>+            PrivilegedAPI.configure('test', 'password');
>+        });
>+
>+        it('should update notified author flag', async () => {
>+            const fetchPromise = TestGroup.fetchForTask(1376);
>+            requests[0].resolve(sampleTestGroup());
>+            let testGroups = await fetchPromise;
>+            assert(testGroups.length, 1);
>+            let testGroup = testGroups[0];
>+            assert.equal(testGroup.notifiedAuthor(), false);
>+
>+            const updatePromise = testGroup.didNotifyAuthor();
>+            assert.equal(requests.length, 2);
>+            assert.equal(requests[1].method, 'POST');
>+            assert.equal(requests[1].url, '/privileged-api/update-test-group');
>+            assert.deepEqual(requests[1].data, {group: '2128', notifiedAuthor: true, slaveName: 'test', slavePassword: 'password'});
>+            requests[1].resolve();
>+
>+            await MockRemoteAPI.waitForRequest();
>+            assert.equal(requests.length, 3);
>+            assert.equal(requests[2].method, 'GET');
>+            assert.equal(requests[2].url, '/api/test-groups/2128');
>+            const updatedTestGroup = sampleTestGroup();
>+            updatedTestGroup.testGroups[0].notifiedAuthor = true;
>+            requests[2].resolve(updatedTestGroup);
>+
>+            testGroups = await updatePromise;
>+            testGroup = testGroups[0];
>+            assert.equal(testGroup.notifiedAuthor(), true);
>+        });
>+    });
>+
>     describe('_createModelsFromFetchedTestGroups', function () {
>         it('should create test groups', function () {
>             var groups = TestGroup._createModelsFromFetchedTestGroups(sampleTestGroup());
>@@ -148,6 +184,7 @@ describe('TestGroup', function () {
>             assert.equal(group.id(), 2128);
>             assert.ok(group.createdAt() instanceof Date);
>             assert.equal(group.isHidden(), false);
>+            assert.equal(group.notifiedAuthor(), false);
>             assert.equal(+group.createdAt(), 1458688514000);
>             assert.equal(group.repetitionCount(), sampleTestGroup()['buildRequests'].length / 2);
>             assert.ok(group.hasPending());

Actions: View | Formatted Diff | Diff

Attachments on bug 184340: 337299 | 339006 | 341254 | 341502 | 341829 | 342008 | 342101