[webkit-reviews] review denied: [Bug 222897] [perf dashboard] Add commit identifier support : [Attachment 422619] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 8 17:38:03 PST 2021


Ryosuke Niwa <rniwa at webkit.org> has denied Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 222897: [perf dashboard] Add commit identifier support
https://bugs.webkit.org/show_bug.cgi?id=222897

Attachment 422619: Patch

https://bugs.webkit.org/attachment.cgi?id=422619&action=review




--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 422619
  --> https://bugs.webkit.org/attachment.cgi?id=422619
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422619&action=review

> Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:137
> +	      
expect(viewer.content('spinner-container').offsetHeight).to.not.be(0);
> +	       await waitForComponentsToRender(context);

What is this test change about? It should be explained in the change log.

> Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:142
> -	      
expect(viewer.content('commits-list').textContent).to.contain('r210950');
> +	      
expect(viewer.content('commits-list').textContent).to.contain('184278 at main
(r210950)');

Please add a new test instead of mutating an existing test case.

> Websites/perf.webkit.org/init-database.sql:105
> +    commit_identifier varchar(64) DEFAULT NULL,

Calling this "identifier" is a bit confusing because we also have commit_id.
Maybe commit_revision_identifier?

> Websites/perf.webkit.org/migrate-database.sql:19
> +    ALTER TABLE commits ADD COLUMN IF NOT EXISTS commit_identifier;
> +    ALTER TABLE commits DROP CONSTRAINT
commit_identifier_in_repository_must_be_unique;
> +    ALTER TABLE commits ADD CONSTRAINT
commit_identifier_in_repository_must_be_unique UNIQUE(commit_repository,
commit_identifier);

r-. This can't be right. This migration is nothing to do with having
build_number.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:9
>      static function find_commit_id_by_revision($db, $repository_id,
$revision)

Huh, I think this function is really a duplicate of fetch_revision.
Not sure how we ended up with two functions here.
We should probably clean this up in a separate patch.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> +	   $column_name = 'revision';
> +	   if ($this->is_commit_identifier($revision))
> +	       $column_name = 'identifier';

Please put the whole column name into the variable.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:201
> +    private function is_commit_identifier($commit_revision_or_identifier) {
> +	   return preg_match('/\d+@\S+/', $commit_revision_or_identifier);

I don't think \S+ is right. We want 1234@ is match 1234 at main.
Or are we doing that transformation in the frontend?

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:208
> -	   $commit_info = array('repository' => $repository_id, 'revision' =>
$revision);
> +	   if (!$this->is_commit_identifier($revision))
> +	       $commit_info = array('repository' => $repository_id, 'revision'
=> $revision);
> +	   else 
> +	       $commit_info = array('repository' => $repository_id,
'identifier' => $revision);

Looks like this can just be:
$commit_info = array('repository' => $repository_id);
$commit_info[$this->is_commit_identifier($revision) ? 'identifier' :
'revision'] = $revision;

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:218
> +	   $column_name = 'revision';
> +	   if ($this->is_commit_identifier($revision_prefix))
> +	       $column_name = 'identifier';

Please use a ternary expression here as well.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:220
> +	   $rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE
commit_repository = $1 AND commit_'.$column_name.' LIKE $2 ORDER BY
commit_'.$column_name.' LIMIT 2', array($repository_id,
Database::escape_for_like($revision_prefix) . '%'));

Nit: Need spaces around each ".".

> Websites/perf.webkit.org/public/include/commit-updater.php:89
> +		   $commit_identifiers[$commit_info['identifier']] = true;
> +	       }

We should also verify the format of identifier.
r- due to the lack of the check here and the lack of tests for that check.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:39
>      testability() { return this._rawData['testability']; }

We also need to update CommitLog.prototype.updateSingleton
so that we can either assert that commit identifiers don't change or update it
dynamically.
r- due to this missing code and the lack of tests thereof.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:42
> +    identifier() { return this._rawData['identifier']}

Same issue here. It's confusing to have id() and identifier().
Probably we should call it revisionIdentifier().
Also nit: missing ; and a space at the end.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:56
> +	   let formatted_revision = revision;

Nit: Use camelCase.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:66
>  

We also need to update diff(previousCommit).
r- due to the lack of change there and test cases thereof.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:533
> +	   it("should return commit with commit identifier", () => {
> +	       return addSlaveForReport(subversionCommits).then(() => {

Use async & await.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:756
> +	       ]).then(() => {
> +		   return
TestServer.remoteAPI().getJSON('/api/commits/WebKit/?precedingRevision=184276 at m
ain&lastRevision=184278 at main');

Ditto.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:806
> +	       return Promise.all([
> +		   db.insert('repositories', {'id': 1, 'name': 'WebKit'}),

Ditto.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:825
>      });

We need tests for when querying with a partial match.
Also, we need to check that, latest, oldest, etc... all support the commit
identifiers.
r- due to the lack of these test cases.

> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:172
> +    it("should reject with duplicated commit identifier", () => {
> +	   return addSlaveForReport(subversionCommit).then(() => {

Use async & await.

> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:181
> +

We also need tests for updating commit identifiers on an existing commit,
both an existing commit doesn't have any commit identifier and when it already
has some other commit.


More information about the webkit-reviews mailing list