Make it possible to specify A/B testing revision with a partial hash
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Aug 2017 21:18:49 +0000 (21:18 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Aug 2017 21:18:49 +0000 (21:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176047

Rubber-stamped by Chris Dumez.

Added the support for specifying a partial hash in A/B testing instead of the full hash.

* public/include/commit-log-fetcher.php:
(CommitLogFetcher::find_commit_id_by_revision): Extracted from associate-commit.php.
* public/privileged-api/associate-commit.php:
(main):
* public/privileged-api/create-test-group.php:
(main): Use find_commit_id_by_revision here to support scheduling an A/B testing with a partial hash.
* server-tests/privileged-api-create-test-group-tests.js:
(createAnalysisTask): Make it possible to customize revision string in some test cases.
* server-tests/resources/test-server.js:
(TestServer.prototype._stopApache): Fixed the bug that cleanup step always fails whenever the test file
runs more than 8s.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/include/commit-log-fetcher.php
Websites/perf.webkit.org/public/privileged-api/associate-commit.php
Websites/perf.webkit.org/public/privileged-api/create-test-group.php
Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js
Websites/perf.webkit.org/server-tests/resources/test-server.js

index 1c2af46..e26e039 100644 (file)
@@ -1,3 +1,24 @@
+2017-08-29  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Make it possible to specify A/B testing revision with a partial hash
+        https://bugs.webkit.org/show_bug.cgi?id=176047
+
+        Rubber-stamped by Chris Dumez.
+
+        Added the support for specifying a partial hash in A/B testing instead of the full hash.
+
+        * public/include/commit-log-fetcher.php:
+        (CommitLogFetcher::find_commit_id_by_revision): Extracted from associate-commit.php.
+        * public/privileged-api/associate-commit.php:
+        (main): 
+        * public/privileged-api/create-test-group.php:
+        (main): Use find_commit_id_by_revision here to support scheduling an A/B testing with a partial hash.
+        * server-tests/privileged-api-create-test-group-tests.js:
+        (createAnalysisTask): Make it possible to customize revision string in some test cases.
+        * server-tests/resources/test-server.js:
+        (TestServer.prototype._stopApache): Fixed the bug that cleanup step always fails whenever the test file
+        runs more than 8s.
+
 2017-08-26  Ryosuke Niwa  <rniwa@webkit.org>
 
         Build fix. Creating trying a test group no longer updates the page.
index 11189c0..65d95e8 100644 (file)
@@ -6,6 +6,22 @@ class CommitLogFetcher {
         $this->db = $db;
     }
 
+    static function find_commit_id_by_revision($db, $repository_id, $revision)
+    {
+        if (!ctype_alnum($revision))
+            return NULL;
+        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND commit_revision = $2', array($repository_id, $revision));
+        if ($commit_rows)
+            return $commit_rows[0]['commit_id'];
+
+        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND commit_revision LIKE $2 LIMIT 2', array($repository_id, Database::escape_for_like($revision) . '%'));
+        if (!$commit_rows)
+            return NULL;
+        if (count($commit_rows) > 1)
+            return -1; // There are more than one matches.
+        return $commit_rows[0]['commit_id'];
+    }
+
     function fetch_for_tasks($task_id_list, $task_by_id)
     {
         $commit_rows = $this->db->query_and_fetch_all('SELECT task_commits.*, commits.*, committers.*
index 30ec116..08cf01b 100644 (file)
@@ -1,6 +1,7 @@
 <?php
 
 require_once('../include/json-header.php');
+require_once('../include/commit-log-fetcher.php');
 
 function main() {
     $data = ensure_privileged_api_data_and_token();
@@ -29,18 +30,15 @@ function main() {
         require_format('Kind', $kind, '/^(cause|fix)$/');
 
         $commit_info = array('repository' => $repository_id, 'revision' => $revision);
-        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND commit_revision LIKE $2 LIMIT 2',
-            array($repository_id, '%' . Database::escape_for_like($revision) . '%'));
-        if (count($commit_rows) > 1) {
+        $commit_id = CommitLogFetcher::find_commit_id_by_revision($db, $repository_id, $revision);
+        if ($commit_id < 0) {
             $db->rollback_transaction();
             exit_with_error('AmbiguousRevision', $commit_info);            
-        } else if (!$commit_rows) {
+        } else if (!$commit_id) {
             $db->rollback_transaction();
             exit_with_error('CommitNotFound', $commit_info);
         }
 
-        $commit_id = $commit_rows[0]['commit_id'];
-
         $association = array('task' => $analysis_task_id, 'commit' => $commit_id, 'is_fix' => Database::to_database_boolean($kind == 'fix'));
         $commit_id = $db->update_or_insert_row('task_commits', 'taskcommit',
             array('task' => $analysis_task_id, 'commit' => $commit_id), $association, 'commit');
index 4e57ad7..b151118 100644 (file)
@@ -1,6 +1,7 @@
 <?php
 
 require_once('../include/json-header.php');
+require_once('../include/commit-log-fetcher.php');
 require_once('../include/repository-group-finder.php');
 
 function main()
@@ -184,9 +185,10 @@ function commit_sets_from_revision_sets($db, $triggerable_id, $revision_set_list
             $revision = array_get($data, 'revision');
             if (!$revision)
                 exit_with_error('InvalidRevision', array('repository' => $repository_id, 'data' => $data));
-            $commit = $db->select_first_row('commits', 'commit',
-                array('repository' => intval($repository_id), 'revision' => $revision));
-            if (!$commit)
+            $commit_id = CommitLogFetcher::find_commit_id_by_revision($db, $repository_id, $revision);
+            if ($commit_id < 0)
+                exit_with_error('AmbigiousRevision', array('repository' => $repository_id, 'revision' => $revision));
+            if (!$commit_id)
                 exit_with_error('RevisionNotFound', array('repository' => $repository_id, 'revision' => $revision));
 
             $patch_file_id = array_get($data, 'patch');
@@ -196,7 +198,7 @@ function commit_sets_from_revision_sets($db, $triggerable_id, $revision_set_list
                 array_push($repository_with_patch, $repository_id);
             }
 
-            array_push($commit_set, array('commit' => $commit['commit_id'], 'patch_file' => $patch_file_id));
+            array_push($commit_set, array('commit' => $commit_id, 'patch_file' => $patch_file_id));
             array_push($repository_list, $repository_id);
         }
 
index 82eadca..98d7296 100644 (file)
@@ -8,14 +8,14 @@ const TemporaryFile = require('./resources/temporary-file.js').TemporaryFile;
 const addSlaveForReport = require('./resources/common-operations.js').addSlaveForReport;
 const prepareServerTest = require('./resources/common-operations.js').prepareServerTest;
 
-function createAnalysisTask(name)
+function createAnalysisTask(name, webkitRevisions = ["191622", "191623"])
 {
     const reportWithRevision = [{
         "buildNumber": "124",
         "buildTime": "2015-10-27T15:34:51",
         "revisions": {
             "WebKit": {
-                "revision": "191622",
+                "revision": webkitRevisions[0],
                 "timestamp": '2015-10-27T11:36:56.878473Z',
             },
             "macOS": {
@@ -44,7 +44,7 @@ function createAnalysisTask(name)
         "buildTime": "2015-10-27T17:27:41",
         "revisions": {
             "WebKit": {
-                "revision": "191623",
+                "revision": webkitRevisions[1],
                 "timestamp": '2015-10-27T16:38:10.768995Z',
             },
             "macOS": {
@@ -92,7 +92,7 @@ function createAnalysisTask(name)
     }).then((content) => content['taskId']);
 }
 
-function addTriggerableAndCreateTask(name)
+function addTriggerableAndCreateTask(name, webkitRevisions)
 {
     const report = {
         'slaveName': 'anotherSlave',
@@ -120,7 +120,7 @@ function addTriggerableAndCreateTask(name)
     }).then(() => {
         return TestServer.remoteAPI().postJSON('/api/update-triggerable/', report);
     }).then(() => {
-        return createAnalysisTask(name);
+        return createAnalysisTask(name, webkitRevisions);
     });
 }
 
@@ -265,7 +265,31 @@ describe('/privileged-api/create-test-group', function () {
     it('should return "RevisionNotFound" when revision sets contains an invalid revision', () => {
         return addTriggerableAndCreateTask('some task').then((taskId) => {
             const webkit = Repository.all().find((repository) => repository.name() == 'WebKit');
-            const revisionSets = [{[webkit.id()]: {revision: '191622'}}, {[webkit.id()]: {revision: '1'}}];
+            const revisionSets = [{[webkit.id()]: {revision: '191622'}}, {[webkit.id()]: {revision: '1a'}}];
+            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, revisionSets}).then((content) => {
+                assert(false, 'should never be reached');
+            }, (error) => {
+                assert.equal(error, 'RevisionNotFound');
+            });
+        });
+    });
+
+    it('should return "AmbigiousRevision" when there are multiple commits that match the specified revision string', () => {
+        return addTriggerableAndCreateTask('some task', ['2ceda45d3cd63cde58d0dbf5767714e03d902e43', '2c71a8ddc1f661663ccfd1a29c633ba57e879533']).then((taskId) => {
+            const webkit = Repository.all().find((repository) => repository.name() == 'WebKit');
+            const revisionSets = [{[webkit.id()]: {revision: '2ceda'}}, {[webkit.id()]: {revision: '2c'}}];
+            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, revisionSets}).then((content) => {
+                assert(false, 'should never be reached');
+            }, (error) => {
+                assert.equal(error, 'AmbigiousRevision');
+            });
+        });
+    });
+
+    it('should return "RevisionNotFound" when the end of a Git hash is specified', () => {
+        return addTriggerableAndCreateTask('some task', ['2ceda45d3cd63cde58d0dbf5767714e03d902e43', '5471a8ddc1f661663ccfd1a29c633ba57e879533']).then((taskId) => {
+            const webkit = Repository.all().find((repository) => repository.name() == 'WebKit');
+            const revisionSets = [{[webkit.id()]: {revision: '2ceda45d3cd63cde58d0dbf5767714e03d902e43'}}, {[webkit.id()]: {revision: '57e879533'}}];
             return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, revisionSets}).then((content) => {
                 assert(false, 'should never be reached');
             }, (error) => {
@@ -429,6 +453,47 @@ describe('/privileged-api/create-test-group', function () {
         });
     });
 
+    it('should create a test group using Git partial hashes', () => {
+        let webkit;
+        let macos;
+        return addTriggerableAndCreateTask('some task', ['2ceda45d3cd63cde58d0dbf5767714e03d902e43', '5471a8ddc1f661663ccfd1a29c633ba57e879533']).then((taskId) => {
+            webkit = Repository.findById(MockData.webkitRepositoryId());
+            macos = Repository.findById(MockData.macosRepositoryId());
+            const revisionSets = [{[macos.id()]: {revision: '15A284'}, [webkit.id()]: {revision: '2ceda'}},
+                {[macos.id()]: {revision: '15A284'}, [webkit.id()]: {revision: '5471a'}}];
+            const params = {name: 'test', task: taskId, repetitionCount: 2, revisionSets};
+            let insertedGroupId;
+            return PrivilegedAPI.sendRequest('create-test-group', params).then((content) => {
+                insertedGroupId = content['testGroupId'];
+                return TestGroup.fetchForTask(taskId, true);
+            }).then((testGroups) => {
+                assert.equal(testGroups.length, 1);
+                const group = testGroups[0];
+                assert.equal(group.id(), insertedGroupId);
+                assert.equal(group.repetitionCount(), 2);
+                const requests = group.buildRequests();
+                assert.equal(requests.length, 4);
+
+                const set0 = requests[0].commitSet();
+                const set1 = requests[1].commitSet();
+                assert.deepEqual(Repository.sortByNamePreferringOnesWithURL(set0.repositories()), [webkit, macos]);
+                assert.deepEqual(Repository.sortByNamePreferringOnesWithURL(set1.repositories()), [webkit, macos]);
+                assert.equal(set0.revisionForRepository(webkit), '2ceda45d3cd63cde58d0dbf5767714e03d902e43');
+                assert.equal(set0.revisionForRepository(macos), '15A284');
+                assert.equal(set1.revisionForRepository(webkit), '5471a8ddc1f661663ccfd1a29c633ba57e879533');
+                assert.equal(set1.revisionForRepository(macos), '15A284');
+
+                const repositoryGroup0 = requests[0].repositoryGroup();
+                assert.equal(repositoryGroup0.name(), 'system-and-webkit');
+                assert.equal(repositoryGroup0, requests[2].repositoryGroup());
+                const repositoryGroup1 = requests[1].repositoryGroup();
+                assert.equal(repositoryGroup1, repositoryGroup0);
+                assert(repositoryGroup0.accepts(set0));
+                assert(repositoryGroup0.accepts(set1));
+            });
+        });
+    });
+
     it('should create a test group using different repository groups if needed', () => {
         let webkit;
         let macos;
index b0d3f68..87bee45 100644 (file)
@@ -215,6 +215,7 @@ class TestServer {
 
         childProcess.execFileSync('kill', ['-TERM', pid]);
 
+        this._pidWaitStart = Date.now();
         return new Promise(this._waitForPid.bind(this, false));
     }