Custom analysis task configurator should allow supplying commit prefix and revision...
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Jun 2019 23:11:59 +0000 (23:11 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Jun 2019 23:11:59 +0000 (23:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198847

Reviewed by Ryosuke Niwa.

Custom analysis task configurator should not require full SHA to start an A/B test.
Custom analysis task configurator should accept svn revision starts with 'r'.

* browser-tests/custom-analysis-task-configurator-tests.js: Added a unit test for this change.
* public/api/commits.php: Extend this API to allow prefix matching when fethcing a single commit.
* public/include/commit-log-fetcher.php: Added a function to fetch a commit with prefix.
* public/v3/components/custom-analysis-task-configurator.js: Add UI support for accepting partial revision.
(CustomAnalysisTaskConfigurator.prototype._computeCommitSet):
(CustomAnalysisTaskConfigurator.prototype.async._resolveRevision):
(CustomAnalysisTaskConfigurator.prototype._buildTestabilityList):
* public/v3/models/commit-log.js:
(CommitLog.async.fetchForSingleRevision): Added third argument to specify prefix matching which defaults to false.
* server-tests/api-commits-tests.js: Added unit tests.
* unit-tests/commit-log-tests.js: Added a unit test.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js
Websites/perf.webkit.org/public/api/commits.php
Websites/perf.webkit.org/public/include/commit-log-fetcher.php
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js
Websites/perf.webkit.org/public/v3/models/commit-log.js
Websites/perf.webkit.org/server-tests/api-commits-tests.js
Websites/perf.webkit.org/unit-tests/commit-log-tests.js

index ca67a08..6612490 100644 (file)
@@ -1,3 +1,25 @@
+2019-06-13  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Custom analysis task configurator should allow supplying commit prefix and revision starts 'r'.
+        https://bugs.webkit.org/show_bug.cgi?id=198847
+
+        Reviewed by Ryosuke Niwa.
+
+        Custom analysis task configurator should not require full SHA to start an A/B test.
+        Custom analysis task configurator should accept svn revision starts with 'r'.
+
+        * browser-tests/custom-analysis-task-configurator-tests.js: Added a unit test for this change.
+        * public/api/commits.php: Extend this API to allow prefix matching when fethcing a single commit.
+        * public/include/commit-log-fetcher.php: Added a function to fetch a commit with prefix.
+        * public/v3/components/custom-analysis-task-configurator.js: Add UI support for accepting partial revision.
+        (CustomAnalysisTaskConfigurator.prototype._computeCommitSet):
+        (CustomAnalysisTaskConfigurator.prototype.async._resolveRevision):
+        (CustomAnalysisTaskConfigurator.prototype._buildTestabilityList):
+        * public/v3/models/commit-log.js:
+        (CommitLog.async.fetchForSingleRevision): Added third argument to specify prefix matching which defaults to false.
+        * server-tests/api-commits-tests.js: Added unit tests.
+        * unit-tests/commit-log-tests.js: Added a unit test.
+
 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.
index cd73942..85af7b1 100644 (file)
@@ -56,7 +56,7 @@ describe('CustomAnalysisTaskConfigurator', () => {
         customAnalysisTaskConfigurator.content('baseline-revision-table').querySelector('input').dispatchEvent(new Event('input'));
         await sleep(context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval);
         expect(requests.length).to.be(2);
-        expect(requests[1].url).to.be('/api/commits/1/123');
+        expect(requests[1].url).to.be('/api/commits/1/123?prefix-match=true');
 
         customAnalysisTaskConfigurator._configureComparison();
         await waitForComponentsToRender(context);
@@ -65,7 +65,7 @@ describe('CustomAnalysisTaskConfigurator', () => {
         customAnalysisTaskConfigurator.content('comparison-revision-table').querySelector('input').dispatchEvent(new Event('input'));
         await sleep(context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval);
         expect(requests.length).to.be(3);
-        expect(requests[2].url).to.be('/api/commits/1/456');
+        expect(requests[2].url).to.be('/api/commits/1/456?prefix-match=true');
 
         const commitSets = customAnalysisTaskConfigurator.commitSets();
         expect(commitSets.length).to.be(2);
@@ -77,6 +77,93 @@ describe('CustomAnalysisTaskConfigurator', () => {
         context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval = 100;
     });
 
+    const webkitCommit = {
+        "id": "185338",
+        "revision": "abcdefg",
+        "repository": 1,
+        "previousCommit": null,
+        "ownsCommits": false,
+        "time": +new Date("2017-01-20T03:49:37.887Z"),
+        "authorName": "Commit Queue",
+        "authorEmail": "commit-queue@webkit.org",
+        "message": "another message",
+    };
+
+    const anotherWebkitCommit = {
+        "id": "185334",
+        "revision": "aabcdef",
+        "repository": 1,
+        "previousCommit": null,
+        "ownsCommits": false,
+        "time": +new Date("2017-01-20T03:23:50.645Z"),
+        "authorName": "Chris Dumez",
+        "authorEmail": "cdumez@apple.com",
+        "message": "some message",
+    };
+
+    it('Should use full commit revision even if UI does not have the full revision', async () => {
+        const context = new BrowsingContext();
+        const customAnalysisTaskConfigurator = await createCustomAnalysisTaskConfiguratorWithContext(context);
+        context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval = 1;
+
+        const test = new context.symbols.Test(1, {name: 'Speedometer'});
+        const platform = new context.symbols.Platform(1, {
+            name: 'Mojave',
+            metrics: [
+                new context.symbols.Metric(1, {
+                    name: 'Allocation',
+                    aggregator: 'Arithmetic',
+                    test
+                })
+            ],
+            lastModifiedByMetric: Date.now(),
+        });
+        const repository = context.symbols.Repository.ensureSingleton(1, {name: 'WebKit'});
+        const triggerableRepositoryGroup = new context.symbols.TriggerableRepositoryGroup(1, {repositories: [{repository}]});
+        new context.symbols.Triggerable(1, {
+            name: 'test-triggerable',
+            isDisabled: false,
+            repositoryGroups: [triggerableRepositoryGroup],
+            configurations: [{test, platform}],
+        });
+        customAnalysisTaskConfigurator.selectTests([test]);
+        customAnalysisTaskConfigurator.selectPlatform(platform);
+
+        await waitForComponentsToRender(context);
+
+        const requests = context.symbols.MockRemoteAPI.requests;
+        expect(requests.length).to.be(1);
+        expect(requests[0].url).to.be('/api/commits/1/latest?platform=1');
+        requests[0].reject();
+
+        customAnalysisTaskConfigurator.content('baseline-revision-table').querySelector('input').value = 'abc';
+        customAnalysisTaskConfigurator.content('baseline-revision-table').querySelector('input').dispatchEvent(new Event('input'));
+        await sleep(context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval);
+        expect(requests.length).to.be(2);
+        expect(requests[1].url).to.be('/api/commits/1/abc?prefix-match=true');
+        requests[1].resolve({commits: [webkitCommit]});
+
+        customAnalysisTaskConfigurator._configureComparison();
+        await waitForComponentsToRender(context);
+
+        customAnalysisTaskConfigurator.content('comparison-revision-table').querySelector('input').value = 'aabc';
+        customAnalysisTaskConfigurator.content('comparison-revision-table').querySelector('input').dispatchEvent(new Event('input'));
+        await sleep(context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval);
+        expect(requests.length).to.be(3);
+        expect(requests[2].url).to.be('/api/commits/1/aabc?prefix-match=true');
+        requests[2].resolve({commits: [anotherWebkitCommit]});
+
+        await waitForComponentsToRender(context);
+        const commitSets = customAnalysisTaskConfigurator.commitSets();
+        expect(commitSets.length).to.be(2);
+        expect(commitSets[0].repositories().length).to.be(1);
+        expect(commitSets[0].revisionForRepository(repository)).to.be('abcdefg');
+        expect(commitSets[1].repositories().length).to.be(1);
+        expect(commitSets[1].revisionForRepository(repository)).to.be('aabcdef');
+
+        context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval = 100;
+    });
+
     it('Should not update commitSetMap if baseline is set and unmodified but comparison is null', async () => {
         const context = new BrowsingContext();
         const customAnalysisTaskConfigurator = await createCustomAnalysisTaskConfiguratorWithContext(context);
@@ -145,7 +232,7 @@ describe('CustomAnalysisTaskConfigurator', () => {
         customAnalysisTaskConfigurator.content('baseline-revision-table').querySelector('input').dispatchEvent(new Event('input'));
         await sleep(context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval);
         expect(requests.length).to.be(2);
-        expect(requests[1].url).to.be('/api/commits/1/123');
+        expect(requests[1].url).to.be('/api/commits/1/123?prefix-match=true');
 
         customAnalysisTaskConfigurator._configureComparison();
         await waitForComponentsToRender(context);
@@ -154,7 +241,7 @@ describe('CustomAnalysisTaskConfigurator', () => {
         customAnalysisTaskConfigurator.content('comparison-revision-table').querySelector('input').dispatchEvent(new Event('input'));
         await sleep(context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval);
         expect(requests.length).to.be(3);
-        expect(requests[2].url).to.be('/api/commits/1/456');
+        expect(requests[2].url).to.be('/api/commits/1/456?prefix-match=true');
 
         let commitSets = customAnalysisTaskConfigurator.commitSets();
         expect(commitSets.length).to.be(2);
@@ -286,7 +373,7 @@ describe('CustomAnalysisTaskConfigurator', () => {
         customAnalysisTaskConfigurator.content('baseline-revision-table').querySelector('input').dispatchEvent(new Event('input'));
         await sleep(context.symbols.CustomAnalysisTaskConfigurator.commitFetchInterval);
         expect(requests.length).to.be(2);
-        expect(requests[1].url).to.be('/api/commits/1/123');
+        expect(requests[1].url).to.be('/api/commits/1/123?prefix-match=true');
 
         customAnalysisTaskConfigurator._configureComparison();
         await waitForComponentsToRender(context);
index 96cce1f..5c57f02 100644 (file)
@@ -48,8 +48,10 @@ function main($paths) {
             $commits = $fetcher->fetch_last_reported_between_orders($repository_id, $from, $to);
         else
             $commits = $fetcher->fetch_last_reported($repository_id);
-    } else
-        $commits = $fetcher->fetch_revision($repository_id, $filter);
+    } else {
+        $prefix_match = $keyword = array_get($_GET, 'prefix-match');
+        $commits = $fetcher->fetch_revision($repository_id, $filter, $prefix_match);
+    }
 
     if (!is_array($commits))
         exit_with_error('FailedToFetchCommits', array('repository' => $repository_id, 'filter' => $filter));
index d5f61a6..540f1de 100644 (file)
@@ -178,14 +178,21 @@ class CommitLogFetcher {
         return $this->format_single_commit($this->db->select_last_row('commits', 'commit', array('repository' => $repository_id, 'reported' => true), array('time', 'order')));
     }
 
-    function fetch_revision($repository_id, $revision) {
-        return $this->format_single_commit($this->commit_for_revision($repository_id, $revision));
+    function fetch_revision($repository_id, $revision, $prefix_match) {
+        $all_but_first = substr($revision, 1);
+        $is_svn_revision = false;
+        if ($revision[0] == 'r' && ctype_digit($all_but_first)) {
+            $revision = $all_but_first;
+            $is_svn_revision = true;
+        }
+        if ($prefix_match && !$is_svn_revision)
+            $commit = $this->commit_for_revision_prefix($repository_id, $revision);
+        else
+            $commit = $this->commit_for_revision($repository_id, $revision);
+        return $this->format_single_commit($commit);
     }
 
     private function commit_for_revision($repository_id, $revision) {
-        $all_but_first = substr($revision, 1);
-        if ($revision[0] == 'r' && ctype_digit($all_but_first))
-            $revision = $all_but_first;
         $commit_info = array('repository' => $repository_id, 'revision' => $revision);
         $row = $this->db->select_last_row('commits', 'commit', $commit_info);
         if (!$row)
@@ -193,6 +200,15 @@ class CommitLogFetcher {
         return $row;
     }
 
+    private function commit_for_revision_prefix($repository_id, $revision_prefix) {
+        $rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE commit_repository = $1 AND commit_revision LIKE $2 LIMIT 2', array($repository_id, Database::escape_for_like($revision_prefix) . '%'));
+        if (count($rows) == 0)
+            exit_with_error('UnknownCommit', array('repository' => $repository_id, 'revision' => $revision_prefix));
+        if (count($rows) == 2)
+            exit_with_error('AmbiguousRevisionPrefix', array('repository' => $repository_id, 'revision' => $revision_prefix));
+        return $rows[0];
+    }
+
     private function format_single_commit($commit_row) {
         if (!$commit_row)
             return array();
index 3e09464..e2c5342 100644 (file)
@@ -354,9 +354,10 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
         const commitSet = new CustomCommitSet;
         for (let repository of repositoryGroup.repositories()) {
             let revision = this._specifiedRevisions[configurationName].get(repository);
-            if (!revision) {
-                const commit = this._fetchedCommits[configurationName].get(repository);
-                if (commit)
+            const commit = this._fetchedCommits[configurationName].get(repository);
+            if (commit) {
+                const commitLabel = commit.label();
+                if (!revision || commit.revision().startsWith(revision) || commitLabel.startsWith(revision) || revision.startsWith(commitLabel))
                     revision = commit.revision();
             }
             if (!revision)
@@ -405,25 +406,29 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
     async _resolveRevision(repository, revision, specifiedRevisions, invalidRevisionForRepository, fetchedCommits)
     {
         const fetchedCommit = fetchedCommits.get(repository);
-        if (fetchedCommit && fetchedCommit.revision() == revision)
+        const specifiedRevision = specifiedRevisions.get(repository);
+        if (fetchedCommit && fetchedCommit.revision() == revision && (!specifiedRevision || specifiedRevision == revision))
             return;
 
         fetchedCommits.delete(repository);
         let commits = [];
+        const revisionToFetch = specifiedRevision || revision;
         try {
-            commits = await CommitLog.fetchForSingleRevision(repository, revision);
+            commits = await CommitLog.fetchForSingleRevision(repository, revisionToFetch, true);
         } catch (error) {
-            console.assert(error == 'UnknownCommit');
-            if (revision != specifiedRevisions.get(repository))
+            console.assert(error == 'UnknownCommit' || error == 'AmbiguousRevisionPrefix');
+            if (revisionToFetch != specifiedRevisions.get(repository))
                 return;
-            invalidRevisionForRepository.set(repository, revision);
+            invalidRevisionForRepository.set(repository, `"${revisionToFetch}": ${error == 'UnknownCommit' ? 'Invalid revision' : 'Ambiguous revision prefix'}`);
             return;
         }
         console.assert(commits.length, 1);
-        if (revision != specifiedRevisions.get(repository))
+        if (revisionToFetch != specifiedRevisions.get(repository))
             return;
         invalidRevisionForRepository.delete(repository);
         fetchedCommits.set(repository, commits[0]);
+        if (revisionToFetch != commits[0].revision())
+            this._updateCommitSetMap();
     }
 
     _renderRepositoryPanes(triggerable, error, platform, repositoryGroupByConfiguration, showComparison)
@@ -536,7 +541,7 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
             if (commit && commit.testability() && !invalidRevisionForRepository.has(repository))
                 entries.push(element('li', `${commit.repository().name()} - "${commit.label()}": ${commit.testability()}`));
             if (invalidRevisionForRepository.has(repository))
-                entries.push(element('li', `${repository.name()} - "${invalidRevisionForRepository.get(repository)}": Invalid revision`));
+                entries.push(element('li', `${repository.name()} - ${invalidRevisionForRepository.get(repository)}`));
         }
 
         return entries;
index d9bd438..da51f5f 100644 (file)
@@ -172,13 +172,16 @@ class CommitLog extends DataModelObject {
         return this._constructFromRawData(data);
     }
 
-    static async fetchForSingleRevision(repository, revision)
+    static async fetchForSingleRevision(repository, revision, prefixMatch=false)
     {
         const commit = repository.commitForRevision(revision);
         if (commit)
             return [commit];
 
-        const data = await this.cachedFetch(`/api/commits/${repository.id()}/${revision}`);
+        let params = {};
+        if (prefixMatch)
+            params['prefix-match'] = true;
+        const data = await this.cachedFetch(`/api/commits/${repository.id()}/${revision}`, params);
         return this._constructFromRawData(data);
     }
 
index 324962a..98b02ef 100644 (file)
@@ -394,6 +394,50 @@ describe("/api/commits/", function () {
             });
         });
 
+        it("should return the full result for a reported commit with prefix-match to be false", async () => {
+            const remote = TestServer.remoteAPI();
+            await addSlaveForReport(subversionCommits);
+            await remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            const result = await remote.getJSON('/api/commits/WebKit/210949?prefix-match=false');
+            assert.equal(result['status'], 'OK');
+            assert.deepEqual(result['commits'].length, 1);
+            assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'][1]);
+        });
+
+        it("should return the full result for a reported commit with prefix-match to be true", async () => {
+            const remote = TestServer.remoteAPI();
+            await addSlaveForReport(subversionCommits);
+            await remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            const result = await remote.getJSON('/api/commits/WebKit/210949?prefix-match=true');
+            assert.equal(result['status'], 'OK');
+            assert.deepEqual(result['commits'].length, 1);
+            assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'][1]);
+        });
+
+        it("should return 'AmbiguousRevisionPrefix' when more than one commits are found for a revision prefix", async () => {
+            const remote = TestServer.remoteAPI();
+            await addSlaveForReport(subversionCommits);
+            await remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            const result = await remote.getJSON('/api/commits/WebKit/21094?prefix-match=true');
+            assert.equal(result['status'], 'AmbiguousRevisionPrefix');
+        });
+
+        it("should return 'UnknownCommit' when no commit is found for a revision prefix", async () => {
+            const remote = TestServer.remoteAPI();
+            await addSlaveForReport(subversionCommits);
+            await remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            const result = await remote.getJSON('/api/commits/WebKit/21090?prefix-match=true');
+            assert.equal(result['status'], 'UnknownCommit');
+        });
+
+        it("should not match prefix and return 'UnkownCommit' when svn commit starts with 'r' prefix and there is no exact match", async () => {
+            const remote = TestServer.remoteAPI();
+            await addSlaveForReport(subversionCommits);
+            await remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            const result = await remote.getJSON('/api/commits/WebKit/r21095?prefix-match=true');
+            assert.equal(result['status'], 'UnknownCommit');
+        });
+
         it("should handle commit revision with space", () => {
             const db = TestServer.database();
             return Promise.all([
index 86a1263..40daa07 100644 (file)
@@ -539,5 +539,19 @@ describe('CommitLog', function () {
 
             assert.notEqual(commits[0], newCommits[0]);
         });
+
+        it('should allow to fetch commit with prefix', async () => {
+            CommitLog.fetchForSingleRevision(MockModels.webkit, '236643', true);
+            const requests = MockRemoteAPI.requests;
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, `/api/commits/${MockModels.webkit.id()}/236643?prefix-match=true`);
+        });
+
+        it('should not set prefix-match when "prefixMatch" is false', async () => {
+            CommitLog.fetchForSingleRevision(MockModels.webkit, '236643', false);
+            const requests = MockRemoteAPI.requests;
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, `/api/commits/${MockModels.webkit.id()}/236643`);
+        })
     });
 });