REGRESSION(r198234): /api/commits/%revision% always fails
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jan 2017 22:02:09 +0000 (22:02 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jan 2017 22:02:09 +0000 (22:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167235

Reviewed by Antti Koivisto.

The bug was caused by a typo in CommitLogFetcher::fetch_revision, which was calling commit_for_revision on
$this->db instead of $this. This had been monkey-patched in the internal dashboard so it was working there.

Also fixed a bug that /latest wasn't doing what it claimed to do, and a bug that /oldest /latest,
and /last-reported would return a commit with all values set to null instead of an empty list.

Finally, added server API tests for /api/commits.

* public/api/commits.php:
(main): Add a comment for APIs that only exist for v2 UI.

* public/include/commit-log-fetcher.php:
(CommitLogFetcher::fetch_latest): Fixed the bug that this function was returning the oldest commit, not the
the latest commit as desired.
(CommitLogFetcher::fetch_revision): Fixed the bug that this function would always encounter an exception
because commit_for_revision is defined on $this, not $this->db.
(CommitLogFetcher::format_single_commit): Return an empty list instead of an array with a single commit with
all values set to null.

* server-tests/api-commits.js: Added. Added tests for the JSON API at /api/commits.
(.assertCommitIsSameAsOneSubmitted): Added. A helper function to compare a commit returned by /api/commits
to one sent to /api/report-commits.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/api/commits.php
Websites/perf.webkit.org/public/include/commit-log-fetcher.php
Websites/perf.webkit.org/server-tests/api-commits.js [new file with mode: 0644]

index 429d84a..c4f290c 100644 (file)
@@ -1,3 +1,33 @@
+2017-01-20  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION(r198234): /api/commits/%revision% always fails
+        https://bugs.webkit.org/show_bug.cgi?id=167235
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by a typo in CommitLogFetcher::fetch_revision, which was calling commit_for_revision on
+        $this->db instead of $this. This had been monkey-patched in the internal dashboard so it was working there.
+
+        Also fixed a bug that /latest wasn't doing what it claimed to do, and a bug that /oldest /latest,
+        and /last-reported would return a commit with all values set to null instead of an empty list.
+
+        Finally, added server API tests for /api/commits.
+
+        * public/api/commits.php:
+        (main): Add a comment for APIs that only exist for v2 UI.
+
+        * public/include/commit-log-fetcher.php:
+        (CommitLogFetcher::fetch_latest): Fixed the bug that this function was returning the oldest commit, not the
+        the latest commit as desired.
+        (CommitLogFetcher::fetch_revision): Fixed the bug that this function would always encounter an exception
+        because commit_for_revision is defined on $this, not $this->db.
+        (CommitLogFetcher::format_single_commit): Return an empty list instead of an array with a single commit with
+        all values set to null.
+
+        * server-tests/api-commits.js: Added. Added tests for the JSON API at /api/commits.
+        (.assertCommitIsSameAsOneSubmitted): Added. A helper function to compare a commit returned by /api/commits
+        to one sent to /api/report-commits.
+
 2017-01-19  Ryosuke Niwa  <rniwa@webkit.org>
 
         measurement-sets API can incorrectly order points with OS version without commit time
index d5da8d4..882c7eb 100644 (file)
@@ -23,7 +23,7 @@ function main($paths) {
     $filter = array_get($paths, 1);
     $commits = array();
     if (!$filter) {
-        $keyword = array_get($_GET, 'keyword');
+        $keyword = array_get($_GET, 'keyword'); // V2 UI compatibility.
         $from = array_get($_GET, 'from');
         $to = array_get($_GET, 'to');
         $commits = $fetcher->fetch_between($repository_id, $from, $to, $keyword);
@@ -35,7 +35,7 @@ function main($paths) {
         $commits = $fetcher->fetch_last_reported($repository_id);
     } else if (ctype_alnum($filter)) {
         $commits = $fetcher->fetch_revision($repository_id, $filter);
-    } else {
+    } else { // V2 UI compatibility.
         $matches = array();
         if (!preg_match('/([A-Za-z0-9]+)[\:\-]([A-Za-z0-9]+)/', $filter, $matches))
             exit_with_error('UnknownFilter', array('repositoryName' => $repository_name, 'filter' => $filter));
index 0bb73fb..8e710b6 100644 (file)
@@ -88,7 +88,7 @@ class CommitLogFetcher {
     }
 
     function fetch_latest($repository_id) {
-        return $this->format_single_commit($this->db->select_first_row('commits', 'commit', array('repository' => $repository_id), 'time'));
+        return $this->format_single_commit($this->db->select_last_row('commits', 'commit', array('repository' => $repository_id), 'time'));
     }
 
     function fetch_last_reported($repository_id) {
@@ -96,7 +96,7 @@ class CommitLogFetcher {
     }
 
     function fetch_revision($repository_id, $revision) {
-        return $this->format_single_commit($this->db->commit_for_revision($repository_id, $revision));
+        return $this->format_single_commit($this->commit_for_revision($repository_id, $revision));
     }
 
     private function commit_for_revision($repository_id, $revision) {
@@ -111,6 +111,8 @@ class CommitLogFetcher {
     }
 
     private function format_single_commit($commit_row) {
+        if (!$commit_row)
+            return array();
         $committer = $this->db->select_first_row('committers', 'committer', array('id' => $commit_row['commit_committer']));
         return array($this->format_commit($commit_row, $committer));
     }
diff --git a/Websites/perf.webkit.org/server-tests/api-commits.js b/Websites/perf.webkit.org/server-tests/api-commits.js
new file mode 100644 (file)
index 0000000..a914653
--- /dev/null
@@ -0,0 +1,334 @@
+'use strict';
+
+const assert = require('assert');
+
+const TestServer = require('./resources/test-server.js');
+const addSlaveForReport = require('./resources/common-operations.js').addSlaveForReport;
+const connectToDatabaseInEveryTest = require('./resources/common-operations.js').connectToDatabaseInEveryTest;
+
+describe("/api/commits/", () => {
+    TestServer.inject();
+    connectToDatabaseInEveryTest();
+
+    const subversionCommits = {
+        "slaveName": "someSlave",
+        "slavePassword": "somePassword",
+        "commits": [
+            {
+                "repository": "WebKit",
+                "revision": "210948",
+                "time": "2017-01-20T02:52:34.577Z",
+                "author": {"name": "Zalan Bujtas", "account": "zalan@apple.com"},
+                "message": "a message",
+            },
+            {
+                "repository": "WebKit",
+                "revision": "210949",
+                "time": "2017-01-20T03:23:50.645Z",
+                "author": {"name": "Chris Dumez", "account": "cdumez@apple.com"},
+                "message": "some message",
+            },
+            {
+                "repository": "WebKit",
+                "parent": "210949",
+                "revision": "210950",
+                "time": "2017-01-20T03:49:37.887Z",
+                "author": {"name": "Commit Queue", "account": "commit-queue@webkit.org"},
+                "message": "another message",
+            }
+        ]
+    }
+
+    const notYetReportedCommit = {
+        revision: '210951',
+        time: '2017-01-20T03:56:20.045Z'
+    }
+
+    function assertCommitIsSameAsOneSubmitted(commit, submitted)
+    {
+        assert.equal(commit['revision'], submitted['revision']);
+        assert.equal(new Date(commit['time']).toISOString(), submitted['time']);
+        assert.equal(commit['message'], submitted['message']);
+        assert.equal(commit['authorName'], submitted['author']['name']);
+        assert.equal(commit['authorEmail'], submitted['author']['account']);
+    }
+
+    describe('/api/commits/<repository>/', () => {
+        it("should return RepositoryNotFound when there are no matching repository", () => {
+            return TestServer.remoteAPI().getJSON('/api/commits/WebKit').then((response) => {
+                assert.equal(response['status'], 'RepositoryNotFound');
+            });
+        });
+
+        it("should return an empty result when there are no reported commits", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'})
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return the list of all commits for a given repository", () => {
+            return addSlaveForReport(subversionCommits).then(() => {
+                return TestServer.remoteAPI().postJSON('/api/report-commits/', subversionCommits);
+            }).then(function (response) {
+                assert.equal(response['status'], 'OK');
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/');
+            }).then(function (result) {
+                assert.equal(result['status'], 'OK');
+
+                const commits = result['commits'];
+                assert.equal(commits.length, 3);
+                const submittedCommits = subversionCommits['commits'];
+                assertCommitIsSameAsOneSubmitted(commits[0], submittedCommits[0]);
+                assertCommitIsSameAsOneSubmitted(commits[1], submittedCommits[1]);
+                assertCommitIsSameAsOneSubmitted(commits[2], submittedCommits[2]);
+            });
+        });        
+    });
+
+    describe('/api/commits/<repository>/oldest', () => {
+        it("should return RepositoryNotFound when there are no matching repository", () => {
+            return TestServer.remoteAPI().getJSON('/api/commits/WebKit/oldest').then((response) => {
+                assert.equal(response['status'], 'RepositoryNotFound');
+            });
+        });
+
+        it("should return an empty results when there are no commits", () => {
+            return TestServer.database().insert('repositories', {'id': 1, 'name': 'WebKit'}).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/oldest');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return the oldest commit", () => {
+            const remote = TestServer.remoteAPI();
+            return addSlaveForReport(subversionCommits).then(() => {
+                return remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            }).then(() => {
+                return remote.getJSON('/api/commits/WebKit/oldest');
+            }).then(function (result) {
+                assert.equal(result['status'], 'OK');
+                assert.equal(result['commits'].length, 1);
+                assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'][0]);
+            });
+        });
+    });
+
+    describe('/api/commits/<repository>/latest', () => {
+        it("should return RepositoryNotFound when there are no matching repository", () => {
+            return TestServer.remoteAPI().getJSON('/api/commits/WebKit/latest').then((response) => {
+                assert.equal(response['status'], 'RepositoryNotFound');
+            });
+        });
+
+        it("should return an empty results when there are no commits", () => {
+            return TestServer.database().insert('repositories', {'id': 1, 'name': 'WebKit'}).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/latest');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return the oldest commit", () => {
+            const remote = TestServer.remoteAPI();
+            return addSlaveForReport(subversionCommits).then(() => {
+                return remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            }).then(() => {
+                return remote.getJSON('/api/commits/WebKit/latest');
+            }).then(function (result) {
+                assert.equal(result['status'], 'OK');
+                assert.equal(result['commits'].length, 1);
+                assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'].slice().pop());
+            });
+        });
+    });
+
+    describe('/api/commits/<repository>/last-reported', () => {
+        it("should return RepositoryNotFound when there are no matching repository", () => {
+            return TestServer.remoteAPI().getJSON('/api/commits/WebKit/last-reported').then((response) => {
+                assert.equal(response['status'], 'RepositoryNotFound');
+            });
+        });
+
+        it("should return an empty result when there are no reported commits", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'})
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/last-reported');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return an empty results when there are no reported commits", () => {
+            return TestServer.database().insert('repositories', {'id': 1, 'name': 'WebKit'}).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/last-reported');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return the oldest reported commit", () => {
+            const db = TestServer.database();
+            const remote = TestServer.remoteAPI();
+            return Promise.all([
+                addSlaveForReport(subversionCommits),
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': notYetReportedCommit.revision, 'time': notYetReportedCommit.time}),
+            ]).then(() => {
+                return remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            }).then(() => {
+                return remote.getJSON('/api/commits/WebKit/last-reported');
+            }).then(function (result) {
+                assert.equal(result['status'], 'OK');
+                assert.equal(result['commits'].length, 1);
+                assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'].slice().pop());
+            });
+        });
+    });
+
+    describe('/api/commits/<repository>/<commit>', () => {
+        it("should return RepositoryNotFound when there are no matching repository", () => {
+            return TestServer.remoteAPI().getJSON('/api/commits/WebKit/210949').then((response) => {
+                assert.equal(response['status'], 'RepositoryNotFound');
+            });
+        });
+
+        it("should return UnknownCommit when one of the specified commit does not exist in the database", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'})
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/210949');
+            }).then((response) => {
+                assert.equal(response['status'], 'UnknownCommit');
+            });
+        });
+
+        it("should return the commit even if it had not been reported", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'})
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/210950');
+            }).then((result) => {
+                assert.equal(result['status'], 'OK');
+                assert.equal(result['commits'].length, 1);
+                assertCommitIsSameAsOneSubmitted(result['commits'][0], {
+                    parent: null,
+                    revision: '210950',
+                    time: '2017-01-20T03:49:37.887Z',
+                    author: {name: null, account: null},
+                    message: null,
+                });
+            });
+        });
+
+        it("should return the full result for a reported commit", () => {
+            const remote = TestServer.remoteAPI();
+            return addSlaveForReport(subversionCommits).then(() => {
+                return remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            }).then(() => {
+                return remote.getJSON('/api/commits/WebKit/210949');
+            }).then((result) => {
+                assert.equal(result['status'], 'OK');
+                assert.deepEqual(result['commits'].length, 1);
+                assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'][1]);
+            });
+        });
+
+    });
+
+    describe('/api/commits/<repository>/?from=<commit-1>&to=<commit-2>', () => {
+        it("should return RepositoryNotFound when there are no matching repository", () => {
+            return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210900&to=211000').then((response) => {
+                assert.equal(response['status'], 'RepositoryNotFound');
+            });
+        });
+
+        it("should return UnknownCommit when one of the specified commit does not exist in the database", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'})
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210900&to=211000');
+            }).then((response) => {
+                assert.equal(response['status'], 'UnknownCommit');
+            });
+        });
+
+        it("should return an empty result when commits in the specified range have not reported", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210949', 'time': '2017-01-20T03:23:50.645Z'}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210949&to=210950');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return reported commits in the specified range", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210949', 'time': '2017-01-20T03:23:50.645Z', 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z', 'reported': true}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210949&to=210950');
+            }).then((result) => {
+                assert.equal(result['status'], 'OK');
+                assert.deepEqual(result['commits'].length, 2);
+                assertCommitIsSameAsOneSubmitted(result['commits'][0], {
+                    parent: null,
+                    revision: '210949',
+                    time: '2017-01-20T03:23:50.645Z',
+                    author: {name: null, account: null},
+                    message: null,
+                });
+                assertCommitIsSameAsOneSubmitted(result['commits'][1], {
+                    parent: null,
+                    revision: '210950',
+                    time: '2017-01-20T03:49:37.887Z',
+                    author: {name: null, account: null},
+                    message: null,
+                });
+            });
+        });
+
+        it("should not include a revision not within the specified range", () => {
+            const remote = TestServer.remoteAPI();
+            return addSlaveForReport(subversionCommits).then(() => {
+                return remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
+            }).then(() => {
+                return remote.getJSON('/api/commits/WebKit/?from=210948&to=210949');
+            }).then((result) => {
+                assert.equal(result['status'], 'OK');
+                assert.deepEqual(result['commits'].length, 2);
+                assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'][0]);
+                assertCommitIsSameAsOneSubmitted(result['commits'][1], subversionCommits['commits'][1]);
+            });
+        });
+
+    });
+
+});