Bug 46407 – Allow rebaselines for webkit-patch rebaseline to be chosen

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mihai Parparita]

Allow rebaselines for webkit-patch rebaseline to be chosen

[Mihai Parparita]

Created attachment 68594 [details]
Patch

[Mihai Parparita]

Adam or Eric, can you review?

[WebKit Review Bot]

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.

[Adam Barth]

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?

[Adam Barth]

Alternatively, the string "all" could mean choose all.

[Mihai Parparita]

Created attachment 68779 [details]
Patch

[Mihai Parparita]

(In reply to comment #5)
> Alternatively, the string "all" could mean choose all.

I like that. Also added basic validation (and a test).

[Adam Barth]

Comment on attachment 68779 [details]
Patch

Great.  Thanks!

[Eric Seidel (no email)]

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.

[Mihai Parparita]

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

[Mihai Parparita]

Created attachment 68783 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 68783 [details]
Patch

Clearing flags on attachment: 68783

Committed r68317: <http://trac.webkit.org/changeset/68317>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.