[webkit-reviews] review granted: [Bug 190188] Add retry for test groups with failed build requests. : [Attachment 351579] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 190188: Add retry for test groups with failed build requests.
https://bugs.webkit.org/show_bug.cgi?id=190188

Attachment 351579: Patch

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




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

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

> Websites/perf.webkit.org/public/api/build-requests.php:64
> +	       $test_group_id = $request_row['request_group'];

Do this outside of if-else.

> Websites/perf.webkit.org/public/api/build-requests.php:79
> +	   $test_groups_may_need_more_requests[$request_row['request_group']] =
TRUE;

Use $test_group_id once you've done that.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:10
> +    $additional_build_request_count = array_get($data, 'count');

Maybe we should call this addCount?

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:45
> +	   foreach($commit_sets as $commit_set) {

NIt: Need a space between foreach and (.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:51
> +	       insert_build_request_for_configuration($db, $configuration,
++$current_order, $build_request['request_triggerable'],
> +		   $build_request['request_platform'],
$build_request['request_test'], $build_request['request_group']);

Just use $db->insert_row directly here. I don't think we get any benefit from
using this helper function.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:54
> +    $db->update_row('analysis_test_groups', 'testgroup', array('id' =>
$test_group_id), array('may_need_more_requests' => FALSE));

Hm... on my second thought maybe it's better to clear this flag in a separate
API call
so that we can allow addition of more build requests without clearing of this
flag.

> Websites/perf.webkit.org/server-tests/api-test-groups.js:94
> +	       assert.ok(!data['testGroups'][0].mayNeedMoreRequests);

Maybe use assert.equals to be more explicit about true/false?

>
Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js
:208
> +    it('should be created with "may_need_more_requests" field defaults to
false', async () => {

This should read: "should create a test group with"

>
Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js
:212
> +	   let result = await PrivilegedAPI.sendRequest('create-test-group',

Use const?

>
Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js
:229
> +	   let result = await PrivilegedAPI.sendRequest('create-test-group',

Use const?

>
Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js
:252
> +    it('should clear "may_need_more_requests" while hiding test group',
async () => {

while -> when, hiding *a* test group.

>
Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js
:256
> +	   let result = await PrivilegedAPI.sendRequest('create-test-group',

Use const?

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:16
> +	       const buildRequests = testGroup.requestsForCommitSet(commitSet);

We should probably filter out non-testing build requests here.

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:19
> +	       hasCompletedBuildRequestForEachCommitSet &=
(completedBuildRequestCount > 0);

This math is rather confusing. Just do something like:
if (!completedBuildRequestCount)
    hasCompletedBuildRequestForEachCommitSet = false;

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:21
> +	       maxMissingBuildRequestCount =
Math.max(maxMissingBuildRequestCount, testGroup.initialRepetitionCount() -
completedBuildRequestCount - unfinishedBuildRequestCount);

Let's add an assertion that: unfinishedBuildRequestCount <=
testGroup.initialRepetitionCount()
and declare a local variable like: potentiallySuccessfulCount =
completedBuildRequestCount + unfinishedBuildRequestCount;

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:22
> +	       hasUnfinishedBuildRequest = hasUnfinishedBuildRequest ||
(unfinishedBuildRequestCount > 0);

Ditto about using if.

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:24
> +	   const willExceedMaximumRetry = (testGroup.repetitionCount() +
maxMissingBuildRequestCount) > maximumRetryFactor *
testGroup.initialRepetitionCount();

No need for parenthesis.

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:27
> +	   if (maxMissingBuildRequestCount && !willExceedMaximumRetry &&
hasUnfinishedBuildRequest && !hasCompletedBuildRequestForEachCommitSet)
> +	       continue;

I think a cleaner way to do this might be that:
if (!hasCompletedBuildRequestForEachCommitSet) {
    if (maxMissingBuildRequestCount && hasUnfinishedBuildRequest)
	await testGroup.clearMayNeedMoreBuildRequests();
    continue;
}

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:29
> +	   if (maxMissingBuildRequestCount && !willExceedMaximumRetry &&
hasCompletedBuildRequestForEachCommitSet)

Then we don't have to check hasCompletedBuildRequestForEachCommitSet here.

> Websites/perf.webkit.org/unit-tests/retry-failed-build-requests-tests.js:150
> +	   const testGroupConfig = {needsNotification:false,
initialRepetitionCount: 3, mayNeedMoreRequests: true, hidden: false,

Nit: Need a space between : and false, and ditto elsewhere in this file.