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