Bug 37836 – Attempt to make new-run-webkit-tests --help more sane

• Status: REOPENED
• Component: New Bugs

[Eric Seidel (no email)]

Attempt to make new-run-webkit-tests --help more sane

[Eric Seidel (no email)]

Created attachment 53757 [details]
Patch

[Tony Chang]

This looks great.  I am in support of this.  A couple style questions:

- Should we split all the options handling into a separate file?  It's getting kind of long.

- I normally inline really short functions.  So maybe define _split_option_name in _compare_option_names and define _compare_options in _add_option_group?

[Eric Seidel (no email)]

I completely agree this all needs to go into a separate function.  I decided to do that in a separate change, mostly because Dirk had outstanding changes to this code when I started this.

The only problem with inline functions is that I don't think  we can unit-test them, or?

Or is python crazy enough that you can do class.function.inline_function to call the inline function?

[Eric Seidel (no email)]

Sorry.  I mean in a separate file.  This all needs to go into a separate file.

[Dirk Pranke]

I want to review this but my brain is shot for the night ... can we hold off on committing this until at least tomorrow us/pacific time?

[Eric Seidel (no email)]

I'm in no hurry.  It could still get reviewed tonight and further fixerated tomorrow or just committed tomorrow.

[Tony Chang]

(In reply to comment #3)
> The only problem with inline functions is that I don't think  we can unit-test
> them, or?
> 
> Or is python crazy enough that you can do class.function.inline_function to
> call the inline function?

Ah, I see, you have a unittest for _split_option_name.  Yeah, that would have to stay outside to allow testing.

[Eric Seidel (no email)]

This really should all be on some new class in a new file.  But again, wanted to not break dirk's patches.  I'll move it all in a second patch.

[Adam Barth]

I like the concept.  I'll let dirk comment before looking in detail.

[Chris Jerdonek]

(In reply to comment #1)
> Created an attachment (id=53757) [details]

>          optparse.make_option("--startup-dialog", action="store_true",
> -            default=False, help="create a dialog on DumpRenderTree startup"),
> +            help="Create a dialog on test_shell startup."),

FWIW, I'd prefer if we can stick with the optparse convention of starting all help text with a lower-case letter:

http://docs.python.org/library/optparse.html

(Subsequent sentences can start with a capital letter if there is more than one sentence.)

optparse also does this in its default --help and --version help texts.

This is also the approach I took when refactoring check-webkit-style to use optparse:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/optparser.py?rev=57056#L295

[Eric Seidel (no email)]

I didn't realize that was optparse's convention.  I was trying to match old-run-webkit-tests and webkit-patch (and git for that matter).  SVN and python also start with lowercase though.

I'm not sure it matters horribly, but we should be consistent.  Also it appears that none of those use periods at the end of their sentences, so those could be removed.

[Dirk Pranke]

(In reply to comment #11)
> I didn't realize that was optparse's convention.  I was trying to match
> old-run-webkit-tests and webkit-patch (and git for that matter).  SVN and
> python also start with lowercase though.
> 
> I'm not sure it matters horribly, but we should be consistent.  Also it appears
> that none of those use periods at the end of their sentences, so those could be
> removed.

+1 to no capitals and no periods. +2 to consistency :)

[Dirk Pranke]

Comment on attachment 53757 [details]
Patch


> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> index 68fd865..93bab69 100755
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> +class HiddenOptionGroup(optparse.OptionGroup):
>  def _compat_shim_callback(option, opt_str, value, parser):
> +def _deprecated_shim_callback(option, opt_str, value, parser, new_option):
> +def _deprecated_option(parser, option_name, long_name=None, new_option_name=None):
> +def _split_option_name(option_name):
> +def _compare_option_names(a, b):
> +def _compare_options(a, b):
> +def _add_option_group(parser, title, options):
> +def _add_hidden_options(parser, options):

Please add comments and/or docstrings to all of this code. Ideally this would all be moved into a separate module (but you're already onto that). The intent and implementation of this is entirely non-obvious.

>      logging_options = [
> +        # FIXME: This option is super-confusing and should be hidden or removed.


My plan is to add a separate --help-logging message to get the details of this. Variable logging is important and shouldn't be hidden completely.

>          optparse.make_option("--sources", action="store_true",
> -            help="show expected result file path for each test " +
> -                 "(implies --verbose)"),

I think we can eliminate --sources

>      # FIXME: These options should move onto the ChromiumPort.

I don't understand how you expect to move options into different source files. Do expect this main routine to gather the arguments from each file somehow?

Also, in general, editorial cleanup is often better separated into a separate patch from functional cleanup and refactoring. That way it's easier to tell
what's important when reviewing the patch.

Note that --nocheck-sys-deps isn't Chromium-specific. The call is made on every port.

Otherwise, looks reasonable.

[Adam Barth]

Comment on attachment 53757 [details]
Patch

Please address dirk's comments before landing.

[Eric Seidel (no email)]

Attachment 53757 [details] was posted by a committer and has review+, assigning to Eric Seidel for commit.

[Eric Seidel (no email)]

(In reply to comment #13)

> Please add comments and/or docstrings to all of this code. Ideally this would all be moved into a separate module (but you're already onto that). The intent and implementation of this is entirely non-obvious.

Done.

> My plan is to add a separate --help-logging message to get the details of this. Variable logging is important and shouldn't be hidden completely.

Removed, since your plan has been done. :)

> I think we can eliminate --sources

You already did. :)

> >      # FIXME: These options should move onto the ChromiumPort.
> 
> I don't understand how you expect to move options into different source files. Do expect this main routine to gather the arguments from each file somehow?

Exactly.  webkit-patch does this from its various Command objects.

> Also, in general, editorial cleanup is often better separated into a separate patch from functional cleanup and refactoring. That way it's easier to tell
> what's important when reviewing the patch.
> 
> Note that --nocheck-sys-deps isn't Chromium-specific. The call is made on every port.

Yes, but Chromium is the only one who currently does anything with it.

[Eric Seidel (no email)]

Committed r59631: <http://trac.webkit.org/changeset/59631>

[Eric Seidel (no email)]

Updated per Dirk's comments and landed (finally).  Hopefully this didn't conflict with too many in-flight patches.

[Andrew Wilson]

I'm seeing failures on the chromium canaries:

http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/27810/steps/webkit_tests/logs/stdio

Not sure if it's this change, but it looks suspicious (happening somewhere between r59630 and 59634) so i'm looking at rolling it back to see if it fixes things.

[Eric Seidel (no email)]

Broke Chromium, not sure why.

[Eric Seidel (no email)]

Oh, sad.  I liked this patch soo much, but never got around to finishing it.