sync-builedbot.js fails to schedule the second request to test with a patch
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 30 May 2017 17:09:36 +0000 (17:09 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 30 May 2017 17:09:36 +0000 (17:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172701

Reviewed by Antti Koivisto.

The bug was caused by an assertion failure in BuildbotTriggerable's _pullBuildbotOnAllSyncers failing to
take into account that for a test group with a patch could be associated with two syncers, one to build
a patch and another to run tests. Fixed the bug by differentiating the two types of syncers by buildSyncer
and testSyncer per test group.

* server-tests/tools-sync-buildbot-integration-tests.js: Extended a test case so that it would hit the
assertion without the fix.

* tools/js/buildbot-triggerable.js:
(BuildbotTriggerable.prototype.syncOnce): Use the right kind of the syncer to schedule a build or a test.
(BuildbotTriggerable.prototype._pullBuildbotOnAllSyncers): Associate a given syncer based on the kind of
the build request it processed, and assert accordingly.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js
Websites/perf.webkit.org/tools/js/buildbot-triggerable.js

index 4424993..a09732f 100644 (file)
@@ -1,3 +1,23 @@
+2017-05-30  Ryosuke Niwa  <rniwa@webkit.org>
+
+        sync-builedbot.js fails to schedule the second request to test with a patch
+        https://bugs.webkit.org/show_bug.cgi?id=172701
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by an assertion failure in BuildbotTriggerable's _pullBuildbotOnAllSyncers failing to
+        take into account that for a test group with a patch could be associated with two syncers, one to build
+        a patch and another to run tests. Fixed the bug by differentiating the two types of syncers by buildSyncer
+        and testSyncer per test group.
+
+        * server-tests/tools-sync-buildbot-integration-tests.js: Extended a test case so that it would hit the
+        assertion without the fix.
+
+        * tools/js/buildbot-triggerable.js:
+        (BuildbotTriggerable.prototype.syncOnce): Use the right kind of the syncer to schedule a build or a test.
+        (BuildbotTriggerable.prototype._pullBuildbotOnAllSyncers): Associate a given syncer based on the kind of
+        the build request it processed, and assert accordingly.
+
 2017-05-29  Ryosuke Niwa  <rniwa@webkit.org>
 
         Fix UI glitches with a custom analysis test group with a patch
index 86768f5..773195e 100644 (file)
@@ -464,6 +464,23 @@ describe('sync-buildbot', function () {
             assert.deepEqual(requests[6].data, {'test': 'some-test', 'wk': '191622', 'build-request-id': '3', 'forcescheduler': 'force-ab-tests',
                 'roots': JSON.stringify([{url: firstRoot.url()}])});
             return MockRemoteAPI.waitForRequest();
+        }).then(() => {
+            assert.equal(requests.length, 10);
+            assertAndResolveRequest(requests[7], 'GET', '/json/builders/some%20tester/pendingBuilds', [
+                MockData.pendingBuild({builder: 'some tester', buildRequestId: 3}),
+            ]);
+            assertAndResolveRequest(requests[8], 'GET', '/json/builders/some%20builder/pendingBuilds', []);
+            assertAndResolveRequest(requests[9], 'GET', '/json/builders/other%20builder/pendingBuilds', []);
+            return MockRemoteAPI.waitForRequest();
+        }).then(() => {
+            assert.equal(requests.length, 13);
+            assertAndResolveRequest(requests[10], 'GET', '/json/builders/some%20tester/builds/?select=-1&select=-2', {});
+            assertAndResolveRequest(requests[11], 'GET', '/json/builders/some%20builder/builds/?select=-1&select=-2', {
+                [-1]: MockData.finishedBuild({builder: 'some builder', buildRequestId: 1}),
+                [-2]: MockData.finishedBuild({builder: 'some builder', buildRequestId: 2}),
+            });
+            assertAndResolveRequest(requests[12], 'GET', '/json/builders/other%20builder/builds/?select=-1&select=-2', {});
+            return syncPromise;
         });
     });
 
index 2150fc6..bfc6afa 100644 (file)
@@ -74,7 +74,9 @@ class BuildbotTriggerable {
                 const nextRequest = this._nextRequestInGroup(group, updates);
                 if (!validRequests.has(nextRequest))
                     continue;
-                const promise = this._scheduleRequestIfSlaveIsAvailable(nextRequest, group.requests, group.syncer, group.slaveName);
+                const promise = this._scheduleRequestIfSlaveIsAvailable(nextRequest, group.requests,
+                    nextRequest.isBuild() ? group.buildSyncer : group.testSyncer,
+                    nextRequest.isBuild() ? group.buildSlaveName : group.testSlaveName);
                 if (promise)
                     promistList.push(promise);
             }
@@ -141,11 +143,18 @@ class BuildbotTriggerable {
                     associatedRequests.add(request);
 
                     const info = buildReqeustsByGroup.get(request.testGroupId());
-                    assert(!info.syncer || info.syncer == syncer);
-                    info.syncer = syncer;
-                    if (entry.slaveName()) {
-                        assert(!info.slaveName || info.slaveName == entry.slaveName());
-                        info.slaveName = entry.slaveName();
+                    if (request.isBuild()) {
+                        assert(!info.buildSyncer || info.buildSyncer == buildSyncer);
+                        if (entry.slaveName()) {
+                            assert(!info.buildSlaveName || info.buildSlaveName == entry.slaveName());
+                            info.buildSlaveName = entry.slaveName();
+                        }
+                    } else {
+                        assert(!info.testSyncer || info.testSyncer == testSyncer);
+                        if (entry.slaveName()) {
+                            assert(!info.testSlaveName || info.testSlaveName == entry.slaveName());
+                            info.testSlaveName = entry.slaveName();
+                        }
                     }
 
                     const newStatus = entry.buildRequestStatusIfUpdateIsNeeded(request);