Bug 190863 – Customizable test group form should not reset manually edited commit value sometimes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Customizable test group form should not reset manually edited commit value sometimes.

Created attachment 353022 [details]
Patch

rdar://problem/45510389

Comment on attachment 353022 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:255
> +                if (ownerRepository)

The correct logic should be avoid updating commit for repositories with owner.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:270
> +                    const originalCommit = originalCommitSetMap[labelToChoose].commitForRepository(repository) || commit;

If this repository exists in originalCommitSetMap, we the radio button should set the value from original commit.

[Ryosuke Niwa]

Comment on attachment 353022 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:7
> +        After changing the radio button and manually editing the commit value, commit value should not be reset
> +        while changing the name of the test group.

Why don't we add a test for this?
And let's split this patch into two since these two changes are pretty complex.

> Websites/perf.webkit.org/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description bug after the bug link.

> Websites/perf.webkit.org/ChangeLog:22
> +        (CustomizableTestGroupForm.prototype._constructRevisionRadioButtons):

Let's add a comment saying that this code change is the one which fixes the toggling A/B would not update the commit set.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:20
> +        this._originalCommitSetMap = map;

How about uncustomizedCommitSetMap to match the terminology?

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:258
> +                commitSetMap.get(columnLabel).updateRevisionForOwnerRepository(repository, revisionEditor.value).catch(
>                      () => revisionEditor.value = '');

This needs to be updated to revert to the old value.
We should probably warn the user that the specified revision doesn't exist with a nice error message.

Created attachment 354038 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 354038 [details]
Patch

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

> Websites/perf.webkit.org/browser-tests/customizable-test-group-form-tests.js:78
> +        let revisionEditors = customizableTestGroupForm.content('custom-table').querySelectorAll('input:not([type="radio"])');
> +        expect(revisionEditors.length).to.be(2);
> +        let revisionEditor = revisionEditors[0];

Use const?

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:259
> +                        alert(`Commit "${revisionEditor.value}" from "${repository.name()}" does not exits.`);

I think a better error message would be `"${revisionEditor.value}" doesn't exist in "${repository.name()}"`.
Saying that it's a commit is both redundant for regular repositories and erroneous for OS versions.

Comment on attachment 354038 [details]
Patch

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

>> Websites/perf.webkit.org/browser-tests/customizable-test-group-form-tests.js:78
>> +        let revisionEditor = revisionEditors[0];
> 
> Use const?

We need to querySelect another revision editor after form gets re-rendered. Line 89 re-assigned it.

>> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:259
>> +                        alert(`Commit "${revisionEditor.value}" from "${repository.name()}" does not exits.`);
> 
> I think a better error message would be `"${revisionEditor.value}" doesn't exist in "${repository.name()}"`.
> Saying that it's a commit is both redundant for regular repositories and erroneous for OS versions.

Sure.

Landed in r237915.

rdar://problem/45510389