[webkit-reviews] review granted: [Bug 197928] Build type build request should work even there is no repository in triggerable repository group accepts patch. : [Attachment 369996] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 197928: Build type build request should work even there is no repository in
triggerable repository group accepts patch.
https://bugs.webkit.org/show_bug.cgi?id=197928

Attachment 369996: Patch

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




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

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

> Websites/perf.webkit.org/ChangeLog:3
> +	   Build type build request builds owned components should work even
there is no repository in triggerable repository group accepts patch.

This title is misleading. What we want to do is to allow build request when a
revision when there are no patches.
How about this: perf dashboard erroneously rejects a build request to build
owned components when there are no patches

> Websites/perf.webkit.org/ChangeLog:8
> +	   Fix a bug that build type build request that only builds owned
components failed to pass sanity check when there is no repository accepts
patch in triggerable repository group.

Please wrap this line at some point. It's too long.

> Websites/perf.webkit.org/ChangeLog:9
> +	   Add a sanity check to raise an error when build request type is
build but there is no repository group template.

Nit: throw an error.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1048
> +	   it('should allow to build owned component even no repository accepts
patch in the triggerable repository group', () => {

Nit: build *with a* owned component even *when/if* no repository accepts *a*
patch

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1063
> +	       const owner111289 = CommitLog.ensureSingleton('111289', {'id':
'111289', 'time': 1456931874000, 'repository': MockModels.ownerRepository,
'revision': 'owner-001'});
> +	       const owned111222 = CommitLog.ensureSingleton('111222', {'id':
'111222', 'time': 1456932774000, 'repository': MockModels.ownedRepository,
'revision': 'owned-002'});

Can we just call these owner 1 and owner 2?

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1065
> +	       const request = BuildRequest.ensureSingleton(`123123`,
{'triggerable': MockModels.triggerable,

Seems like can just use regular single quotation marks here.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1085
> +	       const owner111289 = CommitLog.ensureSingleton('111289', {'id':
'111289', 'time': 1456931874000, 'repository': MockModels.ownerRepository,
'revision': 'owner-001'});
> +	       const owned111222 = CommitLog.ensureSingleton('111222', {'id':
'111222', 'time': 1456932774000, 'repository': MockModels.ownedRepository,
'revision': 'owned-002'});

Ditto.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1087
> +	       const request = BuildRequest.ensureSingleton(`123123`,
{'triggerable': MockModels.triggerable,

Ditto.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1091
> +	       try {

Use assert.throws:
https://nodejs.org/api/assert.html#assert_assert_throws_fn_error_message