Perf dashboard erroneously rejects a build request to build owned components when...
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 May 2019 06:15:58 +0000 (06:15 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 May 2019 06:15:58 +0000 (06:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197928

Reviewed by Ryosuke Niwa.

Fix a bug that build type build request that only builds owned components failed to pass sanity check when there
is no repository accepts patch in triggerable repository group.
Add a sanity check to throw an error when build request type is build but there is no repository group template.

* tools/js/buildbot-syncer.js:
(BuildbotSyncer.prototype._propertiesForBuildRequest): Changed sanity check the always requires repository accepts patch when there is a build to make it also works for build request only builds owned components.
(BuildbotSyncer._parseRepositoryGroup): Added check for repository group templates not null  when build requiest type is build.
* unit-tests/buildbot-syncer-tests.js: Added unit tests for this change.
* unit-tests/resources/mock-v3-models.js: Added mock date for unit tests.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/tools/js/buildbot-syncer.js
Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js
Websites/perf.webkit.org/unit-tests/resources/mock-v3-models.js

index 6c174c9..ca67a08 100644 (file)
@@ -1,3 +1,20 @@
+2019-05-15  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Perf dashboard erroneously rejects a build request to build owned components when there are no patches.
+        https://bugs.webkit.org/show_bug.cgi?id=197928
+
+        Reviewed by Ryosuke Niwa.
+
+        Fix a bug that build type build request that only builds owned components failed to pass sanity check when there
+        is no repository accepts patch in triggerable repository group.
+        Add a sanity check to throw an error when build request type is build but there is no repository group template.
+
+        * tools/js/buildbot-syncer.js:
+        (BuildbotSyncer.prototype._propertiesForBuildRequest): Changed sanity check the always requires repository accepts patch when there is a build to make it also works for build request only builds owned components.
+        (BuildbotSyncer._parseRepositoryGroup): Added check for repository group templates not null  when build requiest type is build.
+        * unit-tests/buildbot-syncer-tests.js: Added unit tests for this change.
+        * unit-tests/resources/mock-v3-models.js: Added mock date for unit tests.
+
 2019-05-14  Dewei Zhu  <dewei_zhu@apple.com>
 
         Pruning old file logic should not stop after removing 10 files if there are more files to be removed.
index 02256b1..b912728 100644 (file)
@@ -242,6 +242,7 @@ class BuildbotSyncer {
             properties[propertyName] = propertiesTemplate[propertyName];
 
         const repositoryGroupTemplate = buildRequest.isBuild() ? repositoryGroupConfiguration.buildPropertiesTemplate : repositoryGroupConfiguration.testPropertiesTemplate;
+        assert(!buildRequest.isBuild() || repositoryGroupTemplate, 'Repository group template cannot be null for build type build request');
         for (let propertyName in repositoryGroupTemplate) {
             let value = repositoryGroupTemplate[propertyName];
             const type = typeof(value) == 'object' ? value.type : 'string';
@@ -464,10 +465,10 @@ class BuildbotSyncer {
 
         let buildPropertiesTemplate = null;
         if ('buildProperties' in group) {
-            assert(patchAcceptingRepositoryList.size, `Repository group "${name}" specifies the properties for building but does not accept any patches`);
             assert(group.acceptsRoots, `Repository group "${name}" specifies the properties for building but does not accept roots in testing`);
             const revisionRepositories = new Set;
             const patchRepositories = new Set;
+            let hasOwnedRevisions = false;
             buildPropertiesTemplate = this._parseRepositoryGroupPropertyTemplate('build', name, group.buildProperties, (type, value, condition) => {
                 assert(type != 'roots', `Repository group "${name}" specifies roots in the properties for building`);
                 let repository = null;
@@ -482,6 +483,7 @@ class BuildbotSyncer {
                     revisionRepositories.add(repository);
                     return {type, repository};
                 case 'ownedRevisions':
+                    hasOwnedRevisions = true;
                     return {type, ownerRepository: resolveRepository(value)};
                 case 'ifRepositorySet':
                     assert(condition, 'condition must set if type is "ifRepositorySet"');
@@ -489,6 +491,7 @@ class BuildbotSyncer {
                 }
                 return null;
             });
+            assert(patchAcceptingRepositoryList.size || hasOwnedRevisions, `Repository group "${name}" specifies the properties for building but does not accept any patches or need to build owned components`);
             for (const repository of patchRepositories)
                 assert(revisionRepositories.has(repository), `Repository group "${name}" specifies a patch for "${repository.name()}" but does not specify a revision`);
             assert.equal(patchAcceptingRepositoryList.size, patchRepositories.size,
index 66cafc6..9641e40 100644 (file)
@@ -1045,6 +1045,53 @@ describe('BuildbotSyncer', () => {
             assert.deepEqual(JSON.parse(properties['owned-commits']), {'Owner Repository': [{revision: 'owned-002', repository: 'Owned Repository', ownerRevision: 'owner-001'}]});
         });
 
+        it('should allow to build with an owned component even if no repository accepts a patch in the triggerable repository group', () => {
+            const config = sampleiOSConfig();
+            config.repositoryGroups['owner-repository'] = {
+                'repositories': {'Owner Repository': {}},
+                'testProperties': {
+                    'owner-repo': {'revision': 'Owner Repository'},
+                    'roots': {'roots': {}},
+                },
+                'buildProperties': {
+                    'owned-commits': {'ownedRevisions': 'Owner Repository'}
+                },
+                'acceptsRoots': true,
+            };
+            const syncers = BuildbotSyncer._loadConfig(RemoteAPI, config, builderNameToIDMap());
+            const owner111289 = CommitLog.ensureSingleton('111289', {'id': '111289', 'time': 1456931874000, 'repository': MockModels.ownerRepository, 'revision': 'owner-001'});
+            const owned111222 = CommitLog.ensureSingleton('111222', {'id': '111222', 'time': 1456932774000, 'repository': MockModels.ownedRepository, 'revision': 'owned-002'});
+            const commitSet = CommitSet.ensureSingleton('53246486', {customRoots: [], revisionItems: [{commit: owner111289}, {commit: owned111222, commitOwner: owner111289, requiresBuild: true}]});
+            const request = BuildRequest.ensureSingleton(`123123`, {'triggerable': MockModels.triggerable,
+                repositoryGroup: MockModels.ownerRepositoryGroup,
+                'commitSet': commitSet, 'status': 'pending', 'platform': MockModels.iphone, 'test': null, 'order': -1});
+
+            const properties = syncers[2]._propertiesForBuildRequest(request, [request]);
+            assert.deepEqual(JSON.parse(properties['owned-commits']), {'Owner Repository': [{revision: 'owned-002', repository: 'Owned Repository', ownerRevision: 'owner-001'}]});
+        });
+
+        it('should fail if build type build request does not have any build repository group template', () => {
+            const config = sampleiOSConfig();
+            config.repositoryGroups['owner-repository'] = {
+                'repositories': {'Owner Repository': {}},
+                'testProperties': {
+                    'owner-repo': {'revision': 'Owner Repository'},
+                    'roots': {'roots': {}},
+                },
+                'acceptsRoots': true,
+            };
+            const syncers = BuildbotSyncer._loadConfig(RemoteAPI, config, builderNameToIDMap());
+            const owner1 = CommitLog.ensureSingleton('111289', {'id': '111289', 'time': 1456931874000, 'repository': MockModels.ownerRepository, 'revision': 'owner-001'});
+            const owned2 = CommitLog.ensureSingleton('111222', {'id': '111222', 'time': 1456932774000, 'repository': MockModels.ownedRepository, 'revision': 'owned-002'});
+            const commitSet = CommitSet.ensureSingleton('53246486', {customRoots: [], revisionItems: [{commit: owner1}, {commit: owned2, commitOwner: owner1, requiresBuild: true}]});
+            const request = BuildRequest.ensureSingleton(`123123`, {'triggerable': MockModels.triggerable,
+                repositoryGroup: MockModels.ownerRepositoryGroup,
+                'commitSet': commitSet, 'status': 'pending', 'platform': MockModels.iphone, 'test': null, 'order': -1});
+
+            assert.throws(() => syncers[2]._propertiesForBuildRequest(request, [request]),
+                (error) => error.code === 'ERR_ASSERTION');
+        });
+
         it('should set the property for the build request id', () => {
             const syncers = BuildbotSyncer._loadConfig(RemoteAPI, sampleiOSConfig(), builderNameToIDMap());
             const request = createSampleBuildRequest(MockModels.iphone, MockModels.speedometer);
index b7c39a3..1215d56 100644 (file)
@@ -64,8 +64,13 @@ var MockModels = {
                 repositories: [{repository: MockModels.ios}, {repository: MockModels.webkit, acceptsPatch: true}, {repository: MockModels.ownerRepository}],
                 acceptsCustomRoots: true,
             });
+            MockModels.ownerRepositoryGroup = new TriggerableRepositoryGroup(35, {
+                name: 'owner-repository',
+                repositories: [{repository: MockModels.ownerRepository}],
+                acceptsCustomRoots: true
+            });
             MockModels.triggerable = new Triggerable(3, {name: 'build-webkit',
-                repositoryGroups: [MockModels.osRepositoryGroup, MockModels.svnRepositoryGroup, MockModels.gitRepositoryGroup, MockModels.svnRepositoryWithOwnedRepositoryGroup],
+                repositoryGroups: [MockModels.osRepositoryGroup, MockModels.svnRepositoryGroup, MockModels.gitRepositoryGroup, MockModels.svnRepositoryWithOwnedRepositoryGroup, MockModels.ownerRepositoryGroup],
                 configurations: [{test: MockModels.iPhonePLT, platform: MockModels.iphone}]});
 
         });