[Webkit-unassigned] [Bug 190188] Add retry for test groups with failed build requests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=190188

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #351579|review?                     |review+
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181004/addc83b0/attachment.html>