Attachment 372097 Details for Bug 198847 – Patch

[patch] Patch

bug-198847-20190613191959.patch (text/plain), 21.20 KB, created by dewei_zhu on 2019-06-13 19:19:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: dewei_zhu

Created: 2019-06-13 19:19:59 PDT

Size: 21.20 KB

patch

obsolete

>Subversion Revision: 246418
>diff --git a/Websites/perf.webkit.org/ChangeLog b/Websites/perf.webkit.org/ChangeLog
>index ca67a089d0dc18205236216529b6284fabacdba0..873eb961304406459ac80f899c1b6280a521c2fb 100644
>--- a/Websites/perf.webkit.org/ChangeLog
>+++ b/Websites/perf.webkit.org/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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.
>diff --git a/Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js b/Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js
>index cd739423da42ffbde63b21ecc32fba753ef69df9..091839c4691b3166a7514c493e7cbecadf057102 100644
>--- a/Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js
>+++ b/Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js
>@@ -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 webkitCommit123123 = {
>+        "id": "185338",
>+        "revision": "123123",
>+        "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 webkitCommit456456 = {
>+        "id": "185334",
>+        "revision": "456456",
>+        "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 = '123';
>+        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?prefix-match=true');
>+        requests[1].resolve({commits: [webkitCommit123123]});
>+
>+        customAnalysisTaskConfigurator._configureComparison();
>+        await waitForComponentsToRender(context);
>+
>+        customAnalysisTaskConfigurator.content('comparison-revision-table').querySelector('input').value = 'r456';
>+        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/r456?prefix-match=true');
>+        requests[2].resolve({commits: [webkitCommit456456]});
>+
>+        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('123123');
>+        expect(commitSets[1].repositories().length).to.be(1);
>+        expect(commitSets[1].revisionForRepository(repository)).to.be('456456');
>+
>+        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);
>diff --git a/Websites/perf.webkit.org/public/api/commits.php b/Websites/perf.webkit.org/public/api/commits.php
>index 96cce1f970f0b7ffaa9cda647e2bfc81cf2bded6..5c57f0291f0cc8949f3635b60b8514f27d55ddda 100644
>--- a/Websites/perf.webkit.org/public/api/commits.php
>+++ b/Websites/perf.webkit.org/public/api/commits.php
>@@ -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));
>diff --git a/Websites/perf.webkit.org/public/include/commit-log-fetcher.php b/Websites/perf.webkit.org/public/include/commit-log-fetcher.php
>index d5f61a60898ab1585d780db6f571b0d617cf4251..0348356c638486a5b90773f6ffcad262ea40e833 100644
>--- a/Websites/perf.webkit.org/public/include/commit-log-fetcher.php
>+++ b/Websites/perf.webkit.org/public/include/commit-log-fetcher.php
>@@ -178,8 +178,12 @@ 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) {
>+        if ($prefix_match)
>+            $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) {
>@@ -193,6 +197,18 @@ class CommitLogFetcher {
>         return $row;
>     }
> 
>+    private function commit_for_revision_prefix($repository_id, $revision_prefix) {
>+        $all_but_first = substr($revision_prefix, 1);
>+        if ($revision_prefix[0] == 'r' && ctype_digit($all_but_first))
>+            $revision_prefix = $all_but_first;
>+        $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();
>diff --git a/Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js b/Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js
>index 3e09464c0feeec4f135ddfb1a336f232a0099000..6daae96c0d209fe4389a7aaa1bbc8ea254d37315 100644
>--- a/Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js
>+++ b/Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js
>@@ -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,30 @@ 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.log(error);
>+            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 +542,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;
>diff --git a/Websites/perf.webkit.org/public/v3/models/commit-log.js b/Websites/perf.webkit.org/public/v3/models/commit-log.js
>index d9bd438d151b3ecf2dbbda1a240d81a665a621e8..11ee1c90ef223c25857bd29255cdd1a0e2b81410 100644
>--- a/Websites/perf.webkit.org/public/v3/models/commit-log.js
>+++ b/Websites/perf.webkit.org/public/v3/models/commit-log.js
>@@ -172,13 +172,13 @@ 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}`);
>+        const data = await this.cachedFetch(`/api/commits/${repository.id()}/${revision}${prefixMatch ? '?prefix-match=true' : ''}`);
>         return this._constructFromRawData(data);
>     }
> 
>diff --git a/Websites/perf.webkit.org/server-tests/api-commits-tests.js b/Websites/perf.webkit.org/server-tests/api-commits-tests.js
>index 324962ad533ca473781d07418a7105174a5b157d..1bf9461434ef5f5e93e29c4a7f1f578811eb3295 100644
>--- a/Websites/perf.webkit.org/server-tests/api-commits-tests.js
>+++ b/Websites/perf.webkit.org/server-tests/api-commits-tests.js
>@@ -394,6 +394,42 @@ 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 handle commit revision with space", () => {
>             const db = TestServer.database();
>             return Promise.all([
>diff --git a/Websites/perf.webkit.org/unit-tests/commit-log-tests.js b/Websites/perf.webkit.org/unit-tests/commit-log-tests.js
>index 86a1263301380ef053b87b4c17a06b3138171370..d3a0904d6bb44a36e0fa7bc695f3a2641edaecc4 100644
>--- a/Websites/perf.webkit.org/unit-tests/commit-log-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/commit-log-tests.js
>@@ -539,5 +539,12 @@ 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`);
>+        })
>     });
> });

Flags:

rniwa: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198847: 372097