Add 'parallel' repetition type for A/B testing.
Created attachment 436484 [details] Patch
Comment on attachment 436484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436484&action=review As a high level comment, there seems to be a confusion as to which class / object / abstraction is responsible for knowing what types of repetitions are supported. Is it per triggerable? Is it per test-platform pair? We need to stick to one model and implement accordingly. Secondly, we really need to minimize the number of places where we define the default set of repetition types. Ideally, it would be only in the database but there is a fair bit of custom code throughout frontend & backend so that might be hard. But we should try to consolidate to few key places. If this concept is defined per Triggerable, perhaps Triggerable should know what repetition types are supported. > Websites/perf.webkit.org/ChangeLog:3 > + Add 'parallel' repetition type for A/B testing. Use double quotation marks? > Websites/perf.webkit.org/ChangeLog:8 > + Add 'parallel' repetition type for A/B testing. Hm... maybe parallel isn't really a good internal name depending on what we're gonna do with this. > Websites/perf.webkit.org/ChangeLog:9 > + Tied repetition type to triggerable configuraion. Tied each repetition type to a specific triggerable configuration? I think you should mention that this patch also introduces a ID for each test configuration > Websites/perf.webkit.org/init-database.sql:278 > + triggerable_config INTEGER NOT NULL REFERENCES triggerable_configurations ON DELETE CASCADE, > + config_repetition_type analysis_test_group_repetition_type NOT NULL, For consistency, let's call these configrepetition_config configrepetition_type But why does this need to be per configuation given we only support one set of repetition types per triggerable? > Websites/perf.webkit.org/migrate-database.sql:59 > \ No newline at end of file Need a new line at the end. > Websites/perf.webkit.org/public/api/update-triggerable.php:47 > + if(!$db->insert_row('triggerable_configuration_repetition_types', NULL, Nit: Need a space between if and (. > Websites/perf.webkit.org/public/include/json-header.php:198 > + $triggerable_to_config_id = array(); > + foreach ($results as $row) > + $triggerable_to_config_id[$row['triggerable']] = $row['config_id']; I'm a bit confused here. Are we assuming that the repetition types for different configurations are all the same? Doesn't that mean all configurations for each triggerable support the same set of repetition types? If so, we're overcomplicating this by adding new table per triggerable configuration. Otherwise, I don't think this code makes much sense since some repetition type may not be available for whatever test/platform pair we happen to pick. > Websites/perf.webkit.org/public/include/manifest-generator.php:279 > + if (!array_key_exists($row['trigconfig_id'], $repetition_types_by_config)) > + $repetition_types = array('alternating', 'sequential'); > + else > + $repetition_types = $repetition_types_by_config[$row['trigconfig_id']]; Let's not do this during manifest generation. That's highly confusing! We should add this to the database and add a migration rule. > Websites/perf.webkit.org/public/v3/components/custom-configuration-test-group-form.js:58 > + const supportedRepetitionTypes = Triggerable.supportedRepetitionTypesByConfiguration(tests[0], platform); I'd call this: supportedRepetitionTypes*For*Configuration. > Websites/perf.webkit.org/public/v3/components/test-group-form.js:8 > + this._supportedRepetitionTypes = ['alternating', 'sequential']; I don't think we want this kind of hard-coding in UI code. We should get this information from the backend instead. > Websites/perf.webkit.org/public/v3/components/test-group-form.js:26 > + setSupportedRepetitionTypes(supportedRepetitionTypes) > + { > + this._supportedRepetitionTypes = supportedRepetitionTypes; It's very confusing that this is only used by a specific subclass. Maybe a better design is to let a subclass override the repetition part of the UI. Yet another alternative is to have a function which returns a set of supported types and a subclass override it. Also, we need to validate that it's one of the three types we know of. > Websites/perf.webkit.org/public/v3/models/manifest.js:91 > + const [testId, platformId, repetitionTypes] = configuration; > + return {test: Test.findById(testId), platform: Platform.findById(platformId), repetitionTypes}; supportedRepetitionTypes? > Websites/perf.webkit.org/public/v3/models/test-group.js:350 > + const {retryCount, stopFutureRetries} = await (this.repetitionType() == 'sequential' ? > + this._createSequentialRetriesForTestGroup(repetitionLimit): this._createAlternatingRetriesForTestGroup(repetitionLimit)); Why are we making this change? Please explain it in the change log. > Websites/perf.webkit.org/public/v3/models/triggerable.js:22 > + console.assert(!(key in supportedRepetitionTypesMap)) > + supportedRepetitionTypesMap[key] = config.repetitionTypes; Why are we overriding it? Don't we need to take the union of what we have already with new ones? > Websites/perf.webkit.org/public/v3/models/triggerable.js:33 > + static supportedRepetitionTypesByConfiguration(test, platform) > + { Either this doesn't make sense to be static because it's specific to each triggerable or the above comment needs to be addressed. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:134 > + const supportedRepetitionTypes = Triggerable.supportedRepetitionTypesByConfiguration(testGroup.test(), testGroup.platform()); > + form.setSupportedRepetitionTypes(supportedRepetitionTypes); Why is this AnalysisTaskResultsPane's responsibility? It seems like there is quite a bit of confusion as to which class is responsible for figuring out what repetition types are supported. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:382 > + const retryForm = this.part('retry-form'); > + retryForm.setRepetitionCount(currentGroup.initialRepetitionCount()); > + retryForm.setSupportedRepetitionTypes(supportedRepetitionTypes); > + retryForm.setRepetitionType(currentGroup.repetitionType()); > + retryForm.enqueueToRender(); Let's not repeat all this code multiple times. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:107 > + this._repetitionType = 'alternating'; Again, the number of types I've seen either this default or the list of default repetition types is alarming. This kind of magic string spread across multiple files, subsystems, and components will inevitably lead to bugs. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:351 > + const explanationByRepetitionTypes = [['alternating', 'alternate (ABAB)'], ['sequential', 'sequence (AABB)'], > + ['parallel', 'parallel']].filter((entry) => repetitionTypes.includes(entry[0])) Ditto. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:90 > + assert(Array.isArray(repetitionTypes)); Let's assert the values too. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:114 > + return this._configurations.some((config) => config.platform == request.platform() && config.test == request.test() > + && (!this.isTester() || (config.repetitionTypes.includes(request.testGroup().repetitionType()) > + && this._supportedTestRepetitionTypes.includes(request.testGroup().repetitionType())))); This is getting way too long to be an inline function. Please use { ~ } and if statements to make it clear. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:395 > + if (Array.isArray(type)) { Why does this need to be an array instead of a regular property in type?
Comment on attachment 436484 [details] Patch r- because there are enough design issues & code that's highly suspicious of major bugs that I don't think this path is landable as is.
Comment on attachment 436484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436484&action=review >> Websites/perf.webkit.org/public/include/json-header.php:198 >> + $triggerable_to_config_id[$row['triggerable']] = $row['config_id']; > > I'm a bit confused here. > Are we assuming that the repetition types for different configurations are all the same? > Doesn't that mean all configurations for each triggerable support the same set of repetition types? > If so, we're overcomplicating this by adding new table per triggerable configuration. > Otherwise, I don't think this code makes much sense > since some repetition type may not be available for whatever test/platform pair we happen to pick. Nice catch, what I meant to do is ``` $test_to_trigconfig = array(); foreach ($results as $row) $test_to_trigconfig[$row['test']] = $row['config_id']; ``` >> Websites/perf.webkit.org/public/include/manifest-generator.php:279 >> + $repetition_types = $repetition_types_by_config[$row['trigconfig_id']]; > > Let's not do this during manifest generation. That's highly confusing! > We should add this to the database and add a migration rule. Sure. >> Websites/perf.webkit.org/public/v3/models/triggerable.js:33 >> + { > > Either this doesn't make sense to be static because it's specific to each triggerable or the above comment needs to be addressed. Each triggerable_configurations is keyed by (test, platform), so supported repetition types can be determined by (test, platform) pair.
<rdar://problem/82665951>
Created attachment 441865 [details] Patch
Comment on attachment 441865 [details] Patch Looks like all feedback was addressed.
Comment on attachment 441865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441865&action=review > Websites/perf.webkit.org/public/include/json-header.php:206 > + $rows = $db->select_rows('triggerable_configuration_repetition_types', 'configrepetition', array('config' => $config_id)); > + foreach ($rows as $row) > + array_push($supported_repetition_types, $row['configrepetition_type']); This will run the query for every test. Why not do this in the above foreach loop? > Websites/perf.webkit.org/public/include/manifest-generator.php:266 > + foreach($repetition_types_by_configuration as &$row) Nit: Need a space between foreach and (. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:44 > add_alternating_build_requests($db, $existing_test_type_build_requests, $additional_build_request_count, $current_order); Does it really make sense for build requests to be alternating when requests are parallelized? > Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:4 > + #triggerableConfiguration; > + #selectedRepetitionType; > + #renderRepetitionTypeListLazily; Nice! > Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:27 > + set disabled(value) {this.content('repetition-type').disabled = value;} Nit: Need spaces around { ~ }; Maybe we should ensure the value is a boolean? > Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:33 > + if (this.#triggerableConfiguration && !this.#triggerableConfiguration.supportedRepetitionTypes.includes(this.#selectedRepetitionType)) > + this.#selectedRepetitionType = this.#triggerableConfiguration.supportedRepetitionTypes[0]; Don't we need to reset selectedRepetitionType if triggerableConfiguration is null / undefined? > Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:53 > + _renderRepetitionTypeList(supportedRepetitionTypes, selectedRepetitionType) Given the semantics of LazilyEvaluatedFunction, we should probably do: selectedRepetitionType, ...supportedRepetitionTypes so that each value of supportedRepetitionTypes will be checked instead of the array as a whole. > Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:56 > + this.createElement('option',{selected: repetitionType == selectedRepetitionType, value: repetitionType}, Nit: Need a space after , as well as before-after { ~ }. > Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:66 > + return ` > + <select id="repetition-type"> > + </select> > + `; Seems like we can just do: `<select id="repetition-type"></select>`. > Websites/perf.webkit.org/public/v3/models/triggerable.js:56 > + static REPETITION_TYPES = ['alternating', 'sequential', 'parallel']; We don't typically use ALL_CAP names for this. Please use CamelCase instead. Also, maybe we should call this validRepetitionTypes, or maybe even add a static function: isValidRepetitionType instead? > Websites/perf.webkit.org/public/v3/models/triggerable.js:74 > + const configurationMap = this.ensureNamedStaticMap('triggerableConfigurations'); > + const key = `${object.test.id()}-${object.platform.id()}`; > + console.assert(!(key in configurationMap)); > + configurationMap[key] = this; This isn't right. We're adding each TriggerableConfiguration using config.id as well as this key. We should be creating a unique key and calling TriggerableConfiguration.ensureSingleton with it. So add a new static method like createForTriggerable() which calls ensureSingleton. i.e. id of this object should be this key instead. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:184 > +async function createTestGroupWithPatch(repetitionType='alternating') Nit: space around =.
Comment on attachment 441865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441865&action=review >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:44 >> add_alternating_build_requests($db, $existing_test_type_build_requests, $additional_build_request_count, $current_order); > > Does it really make sense for build requests to be alternating when requests are parallelized? I think this code is used for adding retry build requests. I guess the name `add_alternating_build_requests` is no longer accurate here. May be rename it to `add_non_sequential_build_requests`? >> Websites/perf.webkit.org/public/v3/models/triggerable.js:74 >> + configurationMap[key] = this; > > This isn't right. We're adding each TriggerableConfiguration using config.id as well as this key. > We should be creating a unique key and calling TriggerableConfiguration.ensureSingleton with it. > So add a new static method like createForTriggerable() which calls ensureSingleton. > i.e. id of this object should be this key instead. Got it. It looks like we don't need even expose `ctriggerableConfigurationId` in the return of `api/manifest` at all.
(In reply to dewei_zhu from comment #9) > Comment on attachment 441865 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441865&action=review > > >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:44 > >> add_alternating_build_requests($db, $existing_test_type_build_requests, $additional_build_request_count, $current_order); > > > > Does it really make sense for build requests to be alternating when requests are parallelized? > > I think this code is used for adding retry build requests. I guess the name > `add_alternating_build_requests` is no longer accurate here. May be rename > it to `add_non_sequential_build_requests`? But ordering is still affected, right? Like... should we be using ABABAB... for parallel builds or AAA...BBB...? Are we pairing up each AB pair to a separate device, or are they full parallel in that A & B could run in any random device somewhere? > >> Websites/perf.webkit.org/public/v3/models/triggerable.js:74 > >> + configurationMap[key] = this; > > > > This isn't right. We're adding each TriggerableConfiguration using config.id as well as this key. > > We should be creating a unique key and calling TriggerableConfiguration.ensureSingleton with it. > > So add a new static method like createForTriggerable() which calls ensureSingleton. > > i.e. id of this object should be this key instead. > > Got it. It looks like we don't need even expose > `ctriggerableConfigurationId` in the return of `api/manifest` at all. oh, ok.
Comment on attachment 441865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441865&action=review >>>> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:44 >>>> add_alternating_build_requests($db, $existing_test_type_build_requests, $additional_build_request_count, $current_order); >>> >>> Does it really make sense for build requests to be alternating when requests are parallelized? >> >> I think this code is used for adding retry build requests. I guess the name `add_alternating_build_requests` is no longer accurate here. May be rename it to `add_non_sequential_build_requests`? > > But ordering is still affected, right? Like... should we be using ABABAB... for parallel builds or AAA...BBB...? > > Are we pairing up each AB pair to a separate device, or are they full parallel in that A & B could run in any random device somewhere? Current plan is to pair one iteration A with one iteration B. So, when retry, it behaves more like ABABAB. You made a very good point here. If we completely select random devices, then the retry would be close to AABB but a bit more complicated. Another approach is to have the syncing script automatically reschedule failed runs without adding new build requests. And only fail if retry is exhausted. With this approach, we should reject adding retry for parallelized test group.
(In reply to dewei_zhu from comment #11) > Comment on attachment 441865 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441865&action=review > > >>>> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:44 > >>>> add_alternating_build_requests($db, $existing_test_type_build_requests, $additional_build_request_count, $current_order); > >>> > >>> Does it really make sense for build requests to be alternating when requests are parallelized? > >> > >> I think this code is used for adding retry build requests. I guess the name `add_alternating_build_requests` is no longer accurate here. May be rename it to `add_non_sequential_build_requests`? > > > > But ordering is still affected, right? Like... should we be using ABABAB... for parallel builds or AAA...BBB...? > > > > Are we pairing up each AB pair to a separate device, or are they full parallel in that A & B could run in any random device somewhere? > > Current plan is to pair one iteration A with one iteration B. So, when > retry, it behaves more like ABABAB. I see. Then alternating order makes sense. > You made a very good point here. If we completely select random devices, > then the retry would be close to AABB but a bit more complicated. > > Another approach is to have the syncing script automatically reschedule > failed runs without adding new build requests. And only fail if retry is > exhausted. With this approach, we should reject adding retry for > parallelized test group. Yeah, those are both valid options. I guess we should figure out which model makes most sense in the long term since changing this behavior later after deployment would be highly problematic.
(In reply to Ryosuke Niwa from comment #12) > (In reply to dewei_zhu from comment #11) > > Comment on attachment 441865 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=441865&action=review > > > > >>>> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:44 > > >>>> add_alternating_build_requests($db, $existing_test_type_build_requests, $additional_build_request_count, $current_order); > > >>> > > >>> Does it really make sense for build requests to be alternating when requests are parallelized? > > >> > > >> I think this code is used for adding retry build requests. I guess the name `add_alternating_build_requests` is no longer accurate here. May be rename it to `add_non_sequential_build_requests`? > > > > > > But ordering is still affected, right? Like... should we be using ABABAB... for parallel builds or AAA...BBB...? > > > > > > Are we pairing up each AB pair to a separate device, or are they full parallel in that A & B could run in any random device somewhere? > > > > Current plan is to pair one iteration A with one iteration B. So, when > > retry, it behaves more like ABABAB. > > I see. Then alternating order makes sense. > > > You made a very good point here. If we completely select random devices, > > then the retry would be close to AABB but a bit more complicated. > > > > Another approach is to have the syncing script automatically reschedule > > failed runs without adding new build requests. And only fail if retry is > > exhausted. With this approach, we should reject adding retry for > > parallelized test group. > > Yeah, those are both valid options. I guess we should figure out which model > makes most sense in the long term since changing this behavior later after > deployment would be highly problematic. Now I think about it more, we are talking about two types of parallelized AB: parallel with paired iterations and parallel without pairing. For now, we should stay with `parallel with paired A/B` the retry logic is the same as alternating. And this is how those build requests are ordered in current patch. Maybe I should update the description to be something like 'parallel (paired iteration)'. When we get to implementing the other one in the future, I think we can add another repetition type which behaves more like a sequential test on retry.
(In reply to dewei_zhu from comment #13) > (In reply to Ryosuke Niwa from comment #12) > > > Yeah, those are both valid options. I guess we should figure out which model > > makes most sense in the long term since changing this behavior later after > > deployment would be highly problematic. > Now I think about it more, we are talking about two types of parallelized > AB: parallel with paired iterations and parallel without pairing. > > For now, we should stay with `parallel with paired A/B` the retry logic is > the same as alternating. And this is how those build requests are ordered in > current patch. Maybe I should update the description to be something like > 'parallel (paired iteration)'. > > When we get to implementing the other one in the future, I think we can add > another repetition type which behaves more like a sequential test on retry. Hm... there is also an option of being fully parallel as well. Perhaps we should call this mode "paired parallel" and call it that in triggerable_configuration_repetition_types as well. Otherwise updating the database schema is gonna be annoying.
(In reply to Ryosuke Niwa from comment #14) > (In reply to dewei_zhu from comment #13) > > (In reply to Ryosuke Niwa from comment #12) > > > > > Yeah, those are both valid options. I guess we should figure out which model > > > makes most sense in the long term since changing this behavior later after > > > deployment would be highly problematic. > > Now I think about it more, we are talking about two types of parallelized > > AB: parallel with paired iterations and parallel without pairing. > > > > For now, we should stay with `parallel with paired A/B` the retry logic is > > the same as alternating. And this is how those build requests are ordered in > > current patch. Maybe I should update the description to be something like > > 'parallel (paired iteration)'. > > > > When we get to implementing the other one in the future, I think we can add > > another repetition type which behaves more like a sequential test on retry. > > Hm... there is also an option of being fully parallel as well. Perhaps we > should call this mode "paired parallel" and call it that in > triggerable_configuration_repetition_types as well. > Otherwise updating the database schema is gonna be annoying. I agree. Renaming enum is supported if we upgrade Postgres to 10 or newer: https://www.postgresql.org/docs/10/sql-altertype.html
Created attachment 442669 [details] Patch
Comment on attachment 442669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442669&action=review > Websites/perf.webkit.org/browser-tests/test-group-form-tests.js:40 > + testGroupForm.part('repetition-type-selection').selectedRepetitionType = 'sequential'; This should probably be repetition-type or repetition-type-selector? > Websites/perf.webkit.org/migrate-database.sql:29 > + PRIMARY KEY (configrepetition_config, configrepetition_type)); > + We should probably also insert 'alternating' and 'sequential' as supported types for all existing triggerables? > Websites/perf.webkit.org/public/include/json-header.php:199 > + $configrepetition_rows = $db->select_rows('triggerable_configuration_repetition_types', 'configrepetition', array()); I guess this is okay because we expect triggerable_configuration_repetition_types to be a very small table. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:37 > + exit_with_error('CommitSetNotSupportedForNonSequentialRepetitionType'); We can just say CommitSetNotSupportedRepetitionType in the error name but then spit out the repetition type. That way, we can diagnose what got sent to the server when we see this error message. Maybe also spit out the commit set ID? > Websites/perf.webkit.org/public/privileged-api/create-test-group.php:84 > + $supported_repetition_types = array(); > + $rows = $db->select_rows('triggerable_configuration_repetition_types', NULL, array('configrepetition_config' => $triggerable_configuration['trigconfig_id'])); > + foreach ($rows as $row) > + array_push($supported_repetition_types, $row['configrepetition_type']); > + } Why don't we just query the database directly instead of fetching the entire table & looping over the result? i.e. $is_repetition_type_supported = !!$db->select_first_row('triggerable_configuration_repetition_types', 'configrepetition', array('config' => $triggerable_configuration['trigconfig_id'], 'type' => $repetition_type)); > Websites/perf.webkit.org/public/v3/components/repetition-type-selection.js:17 > + const repetitionType = this.content('repetition-type'); > + repetitionType.onchange = () => this.#selectedRepetitionType = repetitionType.value; Seems like we can just do: this.content('repetition-type').onchange = ~ > Websites/perf.webkit.org/public/v3/models/triggerable.js:31 > + if (!triggerableConfiguration) > return null; > - for (; test; test = test.parentTest()) { > - const key = `${test.id()}-${platform.id()}`; > - if (key in configurationMap) > - return configurationMap[key]; > - } > - return null; > + return triggerableConfiguration.triggerable; Now we can do this: TriggerableConfiguration.findByTestAndPlatform(test, platform).?triggerable; > Websites/perf.webkit.org/public/v3/models/triggerable.js:69 > + static createIdForTestAndPlatform(test, platform) { return `${test.id()}-${platform.id()}`; } Maybe just say idForTestAndPlatform? We're not really creating any object here. > Websites/perf.webkit.org/public/v3/models/triggerable.js:76 > + return TriggerableConfiguration.ensureSingleton(id, { > + test, platform, supportedRepetitionTypes, triggerable > + }); Seems like can it this in a single line? > Websites/perf.webkit.org/server-tests/api-update-triggerable-tests.js:482 > + let result = await db.query('SELECT configrepetition_type AS repetition_type FROM triggerable_configuration_repetition_types'); Why not selectAll? > Websites/perf.webkit.org/server-tests/api-update-triggerable-tests.js:484 > + assert.deepEqual(result.rows.map(row => row['repetition_type']).sort(), ['alternating', 'sequential']); Seems like we should just sort by type when we query the database? > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:882 > + it('should reject with "UnsupportedRepetitionType" when repetition type is not supported under triggerable configuration', async () => { We should probably add another test for rejecting sequential as well as invalid string just as sanity check. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1468 > + it('should reject with "InvalidRepetitionType" if repetition type is not "alternating", "sequential" or "parallel"', async () => { Use Oxford comma? I think that's WebKit standard. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1480 > + it('should reject with "UnsupportedRepetitionType" if repetition type is "parallel" but triggerable configuration only supports "sequential" and "alternating"', async () => { Nit: parallel -> paired-parallel > Websites/perf.webkit.org/server-tests/resources/mock-data.js:328 > + 'repetitionTypesForScheduling': ['alternating', 'sequential'], Why not just repetitionTypes? > 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 spaces around =. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:688 > + it('should build root but bot schedule tests for "paired-parallel" test group on triggerable that only schedules tests for "alternating" and "sequential" test groups', async () => { I don't understand this description.
As we discussed, let's fold the repetition type into builder configurations for buildbot.
Created attachment 442804 [details] Patch
Created attachment 442903 [details] Patch
Comment on attachment 442903 [details] 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.#selectedRepetitionType)) 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?
Comment on attachment 442903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442903&action=review Thanks for the review. >> 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? This test also ensures test type `paired-parallel` build request is not scheduled. That's why it looks gigantic. Will try to make it smaller. >> 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. We need fetch all related test groups before `_validateRequests` where repetition type from test group will be used in `BuildbotSyncer.matchesConfiguration`. Will add that into ChangeLog.
Landed in r285189.