[webkit-reviews] review granted: [Bug 223886] [perf dashboard] Add sequential mode for perf dashboard A/B testing. : [Attachment 427601] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 11 03:26:52 PDT 2021
Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 223886: [perf dashboard] Add sequential mode for perf dashboard A/B
testing.
https://bugs.webkit.org/show_bug.cgi?id=223886
Attachment 427601: Patch
https://bugs.webkit.org/attachment.cgi?id=427601&action=review
--- Comment #26 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 427601
--> https://bugs.webkit.org/attachment.cgi?id=427601
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.j
s: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.j
s: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.j
s: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.j
s: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.j
s: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.j
s: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.j
s: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.j
s: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.j
s: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.
More information about the webkit-reviews
mailing list