Fix the bug that 'TestGroupResultsViewer' creates unnecessary rows.
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jan 2018 22:17:33 +0000 (22:17 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jan 2018 22:17:33 +0000 (22:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181967

Reviewed by Ryosuke Niwa.

Fixed a bug caused by a typo in CommitSet.equals, which makes it returns incorrect results for most
comparison between a CommitSet and a MeasurementCommitSet.

MeasurementCommitSet does not have full information for the commits, thus, it cannot build mappings
between root/patch/owner commit/requires build to repository. When querying whether a given repository
needs to be built, MeasurementCommitSet will return undefined. Due to 'undefined != false', this
equality check will fail. Making 'CommitSet.requiresBuildForRepository' defaults to 'false' would fix
this bug.

* public/v3/models/commit-set.js:
(CommitSet.prototype.requiresBuildForRepository): Make it return false when key does not exist
instead of 'undefined'.
(CommitSet.prototype.equals): Fixed the typo that causes the bug.
Use wrapped functions instead of querying the mapping directly.
* unit-tests/commit-set-tests.js: Added unit tests.

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

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

index 7dc9e00..98c774f 100644 (file)
@@ -1,3 +1,26 @@
+2018-01-22  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Fix the bug that 'TestGroupResultsViewer' creates unnecessary rows.
+        https://bugs.webkit.org/show_bug.cgi?id=181967
+
+        Reviewed by Ryosuke Niwa.
+
+        Fixed a bug caused by a typo in CommitSet.equals, which makes it returns incorrect results for most
+        comparison between a CommitSet and a MeasurementCommitSet.
+
+        MeasurementCommitSet does not have full information for the commits, thus, it cannot build mappings
+        between root/patch/owner commit/requires build to repository. When querying whether a given repository
+        needs to be built, MeasurementCommitSet will return undefined. Due to 'undefined != false', this
+        equality check will fail. Making 'CommitSet.requiresBuildForRepository' defaults to 'false' would fix
+        this bug.
+
+        * public/v3/models/commit-set.js:
+        (CommitSet.prototype.requiresBuildForRepository): Make it return false when key does not exist
+        instead of 'undefined'.
+        (CommitSet.prototype.equals): Fixed the typo that causes the bug.
+        Use wrapped functions instead of querying the mapping directly.
+        * unit-tests/commit-set-tests.js: Added unit tests.
+
 2018-01-18  Dewei Zhu  <dewei_zhu@apple.com>
 
         'run-test.py' script should make sure 'node_modules' directory exists before installing node packages.
index cf8a1cc..ee5f33a 100644 (file)
@@ -87,7 +87,7 @@ class CommitSet extends DataModelObject {
 
     patchForRepository(repository) { return this._repositoryToPatchMap.get(repository); }
     rootForRepository(repository) { return this._repositoryToRootMap.get(repository); }
-    requiresBuildForRepository(repository) { return this._repositoryRequiresBuildMap.get(repository); }
+    requiresBuildForRepository(repository) { return this._repositoryRequiresBuildMap.get(repository) || false; }
 
     // FIXME: This should return a Date object.
     latestCommitTime()
@@ -108,13 +108,13 @@ class CommitSet extends DataModelObject {
         for (const [repository, commit] of this._repositoryToCommitMap) {
             if (commit != other._repositoryToCommitMap.get(repository))
                 return false;
-            if (this._repositoryToPatchMap.get(repository) != other._repositoryToPatchMap.get(repository))
+            if (this.patchForRepository(repository) != other.patchForRepository(repository))
                 return false;
-            if (this._repositoryToRootMap.get(repository) != other._repositoryToRootMap.get(repository))
+            if (this.rootForRepository(repository) != other.rootForRepository(repository))
                 return false;
-            if (this._repositoryToCommitOwnerMap.get(repository) != other._repositoryToCommitMap.get(repository))
+            if (this.ownerCommitForRepository(repository) != other.ownerCommitForRepository(repository))
                 return false;
-            if (this._repositoryRequiresBuildMap.get(repository) != other._repositoryRequiresBuildMap.get(repository))
+            if (this.requiresBuildForRepository(repository) != other.requiresBuildForRepository(repository))
                 return false;
         }
         return CommitSet.areCustomRootsEqual(this._customRoots, other._customRoots);
index ebb7bcd..111c6a6 100644 (file)
@@ -7,16 +7,28 @@ const MockRemoteAPI = require('../unit-tests/resources/mock-remote-api.js').Mock
 
 function createPatch()
 {
-    return new UploadedFile(453, {'createdAt': new Date('2017-05-01T19:16:53Z'), 'filename': 'patch.dat', 'extension': '.dat', 'author': 'some user',
+    return UploadedFile.ensureSingleton(453, {'createdAt': new Date('2017-05-01T19:16:53Z'), 'filename': 'patch.dat', 'extension': '.dat', 'author': 'some user',
         size: 534637, sha256: '169463c8125e07c577110fe144ecd63942eb9472d438fc0014f474245e5df8a1'});
 }
 
+function createAnotherPatch()
+{
+    return UploadedFile.ensureSingleton(454, {'createdAt': new Date('2017-05-01T19:16:53Z'), 'filename': 'patch.dat', 'extension': '.dat', 'author': 'some user',
+        size: 534611, sha256: '169463c8125e07c577110fe144ecd63942eb9472d438fc0014f474245e5dfaaa'});
+}
+
 function createRoot()
 {
-    return new UploadedFile(456, {'createdAt': new Date('2017-05-01T21:03:27Z'), 'filename': 'root.dat', 'extension': '.dat', 'author': 'some user',
+    return UploadedFile.ensureSingleton(456, {'createdAt': new Date('2017-05-01T21:03:27Z'), 'filename': 'root.dat', 'extension': '.dat', 'author': 'some user',
         size: 16452234, sha256: '03eed7a8494ab8794c44b7d4308e55448fc56f4d6c175809ba968f78f656d58d'});
 }
 
+function createAnotherRoot()
+{
+    return UploadedFile.ensureSingleton(457, {'createdAt': new Date('2017-05-01T21:03:27Z'), 'filename': 'root.dat', 'extension': '.dat', 'author': 'some user',
+        size: 16452111, sha256: '03eed7a8494ab8794c44b7d4308e55448fc56f4d6c175809ba968f78f656dbbb'});
+}
+
 function customCommitSetWithoutOwnedCommit()
 {
     const customCommitSet = new CustomCommitSet;
@@ -54,7 +66,7 @@ function customCommitSetWithOwnedRepositoryHasSameNameAsNotOwnedRepository()
 
 function ownerCommit()
 {
-    return new CommitLog(5, {
+    return CommitLog.ensureSingleton(5, {
         repository: MockModels.ownerRepository,
         revision: 'owner-commit-0',
         ownsCommits: true,
@@ -64,7 +76,7 @@ function ownerCommit()
 
 function partialOwnerCommit()
 {
-    return new CommitLog(5, {
+    return CommitLog.ensureSingleton(5, {
         repository: MockModels.ownerRepository,
         revision: 'owner-commit-0',
         ownsCommits: null,
@@ -84,7 +96,7 @@ function ownedCommit()
 
 function webkitCommit()
 {
-    return new CommitLog(2017, {
+    return CommitLog.ensureSingleton(2017, {
         repository: MockModels.webkit,
         revision: 'webkit-commit-0',
         ownsCommits: false,
@@ -92,6 +104,103 @@ function webkitCommit()
     });
 }
 
+describe('CommitSet', () => {
+    MockRemoteAPI.inject();
+    MockModels.inject();
+
+    function oneCommitSet()
+    {
+        return CommitSet.ensureSingleton(1, {
+            revisionItems: [{ commit: webkitCommit(), requiresBuild: false }],
+            customRoots: []
+        });
+    }
+
+    function anotherCommitSet()
+    {
+        return CommitSet.ensureSingleton(2, {
+            revisionItems: [{ commit: webkitCommit(), requiresBuild: false }],
+            customRoots: []
+        });
+    }
+
+    function commitSetWithPatch()
+    {
+        return CommitSet.ensureSingleton(3, {
+            revisionItems: [{ commit: webkitCommit(), requiresBuild: false, patch: createPatch() }],
+            customRoots: []
+        });
+    }
+
+    function commitSetWithAnotherPatch()
+    {
+        return CommitSet.ensureSingleton(4, {
+            revisionItems: [{ commit: webkitCommit(), requiresBuild: false, patch: createAnotherPatch() }],
+            customRoots: []
+        });
+    }
+
+    function commitSetWithRoot()
+    {
+        return CommitSet.ensureSingleton(5, {
+            revisionItems: [{ commit: webkitCommit(), requiresBuild: false }],
+            customRoots: [createRoot()]
+        });
+    }
+
+    function anotherCommitSetWithRoot()
+    {
+        return CommitSet.ensureSingleton(6, {
+            revisionItems: [{ commit: webkitCommit(), requiresBuild: false }],
+            customRoots: [createAnotherRoot()]
+        });
+    }
+
+    function oneMeasurementCommitSet()
+    {
+        return MeasurementCommitSet.ensureSingleton(1, [
+            [2017, 11, 'webkit-commit-0', 1456932773000]
+        ]);
+    }
+
+    describe('equals', () => {
+        it('should return false if patches for same repository are different', () => {
+            assert(!commitSetWithPatch().equals(commitSetWithAnotherPatch()));
+            assert(!commitSetWithAnotherPatch().equals(commitSetWithPatch()));
+        });
+
+        it('should return false if patch is only specified in one commit set', () => {
+            assert(!oneCommitSet().equals(commitSetWithPatch()));
+            assert(!commitSetWithPatch().equals(oneCommitSet()));
+        });
+
+        it('should return false if roots for same repository are different', () => {
+            assert(!commitSetWithRoot().equals(anotherCommitSetWithRoot()));
+            assert(!anotherCommitSetWithRoot().equals(commitSetWithRoot()));
+        });
+
+        it('should return false if root is only specified in one commit set', () => {
+            assert(!commitSetWithRoot().equals(oneCommitSet()));
+            assert(!oneCommitSet().equals(commitSetWithRoot()));
+        });
+
+        it('should return true when comparing two identical commit set', () => {
+            assert(oneCommitSet().equals(oneCommitSet()));
+            assert(anotherCommitSet().equals(anotherCommitSet()));
+            assert(commitSetWithPatch().equals(commitSetWithPatch()));
+            assert(commitSetWithAnotherPatch().equals(commitSetWithAnotherPatch()));
+            assert(oneMeasurementCommitSet().equals(oneMeasurementCommitSet()));
+            assert(commitSetWithRoot().equals(commitSetWithRoot()));
+            assert(anotherCommitSetWithRoot().equals(anotherCommitSetWithRoot()));
+        });
+
+        it('should be able to compare between CommitSet and MeasurementCommitSet', () => {
+            assert(oneCommitSet().equals(oneMeasurementCommitSet()));
+            assert(oneMeasurementCommitSet().equals(oneCommitSet()));
+        });
+    })
+});
+
 describe('IntermediateCommitSet', () => {
     MockRemoteAPI.inject();
     MockModels.inject();