[webkit-reviews] review denied: [Bug 223886] [perf dashboard] Add sequential mode for perf dashboard A/B testing. : [Attachment 426409] Patch

Tue Apr 20 02:15:59 PDT 2021

Ryosuke Niwa <rniwa at webkit.org> has denied 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 426409: Patch

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

--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 426409
  --> https://bugs.webkit.org/attachment.cgi?id=426409
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_Object
s/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.j
s: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.j
s:157
> +	   for (const commitSet of updatedGroups[0].requestedCommitSets())
> +	      
assert.strictEqual(parseInt(updatedGroups[0].repetitionCountForCommitSet(commit
Set)), 4);

Ditto.

>
Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.j
s: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.j
s:192
> +	   let result = await PrivilegedAPI.sendRequest('create-test-group',

const?

>
Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.j
s: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.j
s: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.j
s: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.j
s:207
> +	   const commitSet = testGroups[0].requestedCommitSets()[1].id();

Add assertions for order.

>
Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.j
s:211
> +	   const updatedGroups = await TestGroup.fetchForTask(result['taskId'],
true);

Ditto about true.

>
Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.j
s: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.j
s:221
> +	   assert.strictEqual(buildRequestsForSecondCommitSet.length, 4);

Ditto about needing tests for order.

>
Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.j
s: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.j
s: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.j
s: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.j
s: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.j
s: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.j
s: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.j
s:264
> +	   let result = await PrivilegedAPI.sendRequest('create-test-group',

const?

>
Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.j
s: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.j
s:269
> +	   const testGroups = await TestGroup.fetchForTask(result['taskId'],
true);

Ditto about noCache.

>
Websites/perf.webkit.org/server-tests/privileged-api-add-build-requests-tests.j
s: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.j
s: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.j
s: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 :)