Bug 123185 – Refactor option parsing in WebKitTestRunner

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

[Tamas Gergely]

There is a FIXME in TestContoller.cpp on refactoring the options parsing of WebKitTestRunner in order to automatically generate help message.

[Tamas Gergely]

Created attachment 214902 [details]
Refactor options handling in WKRT.

[WebKit Commit Bot]

Attachment 214902 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/CMakeLists.txt', u'Tools/WebKitTestRunner/GNUmakefile.am', u'Tools/WebKitTestRunner/Options.cpp', u'Tools/WebKitTestRunner/Options.h', u'Tools/WebKitTestRunner/TestController.cpp', u'Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj']" exit_code: 1
Tools/WebKitTestRunner/Options.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.h:34:  Streams are highly discouraged.  [readability/streams] [3]
Tools/WebKitTestRunner/Options.h:50:  c_defaultLongTimeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.h:51:  c_defaultShortTimeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.h:66:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/Options.h:68:  Missing spaces around =  [whitespace/operators] [4]
Tools/WebKitTestRunner/Options.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.cpp:29:  Streams are highly discouraged.  [readability/streams] [3]
Tools/WebKitTestRunner/Options.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.cpp:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:46:  handle_option_timeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:53:  handle_option_no_timeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:58:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:58:  handle_option_no_timeout_at_all is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:64:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:64:  handle_option_verbose is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:69:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:69:  handle_option_gc_between_tests is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:74:  handle_option_pixel_tests is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:79:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:79:  handle_option_print_supported_features is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:84:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:84:  handle_option_unmatched is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:87:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:92:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:108:  Missing space inside { }.  [whitespace/braces] [5]
Tools/WebKitTestRunner/Options.cpp:110:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:116:  Missing spaces around =  [whitespace/operators] [4]
Tools/WebKitTestRunner/Options.cpp:116:  Missing spaces around <  [whitespace/operators] [3]
Tools/WebKitTestRunner/Options.cpp:116:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:116:  arg_counter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:117:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:118:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:121:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:122:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:124:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:125:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:127:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:129:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:131:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:133:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:135:  Use 0 instead of NULL.  [readability/null] [5]
Tools/WebKitTestRunner/Options.cpp:136:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:146:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:147:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:148:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:149:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:151:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/TestController.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 56 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 214902 [details]
Refactor options handling in WKRT.

Attachment 214902 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/9358035

New failing tests:
accessibility/anchor-linked-anonymous-block-crash.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/added-while-suspended.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
http/tests/cache/display-image-unset-allows-cached-image-load.html
animations/3d/change-transform-in-end-event.html
http/tests/cache/content-type-ignored-during-revalidation.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/cancel-during-revalidation-succeeded.html
canvas/philip/tests/2d.canvas.readonly.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
http/tests/cache/cached-main-resource.html
accessibility/adjacent-continuations-cause-assertion-failure.html
compositing/absolute-position-changed-with-composited-parent-layer.html

[Build Bot]

Created attachment 214909 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Csaba Osztrogonác]

Comment on attachment 214902 [details]
Refactor options handling in WKRT.

The idea is good to have a better option parser.
But first please fix the style issues and crashes.

[Tamas Gergely]

Created attachment 215489 [details]
Refactor options parsing.

This will probably fix Mac issues.

[Tamas Gergely]

Created attachment 215645 [details]
Refactor options parsing in WTR

[Peter Gal]

Comment on attachment 215645 [details]
Refactor options parsing in WTR

View in context: https://bugs.webkit.org/attachment.cgi?id=215645&action=review

> Tools/WebKitTestRunner/Options.cpp:34
> +Options::Options(double dlto, double dsto):

What does dlto/dsto mean? If possible we should not use abbreviations.

> Tools/WebKitTestRunner/Options.cpp:48
> +bool handleOptionTimeout(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:56
> +bool handleOptionNoTimeout(Options& o, const char* opt, const char* arg)

ditto

> Tools/WebKitTestRunner/Options.cpp:62
> +bool handleOptionNoTimeoutAtAll(Options& o, const char* opt, const char* arg)

ditto

> Tools/WebKitTestRunner/Options.cpp:69
> +bool handleOptionVerbose(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:75
> +bool handleOptionGcBetweenTests(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:81
> +bool handleOptionPixelTests(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:87
> +bool handleOptionPrintXSupportedFeatures(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:101
> +OptionsHandler::OptionsHandler(Options& o): options(o)

ditto.

> Tools/WebKitTestRunner/Options.cpp:103
> +    add(Option("--timeout",                  "Sets long timeout to <param> and scales short timeout.", handleOptionTimeout, true));

There are too many whitespace before the option description, we should keep only one.

> Tools/WebKitTestRunner/Options.cpp:128
> +    for (int argCounter = 1; argCounter < argc; argCounter++) {

++argCounter instead of argCounter++.

> Tools/WebKitTestRunner/Options.cpp:133
> +        for (Option &o : optionlist) {

don't use abbreviations.
'Option&' instead of 'Option &'

> Tools/WebKitTestRunner/Options.cpp:153
> +void OptionsHandler::help(FILE *channel)

'FILE*' instead of the 'FILE *'

> Tools/WebKitTestRunner/Options.cpp:170
> +void OptionsHandler::add(Option o)

Do we need this method at all? We only use it in the handler constructor. On the side note: Maybe we can use Option& here to avoid copy.

> Tools/WebKitTestRunner/Options.h:59
> +    const char* desc;

desc-> description.

> Tools/WebKitTestRunner/Options.h:74
> +    static const char * usagestr;
> +    static const char * helpstr;

maybe we can remove the str part from the variable name, and 'char*' instead of 'char *'.

> Tools/WebKitTestRunner/TestController.cpp:298
> +    m_longTimeout                 = wtrOptions.m_longTimeout;
> +    m_shortTimeout                = wtrOptions.m_shortTimeout;
> +    m_useWaitToDumpWatchdogTimer  = wtrOptions.m_useWaitToDumpWatchdogTimer;
> +    m_forceNoTimeout              = wtrOptions.m_forceNoTimeout;
> +    m_verbose                     = wtrOptions.m_verbose;
> +    m_gcBetweenTests              = wtrOptions.m_gcBetweenTests;
> +    m_shouldDumpPixelsForAllTests = wtrOptions.m_shouldDumpPixelsForAllTests;
> +    m_paths                       = wtrOptions.m_paths;

keep only one space before the = signs.

[Tamas Gergely]

Created attachment 217667 [details]
Options refactoring

Modified according to the comments.

[EFL EWS Bot]

Comment on attachment 217667 [details]
Options refactoring

Attachment 217667 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/33868042

[Build Bot]

Comment on attachment 217667 [details]
Options refactoring

Attachment 217667 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/33748041

[Build Bot]

Comment on attachment 217667 [details]
Options refactoring

Attachment 217667 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/33848056

[Tamas Gergely]

Created attachment 217672 [details]
Options refactoring (corrected)

Sorry, previous patch was not the final one. But I'm surprised that it passed two checks.

[Tamas Gergely]

Created attachment 219167 [details]
Updated patch.

The 3 new options are included.

[Darin Adler]

Comment on attachment 219167 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=219167&action=review

Looks OK, lost of style mistakes.

> Tools/WebKitTestRunner/Options.cpp:34
> +Options::Options(double _default_long_timeout, double _default_short_timeout)

Should not have identifiers with underscores in them, especially not leading underscores. defaultLongTimeout and defaultShortTimeout.

> Tools/WebKitTestRunner/Options.cpp:158
> +        const char * currentOption = argv[argCounter];

Not our style to have a space before the *.

> Tools/WebKitTestRunner/Options.cpp:159
> +        for (Option &option : optionlist) {

Not our style to have a space before the & and no space after the &.

> Tools/WebKitTestRunner/Options.cpp:163
> +                if (!option.paramhandler) {
> +                    status = false;
> +                } else if (option.hasargument) {

Not our style to have braces around one line if statement body.

> Tools/WebKitTestRunner/Options.cpp:184
> +    for (Option &option : optionlist) {

Same problem with option formatting.

> Tools/WebKitTestRunner/Options.h:52
> +    double m_longTimeout;
> +    double m_shortTimeout;
> +    bool m_useWaitToDumpWatchdogTimer;
> +    bool m_forceNoTimeout;
> +    bool m_verbose;
> +    bool m_gcBetweenTests;
> +    bool m_shouldDumpPixelsForAllTests;
> +    bool m_printSupportedFeatures;
> +    bool m_forceComplexText;
> +    bool m_shouldUseAcceleratedDrawing;
> +    bool m_shouldUseRemoteLayerTree;
> +    std::vector<std::string> m_paths;

We normally use the "m_" for private data members, not public ones. I also suggest making this a struct.

> Tools/WebKitTestRunner/Options.h:54
> +    double constDefaultLongTimeout;
> +    double constDefaultShortTimeout;

What does “const” mean here? These are not const. What  is this?

> Tools/WebKitTestRunner/Options.h:63
> +    std::function<bool(Options&, const char*, const char*)> paramhandler;

Should be parameterHandler rather than paramhandler.

> Tools/WebKitTestRunner/Options.h:64
> +    bool hasargument;

Should be hasArgument with capital “A”.

> Tools/WebKitTestRunner/Options.h:69
> +    OptionsHandler(Options&);

Should be marked explicit.

> Tools/WebKitTestRunner/Options.h:70
> +    bool parse(int, const char*[]);

Should have argument names here.

> Tools/WebKitTestRunner/Options.h:71
> +    void printhelp(FILE* channel = stderr);

Should be printHelp not printhelp.

> Tools/WebKitTestRunner/Options.h:73
> +    WTF::Vector<Option> optionlist;

No need for WTF:: here. Also, should be optionList rather than optionlist with no capital letter.

> Tools/WebKitTestRunner/TestController.cpp:270
> +    Options wtrOptions(defaultLongTimeout, defaultShortTimeout);

Strange to have “wtr” here. Can we just call this local variable “options”.

> Tools/WebKitTestRunner/TestController.cpp:271
> +    OptionsHandler handleCLargs(wtrOptions);

A handler should not have a verb as its name. Also strange that “args” is lower case. I would call this “commandLineOptionsHandler” or “optionsHandler” or “handler” rather than “handleCLargs”.

> Tools/WebKitTestRunner/TestController.cpp:731
> -        if (strlen(filenameBuffer) == 0)
> +        if (!strlen(filenameBuffer))
>              continue;

If changing this at all, I suggest changing it to:

    if (!filenameBuffer[0])

It’s silly to find the string length just to check if the first character is a null character.

[Tamas Gergely]

Created attachment 219277 [details]
Updated patch.

I renamed the identifiers, and left line Tools/WebKitTestRunner/TestController.cpp:731 unchanged (it does not belong to this change).

[Darin Adler]

Comment on attachment 219277 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=219277&action=review

All fine to land, although we could do better.

> Tools/WebKitTestRunner/Options.cpp:122
> +OptionsHandler::OptionsHandler(Options& o)

Should name the argument options rather than o.

> Tools/WebKitTestRunner/Options.cpp:136
> +    optionList.append(Option("--timeout", "Sets long timeout to <param> and scales short timeout.", handleOptionTimeout, true));
> +    optionList.append(Option("--no-timeout", "Disables timeout.", handleOptionNoTimeout));
> +    optionList.append(Option("--no-timeout-at-all", "Disables all timeouts.", handleOptionNoTimeoutAtAll));
> +    optionList.append(Option("--verbose", "Turns on messages.", handleOptionVerbose));
> +    optionList.append(Option("--gc-between-tests", "Garbage collection between tests.", handleOptionGcBetweenTests));
> +    optionList.append(Option("--pixel-tests", "Check pixels.", handleOptionPixelTests));
> +    optionList.append(Option("-p", "Check pixels.", handleOptionPixelTests));
> +    optionList.append(Option("--print-supported-features", "For DumpRenderTree compatibility.", handleOptionPrintSupportedFeatures));
> +    optionList.append(Option("--complex-text", "Force complex tests.", handleOptionComplexText));
> +    optionList.append(Option("--accelerated-drawing", "Use accelerated drawing.", handleOptionAcceleratedDrawing));
> +    optionList.append(Option("--remote-layer-tree", "Use remote layer tree.", handleOptionRemoteLayerTree));
> +    optionList.append(Option(0, 0, handleOptionUnmatched));

Not really sure why we are building up a vector at runtime for something that is a fixed list known at compile time.

> Tools/WebKitTestRunner/Options.cpp:140
> +const char * OptionsHandler::usage = "Usage: WebKitTestRunner [options] filename [filename2..n]";
> +const char * OptionsHandler::help = "Displays this help.";

Extra space here. Should just be "const char*", not "const char *".

> Tools/WebKitTestRunner/Options.cpp:143
> +Option::Option(const char* name, const char* description, std::function<bool(Options&, const char*, const char*)> parameterHandler, bool hasArgument)
> +    : name(name), description(description), parameterHandler(parameterHandler), hasArgument(hasArgument) { };

Formatting here is not right for our usual syntax. We put the function body at the far left, not on the end of the initializers line.

Also, an extraneous semicolon at the end.

> Tools/WebKitTestRunner/Options.cpp:164
> +                    const char * currentArgument = argv[++argCounter];

Extra space here. Should just be "const char*", not "const char *".

> Tools/WebKitTestRunner/Options.h:39
> +    Options(double, double);

Need argument names here, otherwise it’s not clear what the two arguments are.

> Tools/WebKitTestRunner/Options.h:53
> +    double defaultLongTimeout;
> +    double defaultShortTimeout;

These two could be private and const.

> Tools/WebKitTestRunner/Options.h:57
> +class Option {
> +public:

Since everything is public I would probably make this a struct.

> Tools/WebKitTestRunner/Options.h:58
> +    Option(const char* name, const char* description, std::function<bool(Options&, const char*, const char*)> parameterHandler, bool hasArgument = false);

Need names for the two const char* arguments to make clear what they are. That means we probably should use a typedef.

> Tools/WebKitTestRunner/Options.h:75
> +    static const char* usage;
> +    static const char* help;

Not sure why these are members. They could just be non-member globals. Also, they should be const:

    static const char* const usage;

[WebKit Commit Bot]

Comment on attachment 219277 [details]
Updated patch.

Clearing flags on attachment: 219277

Committed r160627: <http://trac.webkit.org/changeset/160627>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.