Owner commit does not necessarily exist in the same commit set for an owned commit.
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Oct 2017 01:34:34 +0000 (01:34 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Oct 2017 01:34:34 +0000 (01:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=178763

Reviewed by Ryosuke Niwa.

Remove the check based on previous incorrect assumption.
Added unit tests to cover this change.

* public/privileged-api/create-test-group.php:
* server-tests/privileged-api-create-test-group-tests.js:
(return.addTriggerableAndCreateTask.string_appeared_here.then.id.taskId.id.then):

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/privileged-api/create-test-group.php
Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js

index f129ee7..872ade8 100644 (file)
@@ -1,3 +1,17 @@
+2017-10-24  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Owner commit does not necessarily exist in the same commit set for an owned commit.
+        https://bugs.webkit.org/show_bug.cgi?id=178763
+
+        Reviewed by Ryosuke Niwa.
+
+        Remove the check based on previous incorrect assumption.
+        Added unit tests to cover this change.
+
+        * public/privileged-api/create-test-group.php:
+        * server-tests/privileged-api-create-test-group-tests.js:
+        (return.addTriggerableAndCreateTask.string_appeared_here.then.id.taskId.id.then):
+
 2017-10-20  Dewei Zhu  <dewei_zhu@apple.com>
 
         Update perf dashboard upload logic to support uploading binaries from owned commits.
index 7e5ef95..fa55039 100644 (file)
@@ -170,8 +170,6 @@ function commit_sets_from_revision_sets($db, $triggerable_id, $revision_set_list
         $commit_set = array();
         $repository_list = array();
         $repository_with_patch = array();
-        $required_owner_commits = array();
-        $owner_commits_in_set = array();
         foreach ($revision_set as $repository_id => $data) {
             if ($repository_id == 'customRoots') {
                 $file_id_list = $data;
@@ -219,9 +217,7 @@ function commit_sets_from_revision_sets($db, $triggerable_id, $revision_set_list
                     exit_with_error('InvalidCommitOwnership', array('commitOwner' => $owner_commit['commit_id'], 'commitOwned' => $commit_id));
                 $repositories_require_build[$repository_id] =  TRUE;
                 $owner_commit_id = $owner_commit['commit_id'];
-                $required_owner_commits[$owner_commit_id] = $commit_id;
-            } else
-                $owner_commits_in_set[$commit_id] = TRUE;
+            }
 
             array_push($commit_set, array('commit' => $commit_id, 'patch_file' => $patch_file_id, 'requires_build' => FALSE, 'commit_owner' => $owner_commit_id));
 
@@ -241,10 +237,6 @@ function commit_sets_from_revision_sets($db, $triggerable_id, $revision_set_list
                 exit_with_error('PatchNotAccepted', array('repository' => $repository_id, 'repositoryGroup' => $repository_group_id));
         }
 
-        foreach($required_owner_commits as $required_owner_commit => $owned_commit) {
-            if (!array_get($owner_commits_in_set, $required_owner_commit, FALSE))
-                exit_with_error('CommitOwnerMustExistInCommitSet', array('owner_commit' => $required_owner_commit, 'owned_commit' => $owned_commit));
-        }
         array_push($commit_set_list, array('repository_group' => $repository_group_id, 'set' => $commit_set));
     }
 
index 9504421..2f3b297 100644 (file)
@@ -355,9 +355,9 @@ describe('/privileged-api/create-test-group', function () {
         return addTriggerableAndCreateTask('some task').then((id) => taskId = id).then(() => {
             const webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0];
             const macos = Repository.all().filter((repository) => repository.name() == 'macOS')[0];
-            const jsc = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore')[0];
-            const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191621'}},
-                {[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-9191', ownerRevision: '191622'}}];
+            const ownedJSC = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore' && repository.ownerId())[0];
+            const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [ownedJSC.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191621'}},
+                {[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [ownedJSC.id()]: {revision: 'owned-jsc-9191', ownerRevision: '191622'}}];
             return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, repetitionCount: 2, revisionSets});
         }).then(() => {
             assert(false, 'should never be reached');
@@ -371,9 +371,9 @@ describe('/privileged-api/create-test-group', function () {
         return addTriggerableAndCreateTask('some task').then((id) => taskId = id).then(() => {
             const webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0];
             const macos = Repository.all().filter((repository) => repository.name() == 'macOS')[0];
-            const jsc = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore')[0];
-            const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191622'}},
-                {[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-9191', ownerRevision: '191622'}}];
+            const ownedJSC = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore' && repository.ownerId())[0];
+            const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [ownedJSC.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191622'}},
+                {[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [ownedJSC.id()]: {revision: 'owned-jsc-9191', ownerRevision: '191622'}}];
             return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, repetitionCount: 2, revisionSets});
         }).then(() => {
             assert(false, 'should never be reached');
@@ -382,35 +382,94 @@ describe('/privileged-api/create-test-group', function () {
         });
     });
 
-    it('should return "CommitOwnerMustExistInCommitSet" when owner of a commit is missing in the commit set', () => {
+    it('should allow a commit set in which owner of a commit is not in the same set', () => {
         let taskId;
+        let groupId;
         return addTriggerableAndCreateTask('some task').then((id) => taskId = id).then(() => {
             const webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0];
             const macos = Repository.all().filter((repository) => repository.name() == 'macOS')[0];
-            const jsc = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore')[0];
-            const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191622'}},
-                {[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-9191', ownerRevision: '192736'}}];
-            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, repetitionCount: 2, revisionSets});
-        }).then(() => {
-            assert(false, 'should never be reached');
-        }, (error) => {
-            assert.equal(error, 'CommitOwnerMustExistInCommitSet');
+            const ownedJSC = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore' && repository.ownerId())[0];
+            const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [ownedJSC.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191622'}},
+                {[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [ownedJSC.id()]: {revision: 'owned-jsc-9191', ownerRevision: '192736'}}];
+            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, repetitionCount: 1, revisionSets});
+        }).then((content) => {
+            assert.equal(content['status'], 'OK');
+            groupId = content['testGroupId'];
+            return TestGroup.fetchForTask(taskId, true);
+        }).then((testGroups) => {
+            assert.equal(testGroups.length, 1);
+            const group = testGroups[0];
+            assert.equal(group.id(), groupId);
+            assert.equal(group.repetitionCount(), 1);
+            const requests = group.buildRequests();
+            assert.equal(requests.length, 4);
+            assert(requests[0].isBuild());
+            assert(!requests[0].isTest());
+            assert(requests[1].isBuild());
+            assert(!requests[1].isTest());
+            assert(!requests[2].isBuild());
+            assert(requests[2].isTest());
+            assert(!requests[3].isBuild());
+            assert(requests[3].isTest());
+
+            const macos = Repository.findById(MockData.macosRepositoryId());
+            const webkit = Repository.findById(MockData.webkitRepositoryId());
+            const ownedJSC = Repository.findById(MockData.ownedJSCRepositoryId());
+            const set0 = requests[0].commitSet();
+            const set1 = requests[1].commitSet();
+            assert.deepEqual(Repository.sortByNamePreferringOnesWithURL(set0.repositories()), [ownedJSC, webkit, macos]);
+            assert.deepEqual(Repository.sortByNamePreferringOnesWithURL(set1.repositories()), [ownedJSC, webkit, macos]);
+            assert.equal(set0.revisionForRepository(macos), '15A284');
+            assert.equal(set0.revisionForRepository(webkit), '191622');
+            assert.equal(set0.revisionForRepository(ownedJSC), 'owned-jsc-6161');
+            assert.equal(set1.revisionForRepository(macos), '15A284');
+            assert.equal(set1.revisionForRepository(webkit), '191622');
+            assert.equal(set1.revisionForRepository(ownedJSC), 'owned-jsc-9191');
         });
     });
 
-    it('should return "CommitOwnerMustExistInCommitSet" when repository of owner commit is not specified at all', () => {
+    it('should allow a commit set in which repository of owner commit is not specified at all', () => {
         let taskId;
+        let groupId;
         return addTriggerableAndCreateTask('some task').then((id) => taskId = id).then(() => {
             const webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0];
             const macos = Repository.all().filter((repository) => repository.name() == 'macOS')[0];
-            const jsc = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore')[0];
+            const jsc = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore' && repository.ownerId())[0];
             const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191622'}},
                 {[macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-9191', ownerRevision: '192736'}}];
-            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, repetitionCount: 2, revisionSets});
-        }).then(() => {
-            assert(false, 'should never be reached');
-        }, (error) => {
-            assert.equal(error, 'CommitOwnerMustExistInCommitSet');
+            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, repetitionCount: 1, revisionSets});
+        }).then((content) => {
+            assert.equal(content['status'], 'OK');
+            groupId = content['testGroupId'];
+            return TestGroup.fetchForTask(taskId, true);
+        }).then((testGroups) => {
+            assert.equal(testGroups.length, 1);
+            const group = testGroups[0];
+            assert.equal(group.id(), groupId);
+            assert.equal(group.repetitionCount(), 1);
+            const requests = group.buildRequests();
+            assert.equal(requests.length, 4);
+            assert(requests[0].isBuild());
+            assert(!requests[0].isTest());
+            assert(requests[1].isBuild());
+            assert(!requests[1].isTest());
+            assert(!requests[2].isBuild());
+            assert(requests[2].isTest());
+            assert(!requests[3].isBuild());
+            assert(requests[3].isTest());
+
+            const macos = Repository.findById(MockData.macosRepositoryId());
+            const webkit = Repository.findById(MockData.webkitRepositoryId());
+            const ownedJSC = Repository.findById(MockData.ownedJSCRepositoryId());
+            const set0 = requests[0].commitSet();
+            const set1 = requests[1].commitSet();
+            assert.deepEqual(Repository.sortByNamePreferringOnesWithURL(set0.repositories()), [ownedJSC, webkit, macos]);
+            assert.deepEqual(Repository.sortByNamePreferringOnesWithURL(set1.repositories()), [ownedJSC, macos]);
+            assert.equal(set0.revisionForRepository(macos), '15A284');
+            assert.equal(set0.revisionForRepository(webkit), '191622');
+            assert.equal(set0.revisionForRepository(ownedJSC), 'owned-jsc-6161');
+            assert.equal(set1.revisionForRepository(macos), '15A284');
+            assert.equal(set1.revisionForRepository(ownedJSC), 'owned-jsc-9191');
         });
     });