A/B testing bots should prioritize user created test groups
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Apr 2016 03:19:22 +0000 (03:19 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Apr 2016 03:19:22 +0000 (03:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=156375

Reviewed by Chris Dumez.

Order build requests preferring user created ones over ones automatically created by detect-changes.js.

Also fixed a bug in BuildbotSyncer.scheduleFirstRequestInGroupIfAvailable that it was scheduling a new
build request on a builder/slave even when we had previously scheduled another build request.

* public/include/build-requests-fetcher.php:
(BuildRequestsFetcher::fetch_incomplete_requests_for_triggerable): Order build requested based on
author_order which is 0 when it's created by an user and 1 when it's created by detect-changes.js.
Since we're using ascending order, this would put user created test groups first.
* server-tests/api-build-requests-tests.js: Updated an existing test case and added a new test case
for testing that build requests for an user created test group shows up first.
* server-tests/resources/mock-data.js:
(MockData.addAnotherMockTestGroup): Takes an extra argument to specify the author name.
* server-tests/tools-buildbot-triggerable-tests.js: Added a test case for testing that build requests
for an user created test group shows up first.
* tools/js/buildbot-syncer.js:
(BuildbotSyncer): Added _slavesWithNewRequests to keep track of build slaves on which we have already
scheduled new build requests. Don't schedule more requests on these slaves.
(BuildbotSyncer.prototype.scheduleRequest):
(BuildbotSyncer.prototype.scheduleFirstRequestInGroupIfAvailable): Add the specified slave name (or null
when slaveList is not specified) to _slavesWithNewRequests.
(BuildbotSyncer.prototype.pullBuildbot): Clear the set after pulling buildbot since any build request
we have previously scheduled should be included in one of the entires now.
* unit-tests/buildbot-syncer-tests.js: Added test cases for the aforementioned bug.
(sampleiOSConfig): Added a second slave for new test cases.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/include/build-requests-fetcher.php
Websites/perf.webkit.org/server-tests/api-build-requests-tests.js
Websites/perf.webkit.org/server-tests/resources/mock-data.js
Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js
Websites/perf.webkit.org/tools/js/buildbot-syncer.js
Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js

index 0a14993..57915b6 100644 (file)
@@ -1,5 +1,38 @@
 2016-04-07  Ryosuke Niwa  <rniwa@webkit.org>
 
+        A/B testing bots should prioritize user created test groups
+        https://bugs.webkit.org/show_bug.cgi?id=156375
+
+        Reviewed by Chris Dumez.
+
+        Order build requests preferring user created ones over ones automatically created by detect-changes.js.
+
+        Also fixed a bug in BuildbotSyncer.scheduleFirstRequestInGroupIfAvailable that it was scheduling a new
+        build request on a builder/slave even when we had previously scheduled another build request.
+
+        * public/include/build-requests-fetcher.php:
+        (BuildRequestsFetcher::fetch_incomplete_requests_for_triggerable): Order build requested based on
+        author_order which is 0 when it's created by an user and 1 when it's created by detect-changes.js.
+        Since we're using ascending order, this would put user created test groups first.
+        * server-tests/api-build-requests-tests.js: Updated an existing test case and added a new test case
+        for testing that build requests for an user created test group shows up first.
+        * server-tests/resources/mock-data.js:
+        (MockData.addAnotherMockTestGroup): Takes an extra argument to specify the author name.
+        * server-tests/tools-buildbot-triggerable-tests.js: Added a test case for testing that build requests
+        for an user created test group shows up first.
+        * tools/js/buildbot-syncer.js:
+        (BuildbotSyncer): Added _slavesWithNewRequests to keep track of build slaves on which we have already
+        scheduled new build requests. Don't schedule more requests on these slaves.
+        (BuildbotSyncer.prototype.scheduleRequest):
+        (BuildbotSyncer.prototype.scheduleFirstRequestInGroupIfAvailable): Add the specified slave name (or null
+        when slaveList is not specified) to _slavesWithNewRequests.
+        (BuildbotSyncer.prototype.pullBuildbot): Clear the set after pulling buildbot since any build request
+        we have previously scheduled should be included in one of the entires now.
+        * unit-tests/buildbot-syncer-tests.js: Added test cases for the aforementioned bug.
+        (sampleiOSConfig): Added a second slave for new test cases.
+
+2016-04-07  Ryosuke Niwa  <rniwa@webkit.org>
+
         Migrate legacy perf dashboard tests to mocha.js based tests
         https://bugs.webkit.org/show_bug.cgi?id=156335
 
index 71d9d52..0a76879 100644 (file)
@@ -26,12 +26,13 @@ class BuildRequestsFetcher {
     }
 
     function fetch_incomplete_requests_for_triggerable($triggerable_id) {
-        $this->rows = $this->db->query_and_fetch_all('SELECT * FROM build_requests
-            WHERE request_triggerable = $1 AND request_group
-                IN (SELECT testgroup_id FROM analysis_test_groups WHERE EXISTS
+        $this->rows = $this->db->query_and_fetch_all('SELECT * FROM build_requests,
+            (SELECT testgroup_id, (case when testgroup_author is not null then 0 else 1 end) as author_order, testgroup_created_at
+                FROM analysis_test_groups WHERE EXISTS
                     (SELECT 1 FROM build_requests WHERE testgroup_id = request_group AND request_status
-                        IN (\'pending\', \'scheduled\', \'running\')))
-            ORDER BY request_group, request_order', array($triggerable_id));
+                        IN (\'pending\', \'scheduled\', \'running\'))) AS test_groups
+            WHERE request_triggerable = $1 AND request_group = test_groups.testgroup_id
+            ORDER BY author_order, testgroup_created_at, request_order', array($triggerable_id));
     }
 
     function fetch_request($request_id) {
index 5ca3a7e..1aa79a3 100644 (file)
@@ -326,10 +326,41 @@ describe('/api/build-requests', function () {
         }).catch(done);
     });
 
-    it('should order build requests based on test group and order', function (done) {
+    it('should order build requests based on test group creation time and order', function (done) {
         let db = TestServer.database();
         db.connect().then(function () {
-            return Promise.all([MockData.addMockData(db), MockData.addAnotherMockTestGroup(db)])
+            return Promise.all([MockData.addMockData(db), MockData.addAnotherMockTestGroup(db)]);
+        }).then(function () {
+            return Manifest.fetch();
+        }).then(function () {
+            return BuildRequest.fetchForTriggerable('build-webkit');
+        }).then(function (buildRequests) {
+            assert.equal(buildRequests.length, 8);
+            assert.equal(buildRequests[0].id(), 700);
+            assert.equal(buildRequests[0].testGroupId(), 600);
+            assert.equal(buildRequests[1].id(), 701);
+            assert.equal(buildRequests[1].testGroupId(), 600);
+            assert.equal(buildRequests[2].id(), 702);
+            assert.equal(buildRequests[2].testGroupId(), 600);
+            assert.equal(buildRequests[3].id(), 703);
+            assert.equal(buildRequests[3].testGroupId(), 600);
+
+            assert.equal(buildRequests[4].id(), 710);
+            assert.equal(buildRequests[4].testGroupId(), 599);
+            assert.equal(buildRequests[5].id(), 711);
+            assert.equal(buildRequests[5].testGroupId(), 599);
+            assert.equal(buildRequests[6].id(), 712);
+            assert.equal(buildRequests[6].testGroupId(), 599);
+            assert.equal(buildRequests[7].id(), 713);
+            assert.equal(buildRequests[7].testGroupId(), 599);
+            done();
+        }).catch(done);
+    });
+
+    it('should place build requests created by user before automatically created ones', function (done) {
+        let db = TestServer.database();
+        db.connect().then(function () {
+            return Promise.all([MockData.addMockData(db), MockData.addAnotherMockTestGroup(db, null, 'rniwa')]);
         }).then(function () {
             return Manifest.fetch();
         }).then(function () {
@@ -344,8 +375,16 @@ describe('/api/build-requests', function () {
             assert.equal(buildRequests[2].testGroupId(), 599);
             assert.equal(buildRequests[3].id(), 713);
             assert.equal(buildRequests[3].testGroupId(), 599);
+
+            assert.equal(buildRequests[4].id(), 700);
+            assert.equal(buildRequests[4].testGroupId(), 600);
+            assert.equal(buildRequests[5].id(), 701);
+            assert.equal(buildRequests[5].testGroupId(), 600);
+            assert.equal(buildRequests[6].id(), 702);
+            assert.equal(buildRequests[6].testGroupId(), 600);
+            assert.equal(buildRequests[7].id(), 703);
+            assert.equal(buildRequests[7].testGroupId(), 600);
             done();
         }).catch(done);
     });
-
 });
index 15bcf30..6c88681 100644 (file)
@@ -46,12 +46,12 @@ MockData = {
             db.insert('build_requests', {id: 703, status: statusList[3], triggerable: 1, platform: 65, test: 200, group: 600, order: 3, root_set: 402}),
         ]);
     },
-    addAnotherMockTestGroup: function (db, statusList)
+    addAnotherMockTestGroup: function (db, statusList, author)
     {
         if (!statusList)
             statusList = ['pending', 'pending', 'pending', 'pending'];
         return Promise.all([
-            db.insert('analysis_test_groups', {id: 599, task: 500, name: 'another test group'}),
+            db.insert('analysis_test_groups', {id: 599, task: 500, name: 'another test group', author: author}),
             db.insert('build_requests', {id: 713, status: statusList[3], triggerable: 1, platform: 65, test: 200, group: 599, order: 3, root_set: 402}),
             db.insert('build_requests', {id: 710, status: statusList[0], triggerable: 1, platform: 65, test: 200, group: 599, order: 0, root_set: 401}),
             db.insert('build_requests', {id: 712, status: statusList[2], triggerable: 1, platform: 65, test: 200, group: 599, order: 2, root_set: 401}),
index 8deb630..95d2f7c 100644 (file)
@@ -440,5 +440,45 @@ describe('BuildbotTriggerable', function () {
                 done();
             }).catch(done);
         });
+
+        it('should schedule a build request of an user created test group before ones created by automatic change detection', function (done) {
+            let db = TestServer.database();
+            let syncPromise;
+            db.connect().then(function () {
+                return Promise.all([
+                    MockData.addMockData(db, ['pending', 'pending', 'pending', 'pending']),
+                    MockData.addAnotherMockTestGroup(db, ['pending', 'pending', 'pending', 'pending'], 'rniwa'),
+                ]);
+            }).then(function () {
+                return Manifest.fetch();
+            }).then(function () {
+                let config = MockData.mockTestSyncConfigWithSingleBuilder();
+                let logger = new MockLogger;
+                let slaveInfo = {name: 'sync-slave', password: 'password'};
+                let triggerable = new BuildbotTriggerable(config, TestServer.remoteAPI(), MockRemoteAPI, slaveInfo, logger);
+                syncPromise = triggerable.syncOnce();
+                syncPromise.catch(done);
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 1);
+                assert.equal(MockRemoteAPI.requests[0].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[0].url, '/json/builders/some-builder-1/pendingBuilds');
+                MockRemoteAPI.requests[0].resolve([]);
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 2);
+                assert.equal(MockRemoteAPI.requests[1].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[1].url, '/json/builders/some-builder-1/builds/?select=-1&select=-2');
+                MockRemoteAPI.requests[1].resolve({});
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 3);
+                assert.equal(MockRemoteAPI.requests[2].method, 'POST');
+                assert.equal(MockRemoteAPI.requests[2].url, '/builders/some-builder-1/force');
+                assert.deepEqual(MockRemoteAPI.requests[2].data, {'wk': '191622', 'os': '10.11 15A284', 'build-request-id': '710'});
+                MockRemoteAPI.requests[2].resolve('OK');
+                done();
+            }).catch(done);
+        });
     });
 });
index d5247ef..5d94ebd 100644 (file)
@@ -66,6 +66,7 @@ class BuildbotSyncer {
         this._slaveList = object.slaveList;
         this._buildRequestPropertyName = object.buildRequestArgument;
         this._entryList = null;
+        this._slavesWithNewRequests = new Set;
     }
 
     builderName() { return this._builderName; }
@@ -89,12 +90,14 @@ class BuildbotSyncer {
 
     scheduleRequest(newRequest, slaveName)
     {
+        assert(!this._slavesWithNewRequests.has(slaveName));
         let properties = this._propertiesForBuildRequest(newRequest);
 
         assert.equal(!this._slavePropertyName, !slaveName);
         if (this._slavePropertyName)
             properties[this._slavePropertyName] = slaveName;
 
+        this._slavesWithNewRequests.add(slaveName);
         return this._remote.postFormUrlencodedData(this.pathForForceBuild(), properties);
     }
 
@@ -116,13 +119,13 @@ class BuildbotSyncer {
         }
 
         if (!this._slaveList || hasPendingBuildsWithoutSlaveNameSpecified) {
-            if (usedSlaves.size)
+            if (usedSlaves.size || this._slavesWithNewRequests.size)
                 return null;
             return this.scheduleRequest(newRequest, null);
         }
 
         for (let slaveName of this._slaveList) {
-            if (!usedSlaves.has(slaveName))
+            if (!usedSlaves.has(slaveName) && !this._slavesWithNewRequests.has(slaveName))
                 return this.scheduleRequest(newRequest, slaveName);
         }
 
@@ -148,6 +151,7 @@ class BuildbotSyncer {
                     entryList.push(entryByRequest[id]);
 
                 self._entryList = entryList;
+                self._slavesWithNewRequests.clear();
 
                 return entryList;
             });
index d409149..5bb8ccf 100644 (file)
@@ -44,7 +44,7 @@ function sampleiOSConfig()
             'iPad-bench': {
                 'builder': 'ABTest-iPad-RunBenchmark-Tests',
                 'arguments': { 'forcescheduler': 'ABTest-iPad-RunBenchmark-Tests-ForceScheduler' },
-                'slaveList': ['ABTest-iPad-0'],
+                'slaveList': ['ABTest-iPad-0', 'ABTest-iPad-1'],
             }
         },
         'configurations': [
@@ -963,5 +963,47 @@ describe('BuildbotSyncer', function () {
             }).catch(done);
         });
 
+        it('should not schedule a build if a new request had been submitted to the same slave', function (done) {
+            let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig())[1];
+
+            pullBuildbotWithAssertion(syncer, [], {}).then(function () {
+                syncer.scheduleRequest(createSampleBuildRequest(MockModels.ipad, MockModels.speedometer), 'ABTest-iPad-0');
+                syncer.scheduleRequest(createSampleBuildRequest(MockModels.ipad, MockModels.speedometer), 'ABTest-iPad-1');
+            }).then(function () {
+                assert.equal(requests.length, 2);
+                syncer.scheduleFirstRequestInGroupIfAvailable(createSampleBuildRequest(MockModels.ipad, MockModels.speedometer));
+            }).then(function () {
+                assert.equal(requests.length, 2);
+                done();
+            }).catch(done);
+        });
+
+        it('should schedule a build if a new request had been submitted to another slave', function (done) {
+            let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig())[1];
+
+            pullBuildbotWithAssertion(syncer, [], {}).then(function () {
+                syncer.scheduleRequest(createSampleBuildRequest(MockModels.ipad, MockModels.speedometer), 'ABTest-iPad-0');
+            }).then(function () {
+                assert.equal(requests.length, 1);
+                syncer.scheduleFirstRequestInGroupIfAvailable(createSampleBuildRequest(MockModels.ipad, MockModels.speedometer), 'ABTest-iPad-1');
+            }).then(function () {
+                assert.equal(requests.length, 2);
+                done();
+            }).catch(done);
+        });
+
+        it('should not schedule a build if a new request had been submitted to the same builder without slaveList', function (done) {
+            let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, {'configurations': [smallConfiguration()]})[0];
+
+            pullBuildbotWithAssertion(syncer, [], {}).then(function () {
+                syncer.scheduleRequest(createSampleBuildRequest(MockModels.somePlatform, MockModels.someTest), null);
+            }).then(function () {
+                assert.equal(requests.length, 1);
+                syncer.scheduleFirstRequestInGroupIfAvailable(createSampleBuildRequest(MockModels.somePlatform, MockModels.someTest));
+            }).then(function () {
+                assert.equal(requests.length, 1);
+                done();
+            }).catch(done);
+        });
     });
 });