Allow rebaselines for webkit-patch rebaseline to be chosen
Created attachment 68594 [details] Patch
Adam or Eric, can you review?
Attachment 68594 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py:77: whitespace before ']' [pep8/E202] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 68594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68594&action=review Good idea. I'd prefer if we could avoid adding more command line arguments. They add complexity, especially for commands that aren't used that often. > WebKitTools/Scripts/webkitpy/common/system/user.py:77 > - result = int(cls.prompt("Enter a number: ")) - 1 > - return list_items[result] > + if can_choose_multiple: > + response = cls.prompt("Enter one or more numbers (comma-separated): ") > + indices = [int(r) - 1 for r in re.split("\s*,\s*", response)] > + return [list_items[i] for i in indices] > + else: > + result = int(cls.prompt("Enter a number: ")) - 1 > + return list_items[result] Do we want better error handling if the user doesn't enter an int? > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py:115 > + if options.choose_rebaselines: > + tests_to_update = self._tool.user.prompt_with_list("Which test(s) to rebaseline:", tests_to_update, can_choose_multiple=True) Rather than making it a command-line option, can we just have prompt_with_list+can_choose_multiple make it easy to choose all? Maybe the empty string is how you choose all?
Alternatively, the string "all" could mean choose all.
Created attachment 68779 [details] Patch
(In reply to comment #5) > Alternatively, the string "all" could mean choose all. I like that. Also added basic validation (and a test).
Comment on attachment 68779 [details] Patch Great. Thanks!
Comment on attachment 68779 [details] Patch Interesting. I had assumed that users would just git revert ones they didn't want to change. But this works too. I like the new features in prompt_with_list. I'm torn as to if raw_input should have been mocked through the use of an instance method on User instead of a parameter to the function. In general looks fine. Yay tests! What does enter do? Seems we should test the empty input string/default value too. I would expect enter to just do "all" in the case of rebaseline tool.
(In reply to comment #9) > (From update of attachment 68779 [details]) > Interesting. I had assumed that users would just git revert ones they didn't want to change. My usecase was that I was trying to be a good citizen and updatet the baselines for Windows for a test I had just changed. The Windows bot often has lots of other failing tests, so it was annoying to revert all baselines that got downloaded except for the one that I was interested in. > But this works too. I like the new features in prompt_with_list. I'm torn as to if raw_input should have been mocked through the use of an instance method on User instead of a parameter to the function. But prompt and prompt_with_list are both class methods (not sure if the FIXME above is still relevant). > What does enter do? Seems we should test the empty input string/default value too. I would expect enter to just do "all" in the case of rebaseline tool. Sure, I can do that.
Created attachment 68783 [details] Patch
Comment on attachment 68783 [details] Patch Clearing flags on attachment: 68783 Committed r68317: <http://trac.webkit.org/changeset/68317>
All reviewed patches have been landed. Closing bug.