Range bisector should check the commits for repositories without change in specified...
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 May 2018 20:58:27 +0000 (20:58 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 May 2018 20:58:27 +0000 (20:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185269

Reviewed by Ryosuke Niwa.

For repositories without a change in the specified range, we still need to use them to filter commit
sets. Before this change, code does not apply filtering by those repositories against commit set. As
a result, commit sets with different commits for those repositories may be chosen as bisecting commit set.

* public/v3/commit-set-range-bisector.js: Updated the logic to verify range for repositories without
change in range.
(CommitSetRangeBisector.async.commitSetClosestToMiddleOfAllCommits):
* unit-tests/commit-set-range-bisector-tests.js: Added a unit test to guard against this change.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js
Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js

index f88ea15..36e18f6 100644 (file)
@@ -1,3 +1,19 @@
+2018-05-03  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Range bisector should check the commits for repositories without change in specified range.
+        https://bugs.webkit.org/show_bug.cgi?id=185269
+
+        Reviewed by Ryosuke Niwa.
+
+        For repositories without a change in the specified range, we still need to use them to filter commit
+        sets. Before this change, code does not apply filtering by those repositories against commit set. As
+        a result, commit sets with different commits for those repositories may be chosen as bisecting commit set.
+
+        * public/v3/commit-set-range-bisector.js: Updated the logic to verify range for repositories without
+        change in range.
+        (CommitSetRangeBisector.async.commitSetClosestToMiddleOfAllCommits):
+        * unit-tests/commit-set-range-bisector-tests.js: Added a unit test to guard against this change.
+
 2018-05-02  Dewei Zhu  <dewei_zhu@apple.com>
 
         Create analysis task should sync analysis task status after creation.
index e478474..c99bdc3 100644 (file)
@@ -16,18 +16,20 @@ class CommitSetRangeBisector {
         const commitRangeByRepository = new Map;
         const indexForAllTimelessCommitsWithOrderByRepository = new Map;
         const allCommitsWithCommitTime = [];
-        const topLevelRepositoriesWithCommitChange = firstCommitSet.topLevelRepositories()
+        const topLevelRepositoriesWithOrderedCommits = firstCommitSet.topLevelRepositories()
             .filter((repository) => {
                 const firstCommit = firstCommitSet.commitForRepository(repository);
                 const secondCommit = secondCommitSet.commitForRepository(repository);
-                return firstCommit !== secondCommit && CommitLog.hasOrdering(firstCommit, secondCommit);
+                return CommitLog.hasOrdering(firstCommit, secondCommit);
             });
 
-        await Promise.all(topLevelRepositoriesWithCommitChange.map(async (repository) => {
+        await Promise.all(topLevelRepositoriesWithOrderedCommits.map(async (repository) => {
             const firstCommit = firstCommitSet.commitForRepository(repository);
             const secondCommit = secondCommitSet.commitForRepository(repository);
             const [startCommit, endCommit] = CommitLog.orderTwoCommits(firstCommit, secondCommit);
-            const commits = await CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision());
+
+            const commits = startCommit === endCommit ? [startCommit] : await CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision());
+
             if (startCommit.hasCommitTime()) {
                 allCommitsWithCommitTime.push(startCommit, ...commits);
                 commitRangeByRepository.set(repository, (commit) =>
index b5ae202..bdae30e 100644 (file)
@@ -140,6 +140,54 @@ describe('CommitSetRangeBisector', () => {
         ];
     }
 
+    function commitSetsWitCommitRollback()
+    {
+        return [
+            CommitSet.ensureSingleton(11, {
+                revisionItems: [
+                    { commit: makeCommit(1, MockModels.webkit, 'webkit-commit-1', 10), requiresBuild: false },
+                    { commit: makeCommit(11, MockModels.osx, 'osx-commit-1', 1), requiresBuild: false },
+                    { commit: makeCommit(201, MockModels.ownerRepository, 'owner-commit-1', 0, 1), requiresBuild: false }
+                ],
+                customRoots: []}),
+            CommitSet.ensureSingleton(12, {
+                revisionItems: [
+                    { commit: makeCommit(1, MockModels.webkit, 'webkit-commit-1', 10), requiresBuild: false },
+                    { commit: makeCommit(12, MockModels.osx, 'osx-commit-2', 21), requiresBuild: false },
+                    { commit: makeCommit(202, MockModels.ownerRepository, 'owner-commit-2', 0, 2), requiresBuild: false }
+                ],
+                customRoots: []}),
+            CommitSet.ensureSingleton(13, {
+                revisionItems: [
+                    { commit: makeCommit(2, MockModels.webkit, 'webkit-commit-2', 20), requiresBuild: false },
+                    { commit: makeCommit(12, MockModels.osx, 'osx-commit-2', 21), requiresBuild: false },
+                    { commit: makeCommit(202, MockModels.ownerRepository, 'owner-commit-2', 0, 2), requiresBuild: false }
+                ],
+                customRoots: []}),
+            CommitSet.ensureSingleton(14, {
+                revisionItems: [
+                    { commit: makeCommit(3, MockModels.webkit, 'webkit-commit-3', 30), requiresBuild: false },
+                    { commit: makeCommit(13, MockModels.osx, 'osx-commit-3', 31), requiresBuild: false },
+                    { commit: makeCommit(202, MockModels.ownerRepository, 'owner-commit-2', 0, 2), requiresBuild: false }
+                ],
+                customRoots: []}),
+            CommitSet.ensureSingleton(15, {
+                revisionItems: [
+                    { commit: makeCommit(3, MockModels.webkit, 'webkit-commit-3', 30), requiresBuild: false },
+                    { commit: makeCommit(13, MockModels.osx, 'osx-commit-3', 31), requiresBuild: false },
+                    { commit: makeCommit(203, MockModels.ownerRepository, 'owner-commit-3', 0, 3), requiresBuild: false }
+                ],
+                customRoots: []}),
+            CommitSet.ensureSingleton(17, {
+                revisionItems: [
+                    { commit: makeCommit(6, MockModels.webkit, 'webkit-commit-6', 60), requiresBuild: false },
+                    { commit: makeCommit(13, MockModels.osx, 'osx-commit-3', 31), requiresBuild: false },
+                    { commit: makeCommit(201, MockModels.ownerRepository, 'owner-commit-1', 0, 1), requiresBuild: false }
+                ],
+                customRoots: []}),
+        ];
+    }
+
     function commitSetsWithSomeHaveOwnedCommits()
     {
         return [
@@ -615,6 +663,79 @@ describe('CommitSetRangeBisector', () => {
             assert.equal(await promise, allCommitSets[3]);
         });
 
+        it('should still check the repository revision even the repository has no change in the range', async () => {
+            const allCommitSets = commitSetsWitCommitRollback();
+            const startCommitSet = allCommitSets[0];
+            const endCommitSet = allCommitSets[allCommitSets.length - 1];
+            const promise = CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([startCommitSet, endCommitSet], allCommitSets);
+            const webkitId = MockModels.webkit.id();
+            const osxId = MockModels.osx.id();
+
+            assert.equal(requests.length, 2);
+            assert.equal(requests[0].url, '/api/commits/9/?precedingRevision=osx-commit-1&lastRevision=osx-commit-3');
+            assert.equal(requests[1].url, '/api/commits/11/?precedingRevision=webkit-commit-1&lastRevision=webkit-commit-6');
+
+            requests[0].resolve({
+                'commits': [
+                    {
+                        repository: osxId,
+                        id: 12,
+                        revision: 'osx-commit-2',
+                        ownsCommits: false,
+                        time: 21
+                    },
+                    {
+                        repository: osxId,
+                        id: 13,
+                        revision: 'osx-commit-3',
+                        ownsCommits: false,
+                        time: 31
+                    }
+                ]
+            });
+
+            requests[1].resolve({
+                'commits': [
+                    {
+                        repository: webkitId,
+                        id: 2,
+                        revision: 'webkit-commit-2',
+                        ownsCommits: false,
+                        time: 20
+                    },
+                    {
+                        repository: webkitId,
+                        id: 3,
+                        revision: 'webkit-commit-3',
+                        ownsCommits: false,
+                        time: 30
+                    },
+                    {
+                        repository: webkitId,
+                        id: 4,
+                        revision: 'webkit-commit-4',
+                        ownsCommits: false,
+                        time: 40
+                    },
+                    {
+                        repository: webkitId,
+                        id: 5,
+                        revision: 'webkit-commit-5',
+                        ownsCommits: false,
+                        time: 50
+                    },
+                    {
+                        repository: webkitId,
+                        id: 6,
+                        revision: 'webkit-commit-6',
+                        ownsCommits: false,
+                        time: 60
+                    },
+                ]
+            });
+            assert.equal(await promise, null);
+        });
+
         it('should filter out commit set with owned commit', async () => {
             const allCommitSets = commitSetsWithSomeHaveOwnedCommits();
             const startCommitSet = allCommitSets[0];