Bug 37836 - Attempt to make new-run-webkit-tests --help more sane
Summary: Attempt to make new-run-webkit-tests --help more sane
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: NRWT
Depends on: 39255 76221
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-19 18:19 PDT by Eric Seidel (no email)
Modified: 2017-07-18 08:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (22.28 KB, patch)
2010-04-19 18:22 PDT, Eric Seidel (no email)
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-04-19 18:19:13 PDT
Attempt to make new-run-webkit-tests --help more sane
Comment 1 Eric Seidel (no email) 2010-04-19 18:22:11 PDT
Created attachment 53757 [details]
Patch
Comment 2 Tony Chang 2010-04-19 18:35:38 PDT
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?
Comment 3 Eric Seidel (no email) 2010-04-19 18:44:17 PDT
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?
Comment 4 Eric Seidel (no email) 2010-04-19 18:44:38 PDT
Sorry.  I mean in a separate file.  This all needs to go into a separate file.
Comment 5 Dirk Pranke 2010-04-19 18:46:44 PDT
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?
Comment 6 Eric Seidel (no email) 2010-04-19 18:49:09 PDT
I'm in no hurry.  It could still get reviewed tonight and further fixerated tomorrow or just committed tomorrow.
Comment 7 Tony Chang 2010-04-19 19:16:37 PDT
(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.
Comment 8 Eric Seidel (no email) 2010-04-19 19:38:19 PDT
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.
Comment 9 Adam Barth 2010-04-20 12:14:47 PDT
I like the concept.  I'll let dirk comment before looking in detail.
Comment 10 Chris Jerdonek 2010-04-20 12:33:12 PDT
(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
Comment 11 Eric Seidel (no email) 2010-04-20 12:42:50 PDT
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.
Comment 12 Dirk Pranke 2010-04-20 14:53:45 PDT
(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 :)
Comment 13 Dirk Pranke 2010-04-20 15:06:58 PDT
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.
Comment 14 Adam Barth 2010-04-22 12:56:57 PDT
Comment on attachment 53757 [details]
Patch

Please address dirk's comments before landing.
Comment 15 Eric Seidel (no email) 2010-05-02 19:31:26 PDT
Attachment 53757 [details] was posted by a committer and has review+, assigning to Eric Seidel for commit.
Comment 16 Eric Seidel (no email) 2010-05-17 17:02:58 PDT
(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.
Comment 17 Eric Seidel (no email) 2010-05-17 17:16:09 PDT
Committed r59631: <http://trac.webkit.org/changeset/59631>
Comment 18 Eric Seidel (no email) 2010-05-17 17:17:09 PDT
Updated per Dirk's comments and landed (finally).  Hopefully this didn't conflict with too many in-flight patches.
Comment 19 Andrew Wilson 2010-05-17 18:26:13 PDT
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.
Comment 20 Eric Seidel (no email) 2010-05-17 18:54:53 PDT
Broke Chromium, not sure why.
Comment 21 Eric Seidel (no email) 2011-05-17 10:50:35 PDT
Oh, sad.  I liked this patch soo much, but never got around to finishing it.