[perf dashboard] Add sequential mode for perf dashboard A/B testing.
Created attachment 424552 [details] Patch
Comment on attachment 424552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424552&action=review Maybe we should just skip B's if all A's failed? > Websites/perf.webkit.org/public/include/commit-sets-helpers.php:7 > -function create_test_group_and_build_requests($db, $commit_sets, $task_id, $name, $author, $triggerable_id, $platform_id, $test_id, $repetition_count, $needs_notification) { > +function create_test_group_and_build_requests($db, $commit_sets, $task_id, $name, $author, $triggerable_id, $platform_id, $test_id, $repetition_count, $test_mode, $needs_notification) { "test mode" is a very generic term. How about "repetition type" or "repetition strategy"? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:17 > if (!$additional_build_request_count) We don't need this check anymore given we're using validate_arguments now, right? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:21 > if (!$test_group_id) Ditto. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:24 > + $commit_set = array_get($arguments, 'commitSet', NULL); Nit: call this $commit_set_id. > Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:13 > + $test_mode = array_get($data, 'testMode', 'alternate'); Ditto about naming. > Websites/perf.webkit.org/public/v3/components/test-group-form.js:30 > + modeExplanation.textContent = testModeSelect.value == 'alternate' ? We don't want to be updating textContent like. enqueueToRender instead. > Websites/perf.webkit.org/public/v3/components/test-group-form.js:82 > + <label id="mode-explanation">(Alternate A & B between iterations)</label> This is very wordy. I think we can just say: alternative (e.g. ABAB) sequential (e.g. AABB) in the above select element instead. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:327 > - static async create(name, startPoint, endPoint, testGroupName=null, repetitionCount=0, notifyOnCompletion=false) > + static async create(name, startPoint, endPoint, testGroupName=null, repetitionCount=0, notifyOnCompletion=false, testMode='alternate') Huh, this function has wrong style. All these = should have spaces around them! > Websites/perf.webkit.org/public/v3/models/test-group.js:14 > - this._initialRepetitionCount = object.initialRepetitionCount; > + this._initialRepetitionCount = parseInt(object.initialRepetitionCount); Use +object.initialRepetitionCount instead. It would be more strict than parseInt since parseInt like atoi will ignore non-digit letters at the end. > Websites/perf.webkit.org/public/v3/models/test-group.js:39 > - this._initialRepetitionCount = object.initialRepetitionCount; > + this._initialRepetitionCount = parseInt(object.initialRepetitionCount); Ditto. > Websites/perf.webkit.org/public/v3/models/test-group.js:102 > + const firstTwoBuildRequests = this._orderedBuildRequests().filter(buildRequest => buildRequest.order() > 0).slice(0, 2); > + if (firstTwoBuildRequests.length < 2) > + return 'alternate'; > + const [first, second] = firstTwoBuildRequests; > + return first.commitSet() == second.commitSet() ? 'sequential' : 'alternate'; Hm... this code makes me think that maybe we should just store this information in the database instead. > Websites/perf.webkit.org/public/v3/models/test-group.js:243 > + async addMoreBuildRequests(addCount, commitSet=null) Nit: spaces around =. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:374 > - this.content('status-summary').innerHTML = statusSummary; > - > + this.content('status-summary').textContent = this._summarizedTestGroup(currentGroup); LOL. This is bad. How did we end up with innerHTML!? Also, we should be using this.renderReplace instead. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:376 > - this.content('request-summary').innerHTML = `Scheduled ${authoredBy} at ${currentGroup.createdAt()}` > + this.content('request-summary').textContent = `Scheduled ${authoredBy} at ${currentGroup.createdAt()}` Ditto about using renderReplace. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:404 > + retryCountByCommitSet.set(configurationLabel, addedRepetitionForCurrentConfiguration) > + configurationLabel = String.fromCharCode(configurationLabel.charCodeAt(0) + 1); > + if (addedRepetitionForPreviousConfiguration >= 0 && addedRepetitionForCurrentConfiguration !== addedRepetitionForPreviousConfiguration) > + sameRetryCount = false; > + addedRepetitionForPreviousConfiguration = addedRepetitionForCurrentConfiguration; This is very complicated! I'd suggest something like this instead: const retryCountByCommitSet = new Map; for (const commitSet of testGroup.requestedCommitSets()) retryCountByCommitSet.set(commitSet, testGroup.repetitionCountForCommitSet(commitSet) - testGroup.initialRepetitionCount()); const firstCommit = testGroup.requestedCommitSets()[0]; const retryCountsAreIdentical = testGroup.requestedCommitSets().every((commitSet) => retryCountByCommitSet.get(commitSet) == retryCountByCommitSet.get(firstCommit)); > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:422 > + statusSummaryParts.push('due to failures.') This is hard to read, we probably want to define a local variable to be able to do something like this: return [`${testGroup.initialRepetitionCount()} requested,`, retryDescription, ' due to failures.'] > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:423 > + return statusSummaryParts.join(' '); If we used renderReplace, we can just return an array here. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:133 > + modeExplanation.textContent = testMode.value == 'alternate' ? Again, we shouldn't be updating DOM like this. We need to enqueueToRender > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:609 > + <label>mode</label> > + <label id="mode-explanation">(Alternate A & B between iterations)</label> Same suggestion about this being very verbose. > Websites/perf.webkit.org/server-tests/resources/common-operations.js:56 > - await testFunction() > + await testFunction(); Huh, bad indentation. This should be indented by 4 spaces to the right of try, not by 8 spaces. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:15 > + if (testGroup.testMode() === 'alternate') > + await createAdditionalBuildRequestForTestGroupInAlternateMode(testGroup, maximumRetryFactor); > + else if (testGroup.testMode() === 'sequential') > + await createAdditionalBuildRequestForTestGroupInSequentialMode(testGroup, maximumRetryFactor); Can we use a switch and assert that we don't hit the the default? Also, these are really long name. How about just createAlternatingRetriesForTestGroupInAlternate / createSequentialRetriesForTestGroupInAlternate? > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:19 > +async function createAdditionalBuildRequestForTestGroupInAlternateMode(testGroup, maximumRetryFactor) { Nit: { on the new line. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:54 > +async function createAdditionalBuildRequestForTestGroupInSequentialMode(testGroup, maximumRetryFactor) { Nit: { should be on new line. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:58 > + // Relies on `TestGroup.requestedCommitSets` returns commit sets sorted by build request order. A comment like this is an prime example of something we should write an assertion instead. Here, we can do something like this: console.assert(Array.from(testGroup.requestedCommitSets()).every((commitSet, index, list) => !index || list[index - 1].order() < commitSet.order()));
<rdar://problem/76225788>
Created attachment 425244 [details] Patch
Comment on attachment 424552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424552&action=review >> Websites/perf.webkit.org/public/v3/models/test-group.js:102 >> + return first.commitSet() == second.commitSet() ? 'sequential' : 'alternate'; > > Hm... this code makes me think that maybe we should just store this information in the database instead. I agree! I think storing it in database might be easier in many ways, will update the patch with adding repetition_type field. >> Websites/perf.webkit.org/server-tests/resources/common-operations.js:56 >> + await testFunction(); > > Huh, bad indentation. This should be indented by 4 spaces to the right of try, not by 8 spaces. Nice catch!
(In reply to Ryosuke Niwa from comment #2) > Comment on attachment 424552 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424552&action=review > > Maybe we should just skip B's if all A's failed? Like what we did if root building fails? There might be a race that the first B is already running. That means we either leave the B run alone and we will not accept the result or find a way to stop the build.
Created attachment 426210 [details] Patch
Created attachment 426287 [details] Patch
Comment on attachment 426287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426287&action=review > Websites/perf.webkit.org/ChangeLog:11 > + support alternating A and B configurations between runs. > + Add retry logic for 'sequential' A/B testing. Can we insert blank lines between different paragraphs? It's hard to read otherwise. > Websites/perf.webkit.org/init-database.sql:295 > + testgroup_repetition_type varchar(32) DEFAULT 'alternating', Why isn't this enum? > Websites/perf.webkit.org/public/include/commit-sets-helpers.php:7 > -function create_test_group_and_build_requests($db, $commit_sets, $task_id, $name, $author, $triggerable_id, $platform_id, $test_id, $repetition_count, $needs_notification) { > +function create_test_group_and_build_requests($db, $commit_sets, $task_id, $name, $author, $triggerable_id, $platform_id, $test_id, $repetition_count, $repetition_type, $needs_notification) { Nit: We should put { in the new line we're at it. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:47 > + if (is_null($commit_set_id) || $requested_commit_set == $commit_set_id) > + array_push($commit_sets, $requested_commit_set); Hm... this logic is getting quite a bit complicated. Maybe it's better if we just extracted helper functions and split to sequential case & alternating case right away? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:51 > + if (count($commit_sets) && $is_sequential_type) { Nit: Two spaces between if and (. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:66 > + $db->query('UPDATE build_requests SET request_order = request_order + ' . $incremental > + . ' WHERE request_commit_set = $1', array($commit_set)); We can't use prepared statement like 'request_order + $1'? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:90 > +function insert_build_requests($db, $build_request, $order_overwrite) { Nit: { on the next line. > Websites/perf.webkit.org/public/v3/components/test-group-form.js:92 > + <label>iterations per set in</label> > + <select id="repetition-type"> > + <option selected>alternating</option> > + <option>sequential</option> > + </select> > + <label>mode</label> > + <label id="repetition-type-explanation">(e.g. ABABAB)</label> Again, this is quite verbose. I think we can just do: <label>iterations per set in</label> <select id="repetition-type"> <option selected>alternate (ABAB)</option> <option>sequence (AABB)</option> </select> > Websites/perf.webkit.org/public/v3/models/build-request.js:-34 > - console.assert(this._order == object.order); I guess can we still assert that this._order <= object.order? > Websites/perf.webkit.org/public/v3/models/test-group.js:99 > + hasRetry() Nit: hasRetries* > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:140 > + _deferSchedulingRequest(buildRequest) I'd prefer if this function name said why we may defer it. e.g. _shouldDeferSequentialTestingRequestWithNewCommitSet(~) > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:146 > + if (testGroup.repetitionType() !== 'sequential') Just use !=. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:151 > + if (buildRequestIndex <= 0) This can't be negative as that would mean that buildRequest isn't in orderedBuildRequests. We should assert that instead. Then this condition becomes !buildRequestIndex > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:159 > + const buildRequestBeforeCurrent = orderedBuildRequests[buildRequestIndex - 1]; > + if (buildRequestBeforeCurrent.isBuild()) > + return false; > + > + if (buildRequestBeforeCurrent.commitSet() === buildRequest.commitSet()) > + return false; This is a very round about way of checking whether this request is the first one in the group of build requests for a given commit set. We can simply check this: const isFirstTestRequestForCommitSet = testGroup.requestsForCommitSet(buildRequest.commitSet()).find((request) => !request.isBuild()) == request; Then we don't even need to make testGroup.orderedBuildRequests a public method. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:162 > + return testGroup.initialRepetitionCount() > testGroup.requestsForCommitSet(buildRequestBeforeCurrent.commitSet()) > + .filter(request => request.isTest() && request.hasCompleted()).length; I'm a bit confused here. Didn't we say we want to do retires first before trying the second configuration? > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:-223 > - This seems like an unrelated change? > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:273 > - _scheduleRequestIfWorkerIsAvailable(nextRequest, requestsInGroup, syncer, workerName) > + async _scheduleRequestIfWorkerIsAvailable(nextRequest, requestsInGroup, syncer, workerName) Why are we making this async? We're not using await anywhere here. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:9 > continue; Huh, I never caught this but it's very strange that this is a standalone module file. Why isn't this a part of TestGroup? > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:13 > + switch(testGroup.repetitionType()) { > + case 'alternating': This is very verbose. I think it's better to do: console.assert(['alternating', 'sequential'].includes(testGroup.repetitionType()); if (testGroup.repetitionType() == 'alternating') await ~ else await ~ > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:20 > + console.assert(false, 'InvalidRepetitionType') Missing a semicolon at the end. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:37 > + hasCompletedBuildRequestForEachCommitSet &= (completedBuildRequestCount > 0); Oh man, we should be using &&= instead. This is bitwise AND. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:69 > + console.assert(Array.from(testGroup.requestedCommitSets()).every((commitSet, index, list) => > + !index || Math.min(...(testGroup.requestsForCommitSet(list[index - 1]).map(buildRequest => buildRequest.order()))) > + < Math.min(...(testGroup.requestsForCommitSet(commitSet).map(buildRequest => buildRequest.order()))) > + )); This assertion is impossible to decipher at glance & also incredibly conservative. Since testGroup.requestsForCommitSet sorts all requests via order, it's sufficient to check this instead: const lastItem = (array) => array[array.length - 1]; console.assert(testGroup.requestedCommitSets().every(currentSet, index, setList) => !index || lastItem(testGroup.requestsForCommitSet(setList[index - 1])).order() < testGroup.requestsForCommitSet(currentSet)[0].order()); > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:70 > + const firstCommitSetToRetry = testGroup.requestedCommitSets().find(commitSet => { This variable / find result is very confusing. Despite of the fact this variable claims that this is the commit set to retry, we may also find a commit set which is about to exceed the maximum retries. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:78 > + const unfinishedBuildRequestCount = buildRequests.filter((buildRequest) => !buildRequest.hasFinished()).length; > + const potentiallySuccessfulCount = completedBuildRequestCount + unfinishedBuildRequestCount; > + missingBuildRequestCount = testGroup.initialRepetitionCount() - potentiallySuccessfulCount; This is some funky math here, which is something we can share between two versions of the function. I think we should extract this as a helper method on TestGroup. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:90 > + if (willExceedMaximumRetry) > + await testGroup.cancelPendingRequests(); This doesn't look right. We shouldn't stop running the rest of the test group just because one of the configurations may exceed the maximum retry. What we had discussed is canceling the rest if all requests had failed. If we had requested 8, and the maximum retry factor was 2, and we eventually tried 16 but only got 7 successful runs, we probably still want to go ahead with the rest of testing. It would be infuriating if we canceled the rest of testing when the failure rate is so high as to hit the max retry limit in the first place. > Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:91 > + else if(!firstCommitSetToRetry) Nit: missing space between if and (.
Created attachment 426409 [details] Patch
Comment on attachment 426287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426287&action=review >> Websites/perf.webkit.org/init-database.sql:295 >> + testgroup_repetition_type varchar(32) DEFAULT 'alternating', > > Why isn't this enum? I think the main reason to not using enum is adding new type to enum will require recreating the enum table. >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:66 >> + . ' WHERE request_commit_set = $1', array($commit_set)); > > We can't use prepared statement like 'request_order + $1'? Actually we can, I will update this part. >> Websites/perf.webkit.org/public/v3/models/build-request.js:-34 >> - console.assert(this._order == object.order); > > I guess can we still assert that this._order <= object.order? OK. >> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:159 >> + return false; > > This is a very round about way of checking whether this request is the first one in the group of build requests for a given commit set. > We can simply check this: > const isFirstTestRequestForCommitSet = testGroup.requestsForCommitSet(buildRequest.commitSet()).find((request) => !request.isBuild()) == request; > Then we don't even need to make testGroup.orderedBuildRequests a public method. I'm trying to avoid adding the assumption that we only have two configurations in an A/B test. It can be A/B/C test. >> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:162 >> + .filter(request => request.isTest() && request.hasCompleted()).length; > > I'm a bit confused here. Didn't we say we want to do retires first before trying the second configuration? Yes, the function returns whether we would like to defer the request, so if the user initially scheduled is greater than completed build request from previous configuration (does not meet user expectation), we should defer and wait for retry. >> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:273 >> + async _scheduleRequestIfWorkerIsAvailable(nextRequest, requestsInGroup, syncer, workerName) > > Why are we making this async? We're not using await anywhere here. Will fix this. >> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:9 >> continue; > > Huh, I never caught this but it's very strange that this is a standalone module file. > Why isn't this a part of TestGroup? The reason we did this is because there could be multiple retry strategies for a test group. >> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:13 >> + case 'alternating': > > This is very verbose. I think it's better to do: > console.assert(['alternating', 'sequential'].includes(testGroup.repetitionType()); > if (testGroup.repetitionType() == 'alternating') > await ~ > else > await ~ I think that's one comment from your last review. "Can we use a switch and assert that we don't hit the the default? " I think using `if else` might be cleaner because so far we only have two types.
(In reply to dewei_zhu from comment #11) > Comment on attachment 426287 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426287&action=review > > >> Websites/perf.webkit.org/init-database.sql:295 > >> + testgroup_repetition_type varchar(32) DEFAULT 'alternating', > > > > Why isn't this enum? > > I think the main reason to not using enum is adding new type to enum will > require recreating the enum table. That is not a good reason to avoid using enum. Note that if you're referring to the inability to add a new value to enum, that has become possible since Postgres 9.1: https://www.postgresql.org/docs/9.1/sql-altertype.html > >> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:159 > >> + return false; > > > > This is a very round about way of checking whether this request is the first one in the group of build requests for a given commit set. > > We can simply check this: > > const isFirstTestRequestForCommitSet = testGroup.requestsForCommitSet(buildRequest.commitSet()).find((request) => !request.isBuild()) == request; > > Then we don't even need to make testGroup.orderedBuildRequests a public method. > > I'm trying to avoid adding the assumption that we only have two > configurations in an A/B test. It can be A/B/C test. I don't understand. Even if we had A/B/C configurations, it's still sufficient to check whether this is the first of its kind or not. > >> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:9 > >> continue; > > > > Huh, I never caught this but it's very strange that this is a standalone module file. > > Why isn't this a part of TestGroup? > > The reason we did this is because there could be multiple retry strategies > for a test group. But we never did, right? I don't think it makes to keep that kind of extra verboseness just in the case we may add it in the future. There is virtually no benefit until such a need comes up. As is, the way we've structured the code is making it harder to understand the codebase as a whole. > >> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:13 > >> + case 'alternating': > > > > This is very verbose. I think it's better to do: > > console.assert(['alternating', 'sequential'].includes(testGroup.repetitionType()); > > if (testGroup.repetitionType() == 'alternating') > > await ~ > > else > > await ~ > > I think that's one comment from your last review. > "Can we use a switch and assert that we don't hit the the default? " > I think using `if else` might be cleaner because so far we only have two > types. Oh oops, sorry. I guess I didn't think of the implication there.
Comment on attachment 426409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426409&action=review > Websites/perf.webkit.org/ChangeLog:15 > + Add retry logic for 'sequential' A/B testing and vove retry logic into syncing script so that retry can be Nit: vove?? > Websites/perf.webkit.org/ChangeLog:19 > + Fix a potential race in syncing script that 'Promise.all' may cause test group not scheduled in order . Nit: space between order and . > Websites/perf.webkit.org/init-database.sql:295 > + testgroup_repetition_type varchar(32) DEFAULT 'alternating', We should enum as I mention above. > Websites/perf.webkit.org/public/include/commit-sets-helpers.php:27 > + } else { > + for ($i = 0; $i < $repetition_count; $i++) { We should assert that $repetition_type is alternating here. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:33 > + $is_sequential_type = $test_group['testgroup_repetition_type'] == 'sequential'; Please assert that testgroup_repetition_type is either alternating or sequential. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:43 > +function add_build_request_for_sequential_test_group($db, $build_requests, $add_count, $commit_set_id) > +{ Please flip the order in which functions are defined so that the diff will look saner. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:48 > - foreach ($existing_build_requests as $build_request) { > + foreach ($build_requests as $build_request) { Why are we renaming this? They're clearly existing build requests. It's important for clarify since this whole function is about adding new ones. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:56 > + if (is_null($commit_set_id) || $requested_commit_set == $commit_set_id) Just check !$commit_set_id || $requested_commit_set == $commit_set_id > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:59 > $build_request_by_commit_set[$requested_commit_set] = $build_request; So the only reason have this dictionary is to get the order of the last build request. It's very confusing / misleadingly generic to have this & order_incremental_by_commit_set. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:61 > + if (count($commit_sets)) { Just do: if ($commit_sets) > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:63 > + $order_incremental_by_commit_set[$requested_commit_set] = $current_order_incremental; > + $current_order_incremental += $add_count; This whole incremental thing is rather confusing. Logically, each build request could only move by the number of build requests added before it. But that could be either $add_count or $add_count * (the position of $commit_set in $commit_sets). > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:75 > + $db->query('UPDATE build_requests SET request_order = request_order + $1 WHERE request_commit_set = $2', > + array($incremental, $commit_set)); It's not safe to assume that request_commit_set will uniquely identify a test group. Please specify the group. This also looks wrong. Because we have a unique constraint on (request_group, request_order), this will fail if commit_set_id wasn't set so that we'd try to add more build requests to every commit set since then request_order will start overlapping with later build requests. r- because of this bug. I guess we don't have any tests for this case? I suggest we rewrite this so that we iterate over all commits sets in the reverse order, and update build requests for each commit set (and test group). > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:106 > + if (!is_null($commit_set_id) && !count($commit_sets)) > + exit_with_error('CommitSetNotInTestGroup', array('commitSet' => $commit_set_id)); The first check for commit_set_id is redundant since we never use the value of commit_set_id. > Websites/perf.webkit.org/public/v3/components/test-group-form.js:74 > + <option selected value="alternating">alternating (ABAB)</option> > + <option value="sequential">sequential (AABB)</option> This should be nouns: "alternative" and "sequence", not adjectives. > Websites/perf.webkit.org/public/v3/models/test-group.js:113 > + const buildRequests = this.requestsForCommitSet(commitSet).filter((buildRequest) => buildRequest.isTest()); > + const completedBuildRequestCount = buildRequests.filter((buildRequest) => buildRequest.hasCompleted()).length; > + const unfinishedBuildRequestCount = buildRequests.filter((buildRequest) => !buildRequest.hasFinished()).length; This is very inefficient way of counting these two values. I think we should just loop over this._buildRequests like this: for (const request of this._buildRequests) { if (request.commitSet() != commitSet || !request.isTest()) continue; completedBuildRequestCount += request.hasCompleted(); unfinishedBuildRequestCount += buildRequest.hasFinished(); } > Websites/perf.webkit.org/public/v3/models/test-group.js:276 > - static createWithTask(taskName, platform, test, groupName, repetitionCount, commitSets, notifyOnCompletion) > + static async createWithTask(taskName, platform, test, groupName, repetitionCount, commitSets, notifyOnCompletion, repetitionType) For consistency, let's add repetitionType to right after repetitionCount. > Websites/perf.webkit.org/public/v3/models/test-group.js:281 > + const params = {taskName, name: groupName, platform: platform.id(), test: test.id(), repetitionCount, > + revisionSets, repetitionType, needsNotification: !!notifyOnCompletion}; It seems like we can just inline params to the function call of PrivilegedAPI.sendRequest now? > Websites/perf.webkit.org/public/v3/models/test-group.js:288 > - static createWithCustomConfiguration(task, platform, test, groupName, repetitionCount, commitSets, notifyOnCompletion) > + static async createWithCustomConfiguration(task, platform, test, groupName, repetitionCount, commitSets, notifyOnCompletion, repetitionType) Ditto. > Websites/perf.webkit.org/public/v3/models/test-group.js:294 > + const data = await PrivilegedAPI.sendRequest('create-test-group', params); Ditto. > Websites/perf.webkit.org/public/v3/models/test-group.js:295 > + return await this.fetchForTask(data['taskId'], true); It seems like we can just use fetchById now? create-test-group returns testGroupId too. > Websites/perf.webkit.org/public/v3/models/test-group.js:298 > - static createAndRefetchTestGroups(task, name, repetitionCount, commitSets, notifyOnCompletion) > + static async createAndRefetchTestGroups(task, name, repetitionCount, commitSets, notifyOnCompletion, repetitionType) Ditto. > Websites/perf.webkit.org/public/v3/models/test-group.js:306 > + return await this.fetchForTask(data['taskId'], true); Ditto. > Websites/perf.webkit.org/public/v3/models/test-group.js:314 > + static async fetchById(testGroupId, ignoreCache=false) Nit: space around =. Also, we should also call the first argument id. > Websites/perf.webkit.org/public/v3/models/test-group.js:317 > + return this._createModelsFromFetchedTestGroups(data)[0]; This works but I think our preferred way of writing is this: return TestGroup.findById(testGroupId). > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:28 > + this.part('form').listenToAction('startTesting', (repetitionCount, name, commitSetMap, notifyOnCompletion, repetitionType) => { > + this.dispatchAction('newTestGroup', name, repetitionCount, commitSetMap, notifyOnCompletion, repetitionType); Again, we should reorder two that repetitionType appears right after repetitionCount for consistency. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:110 > + this.part('form').listenToAction('startTesting', (repetitionCount, name, commitSetMap, notifyOnCompletion, repetitionType) => { > + this.dispatchAction('newTestGroup', name, repetitionCount, commitSetMap, notifyOnCompletion, repetitionType); Ditto. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:271 > + this.part('retry-form').listenToAction('startTesting', (repetitionCount, notifyOnCompletion, repetitionType) => { > + this.dispatchAction('retryTestGroup', this._currentTestGroup, repetitionCount, notifyOnCompletion, repetitionType); Ditto. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:273 > - this.part('bisect-form').listenToAction('startTesting', (repetitionCount, notifyOnCompletion) => { > + this.part('bisect-form').listenToAction('startTesting', (repetitionCount, notifyOnCompletion, repetitionType) => { Ditto. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:384 > + if (currentGroup.hasRetries()) { > + statusSummary = this.createElement('div', {id: 'status-summary', class: 'summary'}, > + this._summarizedTestGroup(currentGroup)); > + } This is very confusing. This function is really about summarizing retries. We should be re-using the element in the DOM and simply calling renderReplace on this.content('status-summary'). The element should probably be renamed to retry-summary. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:388 > + `Scheduled ${authoredBy}at ${currentGroup.createdAt()}`); Nit: Missing space between ${authoredBy} and at. The original code was better for that reason. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:391 > + hideButton = this.createElement('button', {'id': 'hide-button'}, > + currentGroup.isHidden() ? 'Unhide' : 'Hide'); Again, we shouldn't be re-constructing button every single time. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:406 > + this.renderReplace(this.content('test-group-details'), [ > + this.content('results-viewer'), this.content('revision-table'), statusSummary, requestSummary, > + retryForm, bisectForm, hideButton, pendingRequestCancelWarning > + ].filter(element => !!element)); Ugh... this will re-generate the entire DOM. I don't think we should do that. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:418 > + testGroup.repetitionCountForCommitSet(commitSet) - testGroup.initialRepetitionCount()); Why are we re-computing retry count here?? Just call testGroup.retryCountForCommitSet. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:419 > + currentLabel = String.fromCharCode(currentLabel.charCodeAt(0) + 1); Use testGroup.labelForCommitSet instead. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:422 > + const retryCountsAreIdentical = testGroup.requestedCommitSets().every((commitSet) => > + retryCountByLabel.get(commitSet) === retryCountByLabel.get(currentLabel)); It seems like this should really be a helper function on TestGroup. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:426 > + if (retryCountsAreIdentical) > + return `${testGroup.initialRepetitionCount()} requested, > + ${retryCountByLabel.get(currentLabel)} added due to failures.`; Two line statements like these require curly braces: { ~ } Since testGroup.initialRepetitionCount() is used many times in this function, we might as well as just store in a local variable instead so that this whole thing fits in a single line. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:435 > + const retrySummary = retrySummaryParts.length === 1 ? retrySummaryParts[0] : > + `${retrySummaryParts.slice(0, -1).join(', ')} and ${retrySummaryParts[retrySummaryParts.length - 1]}`; Use new Intl.ListFormat('en', {style:'long', type:'conjunction'}).format(~)? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat/ListFormat > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:917 > - _createTestGroupAfterVerifyingCommitSetList(testGroupName, repetitionCount, commitSetMap, notifyOnCompletion) > + _createTestGroupAfterVerifyingCommitSetList(testGroupName, repetitionCount, commitSetMap, notifyOnCompletion, repetitionType) Maybe make this function async as well? > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:111 > + this._repetitionType = 'alternating'; Looks like this variable is never used now? So maybe remove it? > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:131 > + repetitionType.onchange = () => this._repetitionType = repetitionType.value; Ditto. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:602 > + <option value="alternating" selected>alternating (ABAB)</option> > + <option value="sequential">sequential (AABB)</option> Again, this should read "In alternate (ABAB)" / "In sequence (AABB)" > Websites/perf.webkit.org/public/v3/pages/create-analysis-task-page.js:25 > - _createAnalysisTaskWithGroup(repetitionCount, testGroupName, commitSets, platform, test, taskName, notifyOnCompletion) > + async _createAnalysisTaskWithGroup(repetitionCount, testGroupName, commitSets, platform, test, taskName, notifyOnCompletion, repetitionType) Ditto about putting repetitionType right after repetitionCount. > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:570 > + it('should still return build requests associated with a given triggerable when test group has all build requests finished with "mayNeedMoreRequest" flag to be trues', () => { > + return MockData.addMockData(TestServer.database(), ['completed', 'completed', 'completed', 'failed'], true, null, null, 'alternating', true).then(() => { Use async? > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:578 > + assert.deepStrictEqual(content['commitSets'][0].revisionItems, > + [{commit: '87832', commitOwner: null, patch: null, requiresBuild: false, rootFile: null}, {commit: '93116', commitOwner: null, patch: null, requiresBuild: false, rootFile: null}]); Please insert another line break in this long line. > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:581 > + assert.deepStrictEqual(content['commitSets'][1].revisionItems, > + [{commit: '87832', commitOwner: null, patch: null, requiresBuild: false, rootFile: null}, {commit: '96336', commitOwner: null, patch: null, requiresBuild: false, rootFile: null}]); Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:149 > + for (const commitSet of group.requestedCommitSets()) > + assert.strictEqual(parseInt(group.repetitionCountForCommitSet(commitSet)), 2); Just use + here. parseInt is overly permissive (like atoi, it would ignore trailing non-digit characters). > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:157 > + for (const commitSet of updatedGroups[0].requestedCommitSets()) > + assert.strictEqual(parseInt(updatedGroups[0].repetitionCountForCommitSet(commitSet)), 4); Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:181 > + for (const commitSet of group.requestedCommitSets()) > + assert.strictEqual(parseInt(group.repetitionCountForCommitSet(commitSet)), 2); Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:192 > + let result = await PrivilegedAPI.sendRequest('create-test-group', const? > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:198 > + const testGroups = await TestGroup.fetchForTask(result['taskId'], true); Please add /* noCache */ or define a local variable like const noCache = true, to make it clear what this true is. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:203 > + assert.strictEqual(parseInt(group.initialRepetitionCount()), 2); Ditto about using + and the rest of tests. We should also assert that the repetition type retuned by the server is "sequential" > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:205 > + for (const commitSet of group.requestedCommitSets()) > + assert.strictEqual(parseInt(group.repetitionCountForCommitSet(commitSet)), 2); We should also assert that requestedCommitSets().length is 2. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:207 > + const commitSet = testGroups[0].requestedCommitSets()[1].id(); Add assertions for order. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:211 > + const updatedGroups = await TestGroup.fetchForTask(result['taskId'], true); Ditto about true. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:215 > + const firstCommitSet = updatedGroups[0].requestedCommitSets()[0]; Need an assert for the length of commit sets here as well. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:221 > + assert.strictEqual(buildRequestsForSecondCommitSet.length, 4); Ditto about needing tests for order. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:224 > + it('should add build requests right after requests with same commit set for sequential test group', async () => { This is very wordy. How about this: it should be able to build requests for a specific commit set in a sequential test group. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:230 > + {name: 'test', taskName: 'other task', platform: MockData.somePlatformId(), repetitionCount: 2, > + test: MockData.someTestId(), needsNotification: false, repetitionType: 'sequential', revisionSets}); I don't think we should be nesting the indentation here. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:237 > + assert.strictEqual(parseInt(group.initialRepetitionCount()), 2); Use + here too. But we should also assert that the repetition type is "sequential". > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:239 > + assert.strictEqual(parseInt(group.repetitionCountForCommitSet(commitSet)), 2); We need to assert order here as well as the number of distinct commit sets. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:251 > + assert.strictEqual(buildRequestsForFirstCommitSet[3].order(), 3) We probably want to check all the orders here. We should also make sure build requests kept the same IDs & retained the original sorting order e.g. 1st build request for a commit set didn't become 3rd, etc... So just store the build request IDs early, and then assert that they're in the same order here. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:260 > + it('should not shift request order if adding build request to commit set representing last configuration in sequential test group', async () => { Hm... what's important here is the fact the order of the first configuration so I'd rephrase it like this: it should not modify the order of preceding build requests when adding new build requests in a sequential test group > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:264 > + let result = await PrivilegedAPI.sendRequest('create-test-group', const? > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:266 > + {name: 'test', taskName: 'other task', platform: MockData.somePlatformId(), repetitionCount: 2, > + test: MockData.someTestId(), needsNotification: false, repetitionType: 'sequential', revisionSets}); Wrong indentation again. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:269 > + const testGroups = await TestGroup.fetchForTask(result['taskId'], true); Ditto about noCache. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:274 > + for (const commitSet of group.requestedCommitSets()) Same thing about the number of distinct commit sets & order. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:318 > + await assertThrows('CommitSetNotInTestGroup', () => > + PrivilegedAPI.sendRequest('add-build-requests', {group: insertedGroupId, addCount: 2, commitSet})); Maybe we need another test case for a sequential test group? > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:341 > + await assertThrows('InvalidCommitSet', () => > + PrivilegedAPI.sendRequest('add-build-requests', {group: insertedGroupId, addCount: 2, commitSet})); Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:686 > + it('should create an analysis task with test group with "sequential" mode when specified', async () => { I think we can just say: it should be able to create an analysis task with a sequential test group > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:694 > + await db.insert('tests', {id: 1, name: 'Suite'}); > + await db.insert('tests', {id: test1Id, name: 'test1', parent: 1}); Do we really need two tests? It seems like one test will be enough? > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:735 > + assert.ok(testGroup.needsNotification()); Check the repetition type & repetition count here too? > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:754 > + it('should reject with "InvalidRepetitionType" when test mode is neither "alternating" nor "sequential"', async () => { Is "test mode" the old terminology we used? We should be consistent and say the "repetition type". Also, I think this will read better: "not 'alternating' or 'sequential'" > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:757 > + const test1Id = 2; Hm... maybe subTestId? "test1" is so non-descriptive. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:762 > + await db.insert('tests', {id: 1, name: 'Suite'}); > + await db.insert('tests', {id: test1Id, name: 'test1', parent: 1}); Do we really need two tests? It seems like a single test will suffice for this test?? > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:776 > + let test1 = Test.findById(test1Id); > + let somePlatform = Platform.findById(platformId); const? > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:786 > + await assertThrows('InvalidRepetitionType', () => > + PrivilegedAPI.sendRequest('create-analysis-task', {name: 'confirm', repetitionCount: 2, > + testGroupName: 'Confirm', revisionSets: [oneRevisionSet, anotherRevisionSet], repetitionType: 'invalid-mode', > + startRun: testRuns[0]['id'], endRun: testRuns[1]['id'], needsNotification: true})); These multiple line statements need { ~ } > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1342 > + it('should create a test group with an analysis task with "sequential" mode', async () => { just say: should create a sequential test group with an analysis task > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1357 > + assert.strictEqual(group.initialRepetitionCount(), 2); Check the repetition type. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1367 > + assert(requests[0].commitSet().equals(requests[1].commitSet())); Let's first assert that the number of distinct commit sets is 2. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1380 > + it('should reject with "InvalidMode" if test mode is neither "alternating" nor "sequential"', async () => { Ditto about the test name. > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:280 > + it('should cancel a test group with "may_need_more_requests" flag cleared', async () => { I think it would be better to say: should be able to cancel a test group and the "may need more requests" flag There is no reason to put the exact database column name in the description. > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:285 > + const result = await PrivilegedAPI.sendRequest('create-test-group', > + {name: 'test', taskName: 'other task', platform: MockData.somePlatformId(), test: MockData.someTestId(), needsNotification: false, revisionSets}); Should we add another test case for a sequential test group? > Websites/perf.webkit.org/server-tests/resources/mock-data.js:67 > + addMockData: function (db, statusList, needsNotification = true, urlList = null, commitSetList = null, repetitionType='alternating', mayNeedMoreRequests=false) Haha, you fixed the existing code but then forgot to add spaces around = in the new arguments :)
Comment on attachment 426409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426409&action=review >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:75 >> + array($incremental, $commit_set)); > > It's not safe to assume that request_commit_set will uniquely identify a test group. Please specify the group. > This also looks wrong. Because we have a unique constraint on (request_group, request_order), > this will fail if commit_set_id wasn't set so that we'd try to add more build requests to every commit set > since then request_order will start overlapping with later build requests. > r- because of this bug. I guess we don't have any tests for this case? > > I suggest we rewrite this so that we iterate over all commits sets in the reverse order, > and update build requests for each commit set (and test group). Oh, I know why we don't have a test for this. It's because this bug will only manifest when there are more than two commit sets involved. e.g. Say we have AABBCC, and want to add AA. We'd have to adjust the orders of BB and CC but the adjusted orders of BB will overlap with that of CC.
I think r- is still right given there are many changes we want to make before landing it.
Comment on attachment 426409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426409&action=review >> Websites/perf.webkit.org/ChangeLog:15 >> + Add retry logic for 'sequential' A/B testing and vove retry logic into syncing script so that retry can be > > Nit: vove?? I meant 'move'. >> Websites/perf.webkit.org/init-database.sql:295 >> + testgroup_repetition_type varchar(32) DEFAULT 'alternating', > > We should enum as I mention above. Will do >>> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:75 >>> + array($incremental, $commit_set)); >> >> It's not safe to assume that request_commit_set will uniquely identify a test group. Please specify the group. >> This also looks wrong. Because we have a unique constraint on (request_group, request_order), >> this will fail if commit_set_id wasn't set so that we'd try to add more build requests to every commit set >> since then request_order will start overlapping with later build requests. >> r- because of this bug. I guess we don't have any tests for this case? >> >> I suggest we rewrite this so that we iterate over all commits sets in the reverse order, >> and update build requests for each commit set (and test group). > > Oh, I know why we don't have a test for this. It's because this bug will only manifest when there are more than two commit sets involved. > e.g. Say we have AABBCC, and want to add AA. > We'd have to adjust the orders of BB and CC but the adjusted orders of BB will overlap with that of CC. Nice catch, will fix this and add unit tests for it. >> Websites/perf.webkit.org/public/v3/models/test-group.js:295 >> + return await this.fetchForTask(data['taskId'], true); > > It seems like we can just use fetchById now? create-test-group returns testGroupId too. Maybe we can, we will need to `return this.findAllByTask(data['taskId'])` afterwards, because we expect test groups under current analysis task on the place we use it. One thing that I'm kind afraid is that the code doesn't guarantee that all test groups under this task are fetched already. So far, the usage of this API is in the UI page, we shouldn't have this issue. >> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:418 >> + testGroup.repetitionCountForCommitSet(commitSet) - testGroup.initialRepetitionCount()); > > Why are we re-computing retry count here?? > Just call testGroup.retryCountForCommitSet. `retryCountForCommitSet` is an ambiguous name, I will rename it to `retryToAddForCommitSet`, which is the retry count we will add. The retryCount here means the number of retries we scheduled already. >> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:435 >> + `${retrySummaryParts.slice(0, -1).join(', ')} and ${retrySummaryParts[retrySummaryParts.length - 1]}`; > > Use new Intl.ListFormat('en', {style:'long', type:'conjunction'}).format(~)? > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat/ListFormat It would be nice to use this. However, the page says Safari hasn't supported it yet. >> Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:318 >> + PrivilegedAPI.sendRequest('add-build-requests', {group: insertedGroupId, addCount: 2, commitSet})); > > Maybe we need another test case for a sequential test group? If we don't support adding build requests for a specific commit set in alternating test group, then this should be sequential test group only. >> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:694 >> + await db.insert('tests', {id: test1Id, name: 'test1', parent: 1}); > > Do we really need two tests? It seems like one test will be enough? I think >> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:762 >> + await db.insert('tests', {id: test1Id, name: 'test1', parent: 1}); > > Do we really need two tests? It seems like a single test will suffice for this test?? Those will be created anyway by submitting reportWithRevision and anotherReportWithRevision. >> Websites/perf.webkit.org/server-tests/resources/mock-data.js:67 >> + addMockData: function (db, statusList, needsNotification = true, urlList = null, commitSetList = null, repetitionType='alternating', mayNeedMoreRequests=false) > > Haha, you fixed the existing code but then forgot to add spaces around = in the new arguments :) lol
Created attachment 427001 [details] Patch
Comment on attachment 426409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426409&action=review >>> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:694 >>> + await db.insert('tests', {id: test1Id, name: 'test1', parent: 1}); >> >> Do we really need two tests? It seems like one test will be enough? > > I think Updated in new patch.
(In reply to dewei_zhu from comment #16) > > >> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:435 > >> + `${retrySummaryParts.slice(0, -1).join(', ')} and ${retrySummaryParts[retrySummaryParts.length - 1]}`; > > > > Use new Intl.ListFormat('en', {style:'long', type:'conjunction'}).format(~)? > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat/ListFormat > > It would be nice to use this. However, the page says Safari hasn't supported > it yet. We definitely do. We just blogged about it! https://webkit.org/blog/11648/new-webkit-features-in-safari-14-1/
That's nice! For backward compatibility, we will use it when it's available.
(In reply to Ryosuke Niwa from comment #19) > (In reply to dewei_zhu from comment #16) > > > > >> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:435 > > >> + `${retrySummaryParts.slice(0, -1).join(', ')} and ${retrySummaryParts[retrySummaryParts.length - 1]}`; > > > > > > Use new Intl.ListFormat('en', {style:'long', type:'conjunction'}).format(~)? > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat/ListFormat > > > > It would be nice to use this. However, the page says Safari hasn't supported > > it yet. > > We definitely do. We just blogged about it! > https://webkit.org/blog/11648/new-webkit-features-in-safari-14-1/ That's nice! For backward compatibility, we will use it when it's available.
(In reply to dewei_zhu from comment #21) > (In reply to Ryosuke Niwa from comment #19) > > (In reply to dewei_zhu from comment #16) > > > > > > >> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:435 > > > >> + `${retrySummaryParts.slice(0, -1).join(', ')} and ${retrySummaryParts[retrySummaryParts.length - 1]}`; > > > > > > > > Use new Intl.ListFormat('en', {style:'long', type:'conjunction'}).format(~)? > > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat/ListFormat > > > > > > It would be nice to use this. However, the page says Safari hasn't supported > > > it yet. > > > > We definitely do. We just blogged about it! > > https://webkit.org/blog/11648/new-webkit-features-in-safari-14-1/ > > That's nice! For backward compatibility, we will use it when it's available. I don't think we need to maintain backward compatibility for that long. Historically, we only kept it until a stable release has shipped. Nobody who is using this website should really be using Safari older than 14.1.
Comment on attachment 427001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427001&action=review > Websites/perf.webkit.org/browser-tests/test-group-form-tests.js:25 > + it('must dispatch "startTesting" action with the repetition type user selects on "Start A/B testing"', () => { Use async? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:37 > + exit_with_error('ShouldNotSpecifyCommitSetForAlternatingRequest'); CommitSetNotSupportedForAlternatingRepetitionType? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:51 > - foreach ($existing_build_requests as $build_request) { > + foreach ($build_requests as $build_request) { Again, we shouldn't rename this variable! The fact these are existing build requests is quite significant. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:53 > if ($build_request['request_order'] < 0) > continue; It seems like we always skip build build-requests in all cases. We should probably do that in "main" before calling these functions. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:62 > - for ($i = 0; $i < $additional_build_request_count; $i++) { > + for ($i = 0; $i < $add_count; $i++) { Ditto. Please stick to the more verbose form of $additional_build_request_count > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:71 > +function add_build_request_for_sequential_test_group($db, $build_requests, $add_count, $commit_set_id, $test_group_id) Please rename $add_count to $additional_build_request_count and $commit_set_id to something like $target_commit_set_id Also, given $target_commit_set_id is an optional argument, it's good to put it at the end and add the default value in the argument list. That would signify to the reader of this function that this argument is optional. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:73 > + $order_incremental_by_commit_set = array(); "incremental" is an adjective. I don't think we need a noun here: $order_increment_by_commit_set > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:74 > + $commit_sets = array(); Again, this variable needs to be more descriptive. It seems like this contains the list of commit sets for which we're adding new build request. But it's also confusing that such a thing exists when $target_commit_set is specified! > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:76 > + foreach ($build_requests as $build_request) { This should be called $existing_build_requests. Also we should probably call the latter variable $current_build_request to further differentiate from the whole list. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:79 > + $requested_commit_set = $build_request['request_commit_set']; It's very confusing to call this variable $requested_commit_set given we also have $commit_set_id, which is used to add new build requests. We should probably call this $commit_set_for_current_request instead. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:97 > + if (!$commit_sets) > + exit_with_error('NoCommitSetInTestGroup'); This is a convoluted way of checking that when $target_commit_set_id isn't found in build_requests. I think it's a lot clearer if the above loop was split into two separate functions. The one for a specific commit set can be as simple as this: $found_target = false; foreach ($existing_build_requests as $current_build_request) { if ($build_request['request_order'] < 0) // This should be moved to main. continue; if ($current_build_request['request_commit_set'] == $target_commit_set) { $found_target = true; break; } } if (!$found_target) exit_with_error('NoCommitSetInTestGroup'); $db->begin_transaction(); foreach (array_reverse($existing_build_requests, true) as $current_build_request) { if ($build_request['request_order'] < 0) // This should be moved to main. continue; if ($current_build_request['request_commit_set'] == $target_commit_set) { for ($i = 0; $i < $additional_build_request_count; $i++) duplicate_build_request_with_new_order($db, $current_build_request, $current_build_request['request_order'] + $i + 1); break; } else { $db->query('UPDATE build_requests SET request_order = request_order + $1 WHERE request_id = $2', array($additional_build_request_count, $build_request['request_id'])); } } > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:117 > +function insert_build_requests($db, $build_request, $order_overwrite) This only adds a single build request so it's confusing to call it insert_build_request*s*. Also, this thing takes an existing build request and simply overrides order so we should convey that in its name. e.g. duplicate_build_request_with_new_order > Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:25 > + exit_with_error('InvalidRepetitionType', array('mode' => $repetition_type)); This should say 'repetitionType' => $repetition_type > Websites/perf.webkit.org/public/v3/components/test-group-form.js:27 > + const repetitionTypeSelect = this.content('repetition-type'); > + repetitionTypeSelect.onchange = () => this._repetitionType = repetitionTypeSelect.value; Since this value is never used elsewhere, it's better to get it in startTesting() > Websites/perf.webkit.org/public/v3/components/test-group-form.js:73 > + <option selected value="alternating">alternative (ABAB)</option> in "alterna*te*", not "alterna*tive*" > Websites/perf.webkit.org/public/v3/models/test-group.js:104 > + willExceedMaxRetryForCommitSetWithRetryCount(commitSet, retryCount, maxRetryFactor) This is a very wordy function name. Not being able to come up with a good function name is usually an indication that we have a bad abstraction. Here, I think it would be better to compute the product of maxRetryFactor and this.initialRepetitionCount() in the script script side and simply pass in retryCountLimit as an argument to other functions. > Websites/perf.webkit.org/public/v3/models/test-group.js:109 > + retryToAddForCommitSet(commitSet) I would call this something like additionalRepetitionNeededToReachInitialRepetitionCount > Websites/perf.webkit.org/public/v3/models/test-group.js:116 > + scheduledRetryCountForCommitSet(commitSet) This is rather confusing name given BuildRequest has a statue value of "scheduled". This is really retryCountForCommitSet. > Websites/perf.webkit.org/public/v3/models/test-group.js:121 > + scheduledRetryCountsAreIdentical() Same issue here. Also, it's unclear they're identical across what. Maybe retryCountsAreSameForAllCommitSets? > Websites/perf.webkit.org/public/v3/models/test-group.js:127 > + async scheduleRetriesIfNecessary(maxRetryFactor) Please add these functions right beneath create* functions. > Websites/perf.webkit.org/public/v3/models/test-group.js:135 > + if (!this.mayNeedMoreRequests()) > + return 0; > + > + if (this.isHidden()) { > + await this.clearMayNeedMoreBuildRequests(); > + return 0; > + } This doesn't seem like something related to scheduling retries. We probably shouldn't put them here. > Websites/perf.webkit.org/public/v3/models/test-group.js:150 > + hasCompletedBuildRequestForEachCommitSet = hasCompletedBuildRequestForEachCommitSet && !!buildRequests.filter((request) => request.hasCompleted()).length; > + hasUnfinishedBuildRequest = hasUnfinishedBuildRequest || !!buildRequests.filter((request) => !request.hasFinished()).length; It's very confusing / hard-to-follow to have stateful variables like these which get updated over loop iterations. It's better to repeat the loop instead: const eachCommitSetHasCompletedAtLeastOneTest = this.requestedCommitSets().every((set) => this.requestsForCommitSet(set).some((request) => request.isTest() && request.hasCompleted())); const hasUnfinishedTestRequests = this.requestedCommitSets().some((set) => this.requestsForCommitSet(set).some((request) => request.isTest() && !request.hasFinished())); > Websites/perf.webkit.org/public/v3/models/test-group.js:152 > + maxRetryCount = Math.max(maxRetryCount, this.retryToAddForCommitSet(commitSet)); My goodness, I got totally fooled by the fact this variable is called maxRetryCount, thinking that this is the pre-existing retry count. This should really be called additionalRepetitionNeeded > Websites/perf.webkit.org/public/v3/models/test-group.js:155 > + const willExceedMaxRetry = this.requestedCommitSets().some(commitSet => > + this.willExceedMaxRetryForCommitSetWithRetryCount(commitSet, maxRetryCount, maxRetryFactor)); Maybe call this willExceedRetryLimit? > Websites/perf.webkit.org/public/v3/models/test-group.js:157 > + console.assert(maxRetryCount <= this.initialRepetitionCount()); We'd assume that maxRetryFactor < 2? This seems rather arbitrary thing to assert in TestGroup. > Websites/perf.webkit.org/public/v3/models/test-group.js:161 > + if (maxRetryCount && !willExceedMaxRetry && hasUnfinishedBuildRequest && !hasCompletedBuildRequestForEachCommitSet) > + return 0; > + > + if (maxRetryCount && !willExceedMaxRetry && hasCompletedBuildRequestForEachCommitSet) It seems like these two if's share the same condition: maxRetryCount && !willExceedMaxRetry We should probably nest if's here. > Websites/perf.webkit.org/public/v3/models/test-group.js:163 > + await this.clearMayNeedMoreBuildRequests(); Again, I don't think this is a side effect that's clear from the function name. It probably shouldn't be a part of this function. Maybe we can use NaN to indicate we've exceeded the maximum limit? Alternatively we can return a dictionary but that might be an over engineering. > Websites/perf.webkit.org/public/v3/models/test-group.js:172 > + console.assert(commitSets.every((currentSet, index, setList) => > + !index || lastItem(this.requestsForCommitSet(setList[index - 1])).order() < this.requestsForCommitSet(currentSet)[0].order())); We can just use commitSets[index - 1] instead of setList since we've defined it as a separate variable. > Websites/perf.webkit.org/public/v3/models/test-group.js:174 > + const failedAllForOneCommitSet = commitSets.some(commitSet => Rename this to something like: allTestRequestsHaveFailedForSomeCommitSet > Websites/perf.webkit.org/public/v3/models/test-group.js:178 > + if (failedAllForOneCommitSet) { > + await this.cancelPendingRequests(); Again, this seems like something the syncing script should have a distinct step, not something this function should be doing as a part of creating more retires if needed. > Websites/perf.webkit.org/public/v3/models/test-group.js:184 > + request.isTest() && request.hasCompleted()).length; This value appears so frequently that we should have a helper function like testGroup.successfulTestRequestCount > Websites/perf.webkit.org/public/v3/models/test-group.js:192 > + const retryCount = this.retryToAddForCommitSet(commitSet); > + return !this.willExceedMaxRetryForCommitSetWithRetryCount(commitSet, retryCount, maxRetryFactor); > + }); > + > + if (firstCommitSetWithRetryAllowed && this.retryToAddForCommitSet(firstCommitSetWithRetryAllowed)) { > + const retryCount = this.retryToAddForCommitSet(firstCommitSetWithRetryAllowed); We're calling this.retryToAddForCommitSet three times here! Why can't we just do this: for (const currentCommitSet of commitSets) { if (testGroup.successfulTestRequestCount(currentCommitSet) >= this.initialRepetitionCount()) continue; const retryCount = this.additionalRepetitionNeededToReachInitialRepetitionCount(currentCommitSet); if (retryCount + this.initialRepetitionCount() > retryCountLimit) continue; return this.addMoreBuildRequests(retryCount, currentCommitSet); } > Websites/perf.webkit.org/public/v3/models/test-group.js:340 > + async addMoreBuildRequests(addCount, commitSet = null) We should take commitSet object, not ID. > Websites/perf.webkit.org/public/v3/models/test-group.js:342 > + console.assert(!commitSet || this.repetitionType() == 'sequential'); Assert that commitSet instanceof CommitSet > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:371 > + let retrySummary = null; This variable is assigned once and never used. Remove? > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:372 > + let requestSummary = null; This variable seems unnecessary. We can just call this.content('request-summary') inside the call to renderReplace. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:373 > + let hideButton = null; Ditto. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:374 > + let pendingRequestCancelWarning = null; Ditto. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:395 > + pendingRequestCancelWarning = this.content('pending-request-cancel-warning'); > + this.renderReplace(pendingRequestCancelWarning, '(cancels pending requests)'); We need to clear the warning when it doesn't have a pending requests: this.renderReplace(this.content('pending-request-cancel-warning'), currentGroup.hasPending() ? '(cancels pending requests)' : []); > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:411 > + retryCountByLabel.set(testGroup.labelForCommitSet(commitSet), > + testGroup.repetitionCountForCommitSet(commitSet) - testGroup.initialRepetitionCount()); This is just testGroup.retryCountForCommitSet(commitSet). We should just get rid of this code entirely. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:424 > + const retrySummary = retrySummaryParts.length === 1 ? retrySummaryParts[0] : > + `${retrySummaryParts.slice(0, -1).join(', ')} and ${retrySummaryParts[retrySummaryParts.length - 1]}`; Again, use Intl.ListFormat > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:934 > + const testGroups = TestGroup.createAndRefetchTestGroups(this._task, testGroupName, repetitionCount, repetitionType, commitSets, notifyOnCompletion); Don't we need to await here??? > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:952 > + this._didFetchAnalysisResults(testGroups) Missing semicolon at the end. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:128 > + repetitionType.disabled = shouldDisable; Huh, this is really wrong. We should be scheduling a render call and then updating these states there instead. I guess we'd fix it in a separate patch since this patch is getting really big now. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:599 > + <option value="alternating" selected>alternative (ABAB)</option> In *alternate*, not *alternative* > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:207 > + let group = testGroups[0]; const? > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:217 > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 2]); Nice! > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1342 > + it('should create a sequential test group with an analysis task', async () => { We need another test to create a test group for an existing analysis task. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1382 > + it('should reject with "InvalidMode" if test mode is not "alternating" or "sequential"', async () => { The error code here should be updated per my comment above. > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:280 > + it('should be able to cancel an alternating test group and the "may need more requests" flag', async () => { and *clear* the "may need more requests" flag > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:321 > + it('should be able to cancel a sequential test group and the "may need more requests" flag', async () => { Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1500 > + it('should not scheduling build request from another test group on a builder if a alternating test group with all existing build requests finished still needs retry', async () => { if *an* alternating test group > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1626 > + it('should not reuse the root when a build request with same commit set is available but the build request has been scheduled', async () => { I guess we should add another test which is that when all requests for the first commit set fails in a sequential test group, we'd cancel the rest. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:171 > + if (latestActiveBuildRequest.testGroupId() === newRequest.testGroupId()) Just use ==. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:175 > + if (!testGroup) > + continue; When does this happen? It seems like unsafe to assume the worker is not used just because we couldn't find the test group? In fact, if this happens when there is a build request not associated with a test group known to us, we probably must assume that request needs to be processed or we need to sync up with the dashboard before we can go ahead & schedule a new job. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:176 > + if (testGroup.hasFinished() && !testGroup.mayNeedMoreRequests()) Hm... can we add a helper function to TestGroup for this? Also, it's confusing that testGroup.mayNeedMoreRequests() can return true even when testGroup.hasFinished() is false. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:161 > + const orderedBuildRequests = testGroup.orderedBuildRequests(); > + const buildRequestIndex = orderedBuildRequests.indexOf(buildRequest); > + if (!buildRequestIndex) > + return false; > + > + const buildRequestBeforeCurrent = orderedBuildRequests[buildRequestIndex - 1]; > + if (buildRequestBeforeCurrent.isBuild()) > + return false; > + > + const previousCommitSet = buildRequestBeforeCurrent.commitSet() > + if (previousCommitSet === buildRequest.commitSet()) > + return false; I think we should add two helper functions to TestGroup instead. 1. testGroup.isFirstTestRequest(buildRequest). This could alternatively be a method on buildRequest as well. 2. testGroup.precedingBuildRequest(buildRequest) > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:163 > + if (testGroup.willExceedMaxRetryForCommitSetWithRetryCount(previousCommitSet, 0, this._maxRetryFactor)) I think it's better if we passed in the retry count limit as opposed to a factor since the factor is more of a syncing script concept. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:167 > + return testGroup.initialRepetitionCount() > testGroup.requestsForCommitSet(buildRequestBeforeCurrent.commitSet()) > + .filter(request => request.isTest() && request.hasCompleted()).length; This too should probably be a helper function on TestGroup. e.g. testGroup.successfulTestCount(commitSet) > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:625 > + describe('createAdditionalBuildRequestsForTestGroupsWithFailedRequests', () => { should be renamed to scheduleRetriesIfNecessary
Created attachment 427546 [details] Patch
Created attachment 427601 [details] Patch
Comment on attachment 427601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427601&action=review > Websites/perf.webkit.org/ChangeLog:9 > + Add support to schedule 'sequential' A/B testing which runs all A runs before B runs, whereas previously we only Nit: schedule *a* "sequential" A/B testing Nit: probably say run all the iterations for A configuration then all the iterations for B configuration to be consistent with the next line. Nit: no comma is needed before whereas. Also, we previously only support*ed* There is also a subject mismatch here. How about this: Add support to schedule 'sequential' A/B testing which schedules all iterations for the first configuration (A) before the iterations for the second configuration (B). Before this patch, all A/B testing alternated between two different configurations for each iteration. > Websites/perf.webkit.org/ChangeLog:10 > + support alternating A and B configurations between runs. Nit: alternating between A & B configurations. > Websites/perf.webkit.org/ChangeLog:13 > + Update syncing script to not proceed with next configuration until current configuration meets initial requested Hm... this reads awkwardly. How about this: ... until the current configuration successfully completed more iterations than the initially requested or retries exceeded the list. > Websites/perf.webkit.org/public/include/commit-sets-helpers.php:12 > $group_id = $db->insert_row('analysis_test_groups', 'testgroup', We should probably add an assertion here that $repetition_type is either sequential or alternating. I know we're checking in this function's call sites already but it's good to assert that so that if someone adds a new repetition type or adds a new code without a check, we'd catch it here. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:16 > + $additional_build_request_count = array_get($arguments, 'addCount'); Why not just $arguments['addCount'] since we've forced that addCount exists above. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:17 > + $test_group_id = array_get($arguments, 'group'); Ditto. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:18 > + $commit_set_id = array_get($arguments, 'commitSet', NULL); No need to specify NULL here. That's the default value. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:44 > + add_build_request_for_alternating_test_group($db, $existing_test_type_build_requests, $additional_build_request_count, $current_order); Hm... how about just add_alternating_build_requests? It's pretty clear that adding alternating build requests must happen for alternating test group, right? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:47 > + add_build_requests_for_specific_commit_set_in_sequential_test_group($db, $existing_test_type_build_requests, $additional_build_request_count, $commit_set_id); How about add_sequential_build_requests_for_commit_set? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:49 > + add_build_requests_for_all_commit_sets_in_sequential_test_group($db, $existing_build_requests, $additional_build_request_count, $test_group_id); And add_sequential_build_requests_for_all_commit_sets? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:57 > + $target_commit_sets = array(); I don't think it makes sense to rename this to target_commit_sets since we'd be adding new build requests for all commit sets. It also makes diff needlessly larger. We should stick with the original variable name. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:80 > + $target_commit_sets = array(); I don't think we need this separate array of commit sets. We can just use array_keys($build_request_by_commit_set) instead. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:85 > + $build_request_by_commit_set[$commit_set_for_current_request] = $current_build_request; We should call this $last_build_request_for_commit_set. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:96 > + $db->query('UPDATE build_requests SET request_order = request_order + $1 Did you check that this query is efficient? e.g. does it use the index for build_request_order_must_be_unique_in_group? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:101 > + $current_build_request = $build_request_by_commit_set[$commit_set]; It's very confusing to call this *current* build request since this function adds more build requests. And it's unclear what *current* means in this context. How about $last_existing_build_request? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:103 > + $request_order_incremental = array_get($order_increment_by_commit_set, $commit_set, 0); Hm... it might be more appropriate to call this $order_diff or $order_shift. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:105 > + for ($i = 0; $i < $additional_build_request_count; $i++) > + duplicate_build_request_with_new_order($db, $current_build_request, ++$request_order + $request_order_incremental); It's very confusing that both $i and $request_order are getting incremented. I think it would be more clear to do: $last_existing_order + $order_shift + $i > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:129 > + } > + else { Nit: should be: } else { > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:134 > + $db->commit_transaction(); Very nice! This code is so much easier to understand! > Websites/perf.webkit.org/public/v3/models/test-group.js:107 > + willExceedRetryLimitWithRetryCount(commitSet, retryCount, retryLimit) > + { > + return retryCount + this.repetitionCountForCommitSet(commitSet) > retryLimit; > + } Hm... I don't think this function is necessary. We can just do this math where it's called. It's better that we can see this arithmetic in its call site so we know exactly what we're checking. > Websites/perf.webkit.org/public/v3/models/test-group.js:116 > + successfulTestRequestCount(commitSet) I think we can just call this: successfulTestCount > Websites/perf.webkit.org/public/v3/models/test-group.js:123 > + return !this._orderedBuildRequests().filter((request) => request.isTest()).indexOf(buildRequest); This will search the entire array for every build request. We can just do this instead since filter will always return an array and [][0] is undefined: this._orderedBuildRequests().filter((request) => request.isTest())[0] == buildRequest; > Websites/perf.webkit.org/public/v3/models/test-group.js:132 > + if (buildRequestIndex < 1) > + return null; > + return orderedBuildRequests[buildRequestIndex - 1]; There is no need to check whether buildRequestIndex is 0 or -1 since [][-1] and [][-2] are both undefined. If we really wanted to return null instead of undefined, we can just do this: orderedBuildRequests[buildRequestIndex - 1] || null. > Websites/perf.webkit.org/public/v3/models/test-group.js:137 > + return this.repetitionCountForCommitSet(commitSet) - this.initialRepetitionCount(); Let's assert that this number is greater than or equal to 0. > Websites/perf.webkit.org/public/v3/models/test-group.js:151 > + const additionalRepetitionNeeded = Math.max(...this.requestedCommitSets().map( > + this.additionalRepetitionNeededToReachInitialRepetitionCount.bind(this))); Hm... this is kind of hard to read and it creates a temporary array. How about this? const additionalRepetitionNeeded = this.requestedCommitSets().reduce((commitSet, current) => Math.max(current, this.additionalRepetitionNeededToReachInitialRepetitionCount(commitSet)), 0); > Websites/perf.webkit.org/public/v3/models/test-group.js:165 > + if (hasUnfinishedBuildRequest && !eachCommitSetHasCompletedAtLeastOneTest) > + return doNotClearMayNeedMoreRequestsFlag; Hm... now that I'm looking at this code, it's very much mysterious that we're returning NaN here. Maybe we should return a dictionary instead: {retryCount: ~, mayNeedMoreRequests: ~} > Websites/perf.webkit.org/public/v3/models/test-group.js:183 > + const allTestRequestsHaveFailedForSomeCommitSet = commitSets.some(commitSet => > + this.requestsForCommitSet(commitSet).filter(request => request.isTest()).every(request => request.hasFailed())); Why not just: this.requestsForCommitSet(commitSet).every((request) => !request.isTest() || request.hasFailed()) > Websites/perf.webkit.org/public/v3/models/test-group.js:196 > + await this.clearMayNeedMoreBuildRequests(); I don't think we should be calling this.clearMayNeedMoreBuildRequests here? > Websites/perf.webkit.org/public/v3/models/test-group.js:200 > + return 0; Looks like we're missing the case where this function will return NaN? I don't think it's correct that we'd always clear may-need-more-build-requests flag? > Websites/perf.webkit.org/public/v3/models/test-group.js:-105 > - const commitSetLabelMap = new Map; lol. > Websites/perf.webkit.org/public/v3/models/test-group.js:327 > - async didSendNotification() > + async cancelPendingRequests() Can we re-order functions so that didSendNotification will be left intact? > Websites/perf.webkit.org/public/v3/models/test-group.js:337 > + return this._updateBuildRequest({ Why the change (remove await)? > Websites/perf.webkit.org/public/v3/models/test-group.js:357 > - return await this._updateBuildRequest({ > + return this._updateBuildRequest({ I'm so confused about all these changes to remove await. There is no harm in awaiting another async function. Why don't we revert that since this patch is already so massive? > Websites/perf.webkit.org/public/v3/models/test-group.js:369 > + taskName, name: groupName, platform: platform.id(), test: test.id(), repetitionCount, revisionSets, > + repetitionType, needsNotification: !!notifyOnCompletion}); Can we put repetitionCount and repetitionType next to each other? > Websites/perf.webkit.org/public/v3/models/test-group.js:370 > + const task = await AnalysisTask.fetchById(data['taskId'], true); Please either add comment like /* ignoreCache */ or define a local variable to indicate what "true" means here. > Websites/perf.webkit.org/public/v3/models/test-group.js:381 > + task: task.id(), name: groupName, platform: platform.id(), test: test.id(), repetitionCount, > + revisionSets, repetitionType, needsNotification: !!notifyOnCompletion}); Ditto about putting repetitionCount and repetitionType putting next to each other. > Websites/perf.webkit.org/public/v3/models/test-group.js:382 > + await this.fetchById(data['testGroupId'], true); Ditto about "true". > Websites/perf.webkit.org/public/v3/models/test-group.js:391 > + task: task.id(), name, repetitionCount, revisionSets, repetitionType, Ditto. > Websites/perf.webkit.org/public/v3/models/test-group.js:394 > + await this.fetchById(data['testGroupId'], true); Ditto. > Websites/perf.webkit.org/public/v3/models/test-group.js:398 > + async scheduleRetriesIfNecessary(maxRetryFactor) I don't think there is much value in having this function being separate from resolveMayNeedMoreRequestsFlag. > Websites/perf.webkit.org/public/v3/models/test-group.js:402 > + return this.repetitionType() == 'alternating' ? this._createAlternatingRetriesForTestGroup(maxRetryFactor) > + : this._createSequentialRetriesForTestGroup(maxRetryFactor); Hm... I think this code will be easier to read with if like this: if (this.repetitionType() == 'sequential') return this._createSequentialRetriesForTestGroup(maxRetryFactor); return this._createAlternatingRetriesForTestGroup(maxRetryFactor); > Websites/perf.webkit.org/public/v3/models/test-group.js:405 > + async resolveMayNeedMoreRequestsFlag(maxRetryFactor) I don't think this function name is descriptive. I'd much rather call this function scheduleRetriesIfNecessary. > Websites/perf.webkit.org/public/v3/models/test-group.js:415 > + const retryCount = await this.scheduleRetriesIfNecessary(maxRetryFactor); If we made _createAlternatingRetriesForTestGroup and _createSequentialRetriesForTestGroup return a dictionary as I suggested above, we can do something like this: const {retryCount, mayNeedMoreRequests} = await this. _createAlternatingRetriesForTestGroup(maxRetryFactor); > Websites/perf.webkit.org/public/v3/models/test-group.js:422 > + console.log(`Added ${retryCount} build request(s) to "${testGroup.name()}" of analysis task: ${analysisTask.id()} - "${analysisTask.name()}"`); I don't think we should be logging in this public API of TestGroup since logging is really a concept of syncing scripts. We should do this in the call site (processTestGroupMayNeedMoreRequests) instead. > Websites/perf.webkit.org/public/v3/models/test-group.js:433 > + const data = await this.cachedFetch(`/api/test-groups/${testGroupId}`, {}, ignoreCache); Nit: { } instead of {}. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:377 > + const authoredBy = currentGroup.author() ? `by "${currentGroup.author()}" ` : ''; Nit: there is no need to have a trailing space here. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:397 > + for (const commitSet of testGroup.requestedCommitSets()) > + retryCountByLabel.set(testGroup.labelForCommitSet(commitSet), testGroup.retryCountForCommitSet(commitSet)); I don't think this upfront collection of labels & retry count is necessary. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:400 > + return `${testGroup.initialRepetitionCount()} requested, ${retryCountByLabel.get('A')} added due to failures.`; We can just do this here: const retryCount = testGroup.retryCountForCommitSet(testGroup.requestedCommitSets()[0]); > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:407 > + for (const [label, count] of retryCountByLabel.entries()) { > + if (!count) > + continue; > + retrySummaryParts.push(`${count} added to ${label} configuration`); > + } Then here, we can just do this: retrySummaryParts = testGroup.requestedCommitSets() .map((commitSet) => ({commitSet, retryCount: testGroup.retryCountForCommitSet(commitSet))) .filter((item) => item.retryCount) .map((item) => `${item.retryCount} added to ${testGroup.labelForCommitSet(item.commitSet)} configuration`) > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:408 > + const retrySummary = new Intl.ListFormat('en', {style:'long', type:'conjunction'}).format(retrySummaryParts); Nit: Need a space after : > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:125 > + // FixMe: should invoke render code instead. Nit: should be FIXME. Also, should call enqueueToRender > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:204 > + const noCache = true; Nor that it's a big deal or it's an issue with your patch in particular but we should be consistent with noCache vs ignoreCache... > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:218 > + const firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 2]); > + const secondCommitSet = testGroups[0].requestedCommitSets()[1]; Looks like we can just use `group` here? > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:249 > + let firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 1]); > + let secondCommitSet = testGroups[0].requestedCommitSets()[1]; Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:289 > + let firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 1]); > + let secondCommitSet = testGroups[0].requestedCommitSets()[1]; Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:332 > + let firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 1]); > + let secondCommitSet = testGroups[0].requestedCommitSets()[1]; > + assertOrderOfRequests(group.requestsForCommitSet(secondCommitSet), [2, 3]); > + let thirdCommitSet = testGroups[0].requestedCommitSets()[2]; Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:377 > + let firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 1]); > + let secondCommitSet = testGroups[0].requestedCommitSets()[1]; > + assertOrderOfRequests(group.requestsForCommitSet(secondCommitSet), [2, 3]); > + let thirdCommitSet = testGroups[0].requestedCommitSets()[2]; Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:421 > + const firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 1]); > + const secondCommitSet = testGroups[0].requestedCommitSets()[1]; Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:454 > + const firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 1]); > + const secondCommitSet = testGroups[0].requestedCommitSets()[1]; Ditto. We probably don't need to check / assert request orders here though since that's not the point of this test. > Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:487 > + const firstCommitSet = testGroups[0].requestedCommitSets()[0]; > + assertOrderOfRequests(group.requestsForCommitSet(firstCommitSet), [0, 2]); > + const secondCommitSet = testGroups[0].requestedCommitSets()[1]; Ditto. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1382 > + it('should create a sequential test group with an analysis task', async () => { Is this a duplicate test case?? It has the same description as the preceding test case, and I can't spot a difference. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1434 > + assert.strictEqual(testGroups.length, 1); Can we check that this was 0 before calling PrivilegedAPI.sendRequest above? Just to make sure this test doesn't get broken in the future when someone decides to modify addTriggerableAndCreateTask. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1462 > + it('should reject with "InvalidRepetitionType" if test mode is not "alternating" or "sequential"', async () => { Can we add another test case where we try to do the same with an existing analysis task? > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:280 > + it('should be able to cancel an alternating test group and clear the "may need more requests" flag', async () => { We should add another test case to make sure mayNeedMoreRequests doesn't get cleared as well. > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:321 > + it('should be able to cancel a sequential test group and clear the "may need more requests" flag', async () => { Ditto especially since this patch seems to have a bug there. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1363 > + it('should schedule a build request on a builder the last build request of which is not fetched yet', async () => { This sentence isn't grammatically correct. Did you mean to say something along the line of: should schedule a build request on a builder when its recent builds had not been fetched yet > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1399 > + await MockRemoteAPI.waitForRequest(); What is this MockRemoteAPI.waitForRequest for?? > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1466 > + assert.equal(BuildRequest.findById(701).status(), 'failed'); > + assert.equal(BuildRequest.findById(701).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); I'm a bit confused here. Don't we need to be checking the status of 702 & 703 that they're not getting scheduled?? > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1474 > + assert.equal(BuildRequest.findById(701).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1477 > + it('should not scheduling other build request on a builder if a sequential test group is scheduled on it and waiting for retry', async () => { NutL should not *schedule* another build request > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1483 > + await Promise.all([MockData.addMockData(db, ['failed', 'completed', 'pending', 'pending'], true, > + ['http://build.webkit.org/#/builders/2/builds/123', 'http://build.webkit.org/#/builders/2/builds/124', null, null], > + [401, 401, 402, 402], 'sequential', true), > + MockData.addAnotherMockTestGroup(db), > + ]); Indention here is super confusing. Since the perf isn't all that important, why don't we just serialize them? await MockData.addMockData(~); await MockData.addAnotherMockTestGroup(db); > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1528 > + assert.equal(BuildRequest.findById(701).status(), 'completed'); > + assert.equal(BuildRequest.findById(701).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); Again, I think what we should be checking is the status of 710-713. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1536 > + assert.equal(BuildRequest.findById(701).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1539 > + it('should not scheduling build request from another test group on a builder if an alternating test group with all existing build requests finished still needs retry', async () => { Nit: This sentence isn't well formed. How about this: should not schedule a build request from another test group on a builder which is used by an alternating test group needing more retries. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1591 > + assert.equal(BuildRequest.findById(703).status(), 'completed'); > + assert.equal(BuildRequest.findById(703).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); Ditto about checking the status of 710-713. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1599 > + assert.equal(BuildRequest.findById(703).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1602 > + it('should not scheduling build request from another test group on a builder if a sequential test group with all existing build requests finished still needs retry', async () => { I'd suggest the same rephrasing. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1654 > + assert.equal(BuildRequest.findById(703).status(), 'completed'); > + assert.equal(BuildRequest.findById(703).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1662 > + assert.equal(BuildRequest.findById(703).status(), 'completed'); > + assert.equal(BuildRequest.findById(703).statusUrl(), 'http://build.webkit.org/#/builders/2/builds/124'); Ditto. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:146 > + const latestActiveEntryByWorkerName = new Map; latestEntryByWorkerName would suffice. Introducing a new term like "active" is rather confusing. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:159 > + if (entry.hasFinished() || entry.isInProgress()) { Isn't this just !entry.isPending()? > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:175 > + if (!testGroup) > + continue; We should add a comment as to when this `continue` would run since it looks as if this condition should never be reached at glance. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:89 > + let rootReuseUpdates = {}; Nit: { } instead of {} > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:93 > + await Promise.all([...buildRequestsByGroup.keys()].map(testGroupId => TestGroup.fetchById(testGroupId, true))); Why not Array.from(buildRequestsByGroup.keys())? > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:141 > + _shouldDeferSequentialTestingRequestWithNewCommitSet(buildRequest) Nit: Call it TestRequest for consistency. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:161 > + if (testGroup.willExceedRetryLimitWithRetryCount(previousCommitSet, 0, retryLimit)) This is an example of code which will read a lot better if the function was inlined: if (this.repetitionCountForCommitSet(previousCommitSet) >= retryLimit) return false; Do we have a test for this?? It seems like the condition here is slightly off. We need to return false even when repetition count is identically equal to the retry limit. > Websites/perf.webkit.org/tools/run-analysis.js:71 > +async function processTestGroupMayNeedMoreRequests(maxRetryFactor) Can we merge this into analysisLoop? I don't think it's helpful to split this logic into a separate function since it's basically single for loop. > Websites/perf.webkit.org/tools/sync-buildbot.js:15 > + {name: '--max-retry-factor', type: parseFloat, default: 3}, Huh, it is concerning that run-analysis.js and run-analysis.js in theory can get different values for --max-retry-factor. Not in this patch but we should figure out a way to not duplicate this value in two different syncing scripts' configurations. This is a timing time bomb. > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:626 > + let requests = MockRemoteAPI.inject(null, NodePrivilegedAPI); const? > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:649 > + it('should add 2 more build requests when 2 failed build request found for a commit set', async () => { For consistency, we should spell out 2 as two. > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:675 > + it('should not schedule more build requests when build request is hidden', async () => { when the build requests are hidden? But this is about the test group, right? > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:702 > + it('should not schedule more build request when we\'ve already hit the maximum retry count', async () => { Nit: should not schedule more build requests when doing so will exceed the retry limit > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:716 > + it('should not schedule more when additional build requests are still pending', async () => { schedule more *build requests* when there are still pending build requests > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:739 > + it('should add two more build request on A config and clear "may need more requests" afterwards when one A config request failed for test group in sequential mode', async () => { Nit: should *schedule* retry build request*s* and clear "may need more requests" flag when all requests for a configuration had failed > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:765 > + it('should retry B config if A config exhausted retry and had at least one successful run but less than user requested for test group in sequential mode', async () => { Nit: should retry the second configuration (B) if the first configuration (A) reached the retry limit but had at least one successful iteration for a sequential test group. There is no need to say it's less than initially requested repetition count since that's implied by the fact we've reached the retry limit. > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:781 > + it('should clear test group "may need more requests" flag in sequential mode if all user initially scheduled A runs failed', async () => { I don't think "user" here adds any valuable information so we should just get rid of it. Also, let's stick with repetitions or iterations since that's the term we use.
Comment on attachment 427601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427601&action=review >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:96 >> + $db->query('UPDATE build_requests SET request_order = request_order + $1 > > Did you check that this query is efficient? e.g. does it use the index for build_request_order_must_be_unique_in_group? Yes, I think I have a unit test for this. It will fail if array_reverse is not used. >> Websites/perf.webkit.org/public/v3/models/test-group.js:196 >> + await this.clearMayNeedMoreBuildRequests(); > > I don't think we should be calling this.clearMayNeedMoreBuildRequests here? Sure. >> Websites/perf.webkit.org/public/v3/models/test-group.js:200 >> + return 0; > > Looks like we're missing the case where this function will return NaN? > I don't think it's correct that we'd always clear may-need-more-build-requests flag? The retry logic will only retry the first commitSet that needs retry. And the syncing script ensures we don't schedule test group for next commitSet until current commitSet is done. These two condition will ensure the commitSet to retry is the only commitSet that need retry. So we can always clear the flag once retry is scheduled. >> Websites/perf.webkit.org/public/v3/models/test-group.js:337 >> + return this._updateBuildRequest({ > > Why the change (remove await)? 'await' after return in async function is redundant. But as you mentioned below, let's not do it in this massive patch. Will revert this change. >> Websites/perf.webkit.org/public/v3/models/test-group.js:357 >> + return this._updateBuildRequest({ > > I'm so confused about all these changes to remove await. > There is no harm in awaiting another async function. > Why don't we revert that since this patch is already so massive? Sure. >> Websites/perf.webkit.org/public/v3/models/test-group.js:405 >> + async resolveMayNeedMoreRequestsFlag(maxRetryFactor) > > I don't think this function name is descriptive. > I'd much rather call this function scheduleRetriesIfNecessary. I think previous review, you suggested `scheduleRetriesIfNecessary` does not reveal the fact that it clears `mayNeedMoreRequests` flag. >> Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.js:204 >> + const noCache = true; > > Nor that it's a big deal or it's an issue with your patch in particular > but we should be consistent with noCache vs ignoreCache... Sure, will use 'ignoreCache' for consistency. >> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1382 >> + it('should create a sequential test group with an analysis task', async () => { > > Is this a duplicate test case?? It has the same description as the preceding test case, and I can't spot a difference. Nice catch! I meant to test creating an alternating test group. >> Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:280 >> + it('should be able to cancel an alternating test group and clear the "may need more requests" flag', async () => { > > We should add another test case to make sure mayNeedMoreRequests doesn't get cleared as well. This is testing adding an API to allow canceling pending requests under a test group. The corresponding tests for clearing/not clearing "may need more requests" flag is under `test-groups-tests.js` >> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1399 >> + await MockRemoteAPI.waitForRequest(); > > What is this MockRemoteAPI.waitForRequest for?? Will remove this. >> Websites/perf.webkit.org/tools/sync-buildbot.js:15 >> + {name: '--max-retry-factor', type: parseFloat, default: 3}, > > Huh, it is concerning that run-analysis.js and run-analysis.js > in theory can get different values for --max-retry-factor. > Not in this patch but we should figure out a way to not duplicate this value in two different syncing scripts' configurations. > This is a timing time bomb. As we discussed, I plan to have a patch which will allow `fetchAllThatMayNeedMoreRequests` by triggerable, so that we can move the retry logic into syncing script. In this way, the retry-factor will be unified. >> Websites/perf.webkit.org/unit-tests/test-groups-tests.js:675 >> + it('should not schedule more build requests when build request is hidden', async () => { > > when the build requests are hidden? > But this is about the test group, right? I meant to say `should not schedule more build requests when *test group* is hidden`
Created attachment 429024 [details] Patch
(In reply to dewei_zhu from comment #27) > Comment on attachment 427601 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427601&action=review > > >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:96 > >> + $db->query('UPDATE build_requests SET request_order = request_order + $1 > > > > Did you check that this query is efficient? e.g. does it use the index for build_request_order_must_be_unique_in_group? > > Yes, I think I have a unit test for this. It will fail if array_reverse is > not used. No, I mean that does the SQL query itself update build_requests using the database index instead of doing a sequential search? Could you tell me what EXPLAIN ANALYZE will tell us with a sample query?
Comment on attachment 429024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429024&action=review > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:80 > + $build_request_by_commit_set = array(); We should probably call this $last_build_request_by_commit_set or $commit_set_to_last_build_request > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:81 > + foreach ($existing_build_requests as $last_existing_build_request) { I don't think it's appropriate to call this last_existing_build_request since we're iterating over every build request here. Why not just $current_build_request as you used to call it? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:87 > + if ($is_existing_commit_set) > + continue; > + $order_shift_by_commit_set[$commit_set_for_current_request] = (count($build_request_by_commit_set) - 1) * $additional_build_request_count; Huh, now that this whole thing is one line maybe it's simpler to write it as: $is_first_build_request = !array_key_exists($commit_set_for_current_request, $build_request_by_commit_set); ... if (!array_key_exists($commit_set_for_current_request, $build_request_by_commit_set)) $order_shift_by_commit_set[$commit_set_for_current_request] = count($build_request_by_commit_set) * $additional_build_request_count; $build_request_by_commit_set[$commit_set_for_current_request] = $last_existing_build_request; > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:91 > + foreach (array_reverse($order_shift_by_commit_set, true) as $commit_set => $increment) { Call this $shift for consistency? > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:99 > + $last_existing_build_request = $build_request_by_commit_set[$commit_set]; Whereas here, it's appropriate to call it last_existing_build_request since it *IS* the last build request for the commit set. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:101 > + $order_shift = array_get($order_shift_by_commit_set, $commit_set, 0); There should be no need to use array_get here. $order_shift_by_commit_set[$commit_set] should work. > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:103 > + duplicate_build_request_with_new_order($db, $last_existing_build_request, $last_existing_order + $i + $order_shift); I think it would be better to order them as: $last_existing_order + $order_shift + $i maybe we should store the sum of the first two terms as $starting_order: for ($i = 1, $starting_order = $last_existing_order + $order_shift; $i <= $additional_build_request_count; $i++) duplicate_build_request_with_new_order($db, $last_existing_build_request, $starting_order + $i); Alternatively, we could have computed last_existing_prder as follows: $last_existing_order = $last_existing_build_request['request_order'] + $order_shift_by_commit_set[$commit_set]; > Websites/perf.webkit.org/public/v3/models/test-group.js:113 > + return this.requestsForCommitSet(commitSet).filter((request) => request.isTest() && request.hasCompleted()).length Nit: missing a semicolon at the end. > Websites/perf.webkit.org/public/v3/models/test-group.js:124 > + const buildRequestIndex = orderedBuildRequests.indexOf(buildRequest); Let's make sure buildRequestIndex is greater than 0 by asserting it (i.e. buildRequest is in orderedBuildRequests)? > Websites/perf.webkit.org/public/v3/models/test-group.js:141 > + async _createAlternatingRetriesForTestGroup(maxRetryFactor) We should define this below scheduleRetriesIfNecessary so that it's easier to follow. > Websites/perf.webkit.org/public/v3/models/test-group.js:143 > + const retryLimit = maxRetryFactor * this.initialRepetitionCount(); Since this is the only place where maxRetryFactor is used and we compute this product in both _createAlternatingRetriesForTestGroup and _createSequentialRetriesForTestGroup, we should probably just compute that in the caller instead. > Websites/perf.webkit.org/public/v3/models/test-group.js:144 > + const additionalRepetitionNeeded = this.requestedCommitSets().reduce( Maybe we should call this retryCount since that's what we're returning. > Websites/perf.webkit.org/public/v3/models/test-group.js:152 > + return {retryCount: 0, mayNeedMoreRequests: false}; Hm... I've started to think maybe a better variable name might be retryCount & stopFutureRetries. It's rather counterintuitive that the caller does something special when mayNeedMoreRequests is false. > Websites/perf.webkit.org/public/v3/models/test-group.js:189 > + const additionalRepetition = this.additionalRepetitionNeededToReachInitialRepetitionCount(commitSet); > + > + if (additionalRepetition + this.repetitionCountForCommitSet(commitSet) > retryLimit) > + continue; Wait a minute. Doesn't this stop retry earlier than necessary? e.g. if this.repetitionCountForCommitSet(commitSet) is 4 and retryLimit is at 7, even if additionalRepetitionNeededToReachInitialRepetitionCount returns 4, we probably still want to add 3 more retries? > Websites/perf.webkit.org/public/v3/models/test-group.js:401 > + async resolveMayNeedMoreRequestsFlag(maxRetryFactor) > I think previous review, you suggested `scheduleRetriesIfNecessary` does not reveal the fact that it clears `mayNeedMoreRequests` flag. Yes, but I don't think that means we want to unnecessarily split the functions. Also, resolveMayNeedMoreRequestsFlag now doesn't convey the fact we're scheduling new retries. We should probably call this scheduleMoreRequestsOrClearFlag. "IfNeeded" is the term of art if we wanted to suffix but I don't think that's necessary. By saying that schedule X or clear flag, we're implicitly communicating that conditionality. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:400 > + const retrySummaryParts = testGroup.requestedCommitSets() Nice! So much cleaner! > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:403 > + .map((item) => `${item.retryCount} added to ${testGroup.labelForCommitSet(item.commitSet)} configuration`); Maybe we don't have to say "configuration" here. If we say "6 added to A due to failures" that should be sufficient, right? No need to make it wordy. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1468 > + it('should reject with "InvalidRepetitionType" if test mode is not "alternating" or "sequential"', async () => { Nit: repetition type > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1480 > + it('should reject with "InvalidRepetitionType" while creating test group from existing analysis task with repetition type is not "alternating" or "sequential"', async () => { Nit: should reject with "InvalidRepetitionType" when creating a test group with a bad repetition type for an existing analysis task > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:175 > + // Unlikely to hit this condition, just in case some build requests are created without test group fetched. I think I'd rephrase this as something like this: // The request might be for a very old test group we didn't fetch. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:93 > + await Promise.all(Array.from(buildRequestsByGroup.keys()).map(testGroupId => TestGroup.fetchById(testGroupId, true))); Nit: (testGroupId) => for consistency? Also, we should add /* ignoreCache */ commit here too. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:99 > + const shouldDefer = this._shouldDeferSequentialTestRequestWithNewCommitSet(request); This should probably be renamed to _shouldDeferSequentialTestRequestForNewCommitSet instead. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:161 > + if (testGroup.repetitionCountForCommitSet(previousCommitSet) > retryLimit) Surely, you meant to say testGroup.repetitionCountForCommitSet(~) >= retryLimit? > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:164 > + return testGroup.initialRepetitionCount() > testGroup.successfulTestCount(previousCommitSet); So this could condition would & what I had pointed out above would mean that this function can forever return true & sort of dead-lock the whole thing?
Created attachment 429216 [details] Patch
Comment on attachment 429216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429216&action=review > Websites/perf.webkit.org/ChangeLog:26 > + * public/include/commit-sets-helpers.php: Added code to create build requests based on test mode. Nit: test mode -> repetition type. > Websites/perf.webkit.org/public/v3/models/test-group.js:337 > + async resolveMayNeedMoreRequestsFlag(maxRetryFactor) Again, we should merge scheduleRetriesIfNecessary into this function and call this scheduleMoreRequestsOrClearFlag > Websites/perf.webkit.org/public/v3/models/test-group.js:374 > + const willExceedRetryLimit = this.requestedCommitSets().some(commitSet => > + additionalRepetitionNeeded + this.repetitionCountForCommitSet(commitSet) > retryLimit); > + > + if (!additionalRepetitionNeeded || willExceedRetryLimit) > + return {retryCount: 0, stopFutureRetries: true}; I don't think it's right that we stop adding retries just because adding `willExceedRetryLimit` will go over the limit. We should be adding the maximum retry count that won't exceed the retry limit instead. > Websites/perf.webkit.org/public/v3/models/test-group.js:388 > + await this.addMoreBuildRequests(additionalRepetitionNeeded); > + return {retryCount: additionalRepetitionNeeded, stopFutureRetries: true}; This isn't right. Build requests we added may fail in which case we may need to add even more retries. stopFutureRetries should be false for that reason. > Websites/perf.webkit.org/public/v3/models/test-group.js:410 > + if (additionalRepetition + this.repetitionCountForCommitSet(commitSet) > retryLimit) > + continue; Ditto about the limit here being wrong. > Websites/perf.webkit.org/public/v3/models/test-group.js:413 > + return {retryCount: additionalRepetition, stopFutureRetries: true}; Surely, we don't want to stop future retries for the entire test group!
Created attachment 429490 [details] Patch
Comment on attachment 429490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429490&action=review > Websites/perf.webkit.org/public/v3/models/test-group.js:348 > + const retryLimit = maxRetryFactor * this.initialRepetitionCount(); Now I notice that this isn't really retryLimit but more like repetitionLimit so we maybe we wanna do that rename. > Websites/perf.webkit.org/public/v3/models/test-group.js:365 > + const retryCount = this.requestedCommitSets().reduce( > + (currentMin, commitSet) => Math.min(Math.max(Math.floor(retryLimit - this.repetitionCountForCommitSet(commitSet)), 0), currentMin), additionalRepetitionNeeded); I guess this is more defensive code but shouldn't repetition count for all commit sets same in alternating group? If so, maybe we can just do this math with the first commit set and assert that the repetition counts for all commit sets are same separately. > Websites/perf.webkit.org/public/v3/models/test-group.js:374 > + return {retryCount: 0, stopFutureRetries: !hasUnfinishedBuildRequest}; Oh nice! This is a lot cleaner. > Websites/perf.webkit.org/public/v3/models/test-group.js:400 > + const retryCount = Math.min(Math.max(Math.floor(retryLimit - this.repetitionCountForCommitSet(commitSet)), 0), additionalRepetition); > + if (!retryCount) Maybe we can get rid of Math.max and just check retryCount <= 0 here? It's hard to read the nested min/max/floor right now. I guess the same comment applies to the math in _createAlternatingRetriesForTestGroup as well. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:10 > - constructor(config, remote, buildbotRemote, workerInfo, logger) > + constructor(config, remote, buildbotRemote, workerInfo, maxRetryFactor, logger) Why not maxRetryFactor make be a part of config instead of an argument to sync-buildbot.js? We probably want to store this retry factor thing in the server side in the future though. > Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:161 > + const allRequestsFailedForPreviousCommitSet = testGroup.requestsForCommitSet(previousCommitSet).every(request => !request.isTest() || request.hasFailed()); > + if (allRequestsFailedForPreviousCommitSet) Maybe allTestsFailedForPreviousCommitSet instead? Since we're all talking about requests, the fact we want to know tests might be more relevant than "request" part.
Landed in r278072.