Attempt to make new-run-webkit-tests --help more sane
Created attachment 53757 [details] Patch
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?
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?
Sorry. I mean in a separate file. This all needs to go into a separate file.
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?
I'm in no hurry. It could still get reviewed tonight and further fixerated tomorrow or just committed tomorrow.
(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.
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.
I like the concept. I'll let dirk comment before looking in detail.
(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
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.
(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 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 on attachment 53757 [details] Patch Please address dirk's comments before landing.
Attachment 53757 [details] was posted by a committer and has review+, assigning to Eric Seidel for commit.
(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.
Committed r59631: <http://trac.webkit.org/changeset/59631>
Updated per Dirk's comments and landed (finally). Hopefully this didn't conflict with too many in-flight patches.
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.
Broke Chromium, not sure why.
Oh, sad. I liked this patch soo much, but never got around to finishing it.