[webkit-reviews] review granted: [Bug 229545] Add 'paired-parallel' repetition type for A/B testing. : [Attachment 442903] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 229545: Add 'paired-parallel' repetition type for A/B testing.
https://bugs.webkit.org/show_bug.cgi?id=229545

Attachment 442903: Patch

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




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

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

> Websites/perf.webkit.org/ChangeLog:9
> +	   Add 'TriggerableConfiguration' model to store repetition types
information for each (platform, test) pair.

Nit: store repetition *type* information

> Websites/perf.webkit.org/ChangeLog:12
> +	   schedule A/B testing for builder supported repetition types.

Nit: schedule A/B testing on a builder which supports a given repetition type.

> Websites/perf.webkit.org/migrate-database.sql:66
> +ALTER TYPE analysis_test_group_repetition_type ADD VALUE IF NOT EXISTS
'paired-parallel';

This is very nice!

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:86
> +		   exit_with_error('UnsupportedRepetitionType',
array('repetitionType' => $repetition_type));

Let's also emit the triggerable ID. Otherwise, this error would be
undistingushiable from the above error.
Maybe also rename this to UnsupportedRepetitionTypeForTriggerable?

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:24
> +	   this.#selectedRepetitionType = repetitionType;

We should schedule to render here.

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:30
> +	   this.content('repetition-type').disabled = value;

This should be done in render() function.

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:37
> +	   if (!this.#triggerableConfiguration)
> +	       this.#selectedRepetitionType = null;

Can we turn this into an early return?

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:38
> +	   else if (this.#triggerableConfiguration &&
!this.#triggerableConfiguration.isSupportedRepetitionType(this.#selectedRepetit
ionType))

That way, we don't have to check the nullity for the second time here.

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:40
> +    }

We should schedule to render here.

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:55
> +	   if (!this.#triggerableConfiguration)
> +	       return;

Wouldn't this leave the stale list of repetition types?
I think we should clear out select elements regardless of whether we have
#triggerableConfiguration or not.

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:56
> +	   this.#renderRepetitionTypeListLazily.evaluate(
this.#selectedRepetitionType,
...this.#triggerableConfiguration.supportedRepetitionTypes);

So do this: (this.#triggerableConfiguration?.supportedRepetitionTypes || [])

> Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:62
> +	       this.createElement('option',{ selected: repetitionType ==
selectedRepetitionType, value: repetitionType },

Nit: Need a space between , and {.

> Websites/perf.webkit.org/public/v3/components/test-group-form.js:25
> +	   if (!testGroup)
> +	       return;

Why are we ignoring null? This will mean we'd leave the stale result, right?
We should clear out values and schedule to render even when testGroup is null.
If this is not expected to happen, then we should simply assert that instead of
exiting early.

> Websites/perf.webkit.org/public/v3/models/triggerable.js:13
>	   for (const config of object.configurations) {

Maybe we should just do this:
this._triggerableConfigurationList = object.configurations.map((config) => {
    this._acceptedTests.add(config.test);
    return TriggerableConfiguration.createForTriggerable(this, config.test,
config.platform, config.supportedRepetitionTypes);
});

> Websites/perf.webkit.org/public/v3/models/triggerable.js:26
> +    static findByTestConfiguration(test, platform) { return
TriggerableConfiguration.findByTestAndPlatform(test, platform)?.triggerable; }

Nice!

> Websites/perf.webkit.org/public/v3/models/triggerable.js:60
> +    get supportedRepetitionTypes() { return this.#supportedRepetitionTypes;
}

Instead of returning the internal array (which allows mutations by the caller),
can we return a copy like so:
return [...this.#supportedRepetitionTypes];

> Websites/perf.webkit.org/public/v3/models/triggerable.js:68
> +	   return TriggerableConfiguration.ensureSingleton(id, { test,
platform, supportedRepetitionTypes, triggerable });

I don't think we normally places a space at the beginning and at the end of { ~
} for a dictionary like this?

> Websites/perf.webkit.org/server-tests/api-update-triggerable-tests.js:480
> +	   const initialUpdate =
createUpdateWithRepetitionTypes(['alternating', 'sequential']);
> +	   await addWorkerForReport(initialUpdate);
> +	   await
TestServer.remoteAPI().postJSONWithStatus('/api/update-triggerable',
initialUpdate);

Maybe also test an empty array or not specifying it at all and make sure it
doesn't blow up?

>
Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests
.js:882
> +    it('should reject with "UnsupportedRepetitionType" when
"paired-parallel" is not supported under triggerable configuration', async ()
=> {

Nit: Maybe just "is not supported by the triggerable"?

>
Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:
17
> +function configWithOneTesterTwoBuilders(testConfigurationsOverride=[{types:
['some'], platforms: ['some platform'], builders: ['builder-1'],

Nit: Need a space around =.

>
Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:
18
> +    supportedRepetitionTypes: ['alternating', 'sequential']}]) {

Nit: { should be on the next line.

>
Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:
712
> +    it('should respected "supportedRepetitionTypes" specified by build and
test configurations', async () => {

Did you mean something like this?: should be able to schedule a
"paired-parallel" build request for building a patch on buildbot

This test is getting so long that we should probably add some helper function
to shorten it so that can understand the core logic of the test.
Also, don't we need a test for syncing script taking "build" request but not
"test" request?

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:398
> -	       for (const type of entry['types']) {
> +	       for (let type of entry['types']) {

Keep const?

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:87
> +	   await Promise.all(Array.from(testGroupIds).map(testGroupId =>
TestGroup.fetchById(testGroupId, /* ignoreCache */ true)));

Why did we have to move this here?
Please explain in the change log.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1806
> +	       const syncer = BuildbotSyncer._loadConfig(MockRemoteAPI,
smallConfigurationWithCustomRepetitionTypes(), builderNameToIDMap())[0];

Maybe smallConfigurationWithCustomRepetitionTypes should take an array as an
input?

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1809
> +	       syncer.scheduleRequestInGroupIfAvailable(request, [request],
null);

Check that this function returns null?