Bug 197928

Summary: Build type build request should work even there is no repository in triggerable repository group accepts patch.
Product: WebKit Reporter: dewei_zhu
Component: Tools / TestsAssignee: dewei_zhu
Status: NEW ---    
Severity: Normal CC: dewei_zhu, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch rniwa: review+

Description dewei_zhu 2019-05-15 15:02:28 PDT
Build type build request should work even there is no repository in triggerable repository group accepts patch.
Comment 1 dewei_zhu 2019-05-15 15:08:06 PDT
Created attachment 369996 [details]
Patch
Comment 2 dewei_zhu 2019-05-15 15:11:01 PDT
<rdar://problem/45558814>
Comment 3 Ryosuke Niwa 2019-05-21 15:16:00 PDT
Comment on attachment 369996 [details]
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