DumpRenderTree should report unknown options

• Component: Tools / Tests
• Status: REOPENED ---

[Frédéric Wang (:fredw)]

Currently TestOptions::TestOptions just ignores them. One has to implement parsing for new options, contrary to WebKitTestRunner.

[Frédéric Wang (:fredw)]

Original webkit-dev discussion:
https://lists.webkit.org/pipermail/webkit-dev/2018-November/030239.html
https://lists.webkit.org/pipermail/webkit-dev/2018-November/030247.html

Probably there are some options that are already used in tests but not handled by TestOptions yet.

[darshan]

Created attachment 358263 [details]
Added fallback else for unknown option

Added else for unknown option logged the option and break the  loop

[EWS Watchlist]

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


ERROR: Tools/DumpRenderTree/TestOptions.cpp:111:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 1 files


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

[Frédéric Wang (:fredw)]

Comment on attachment 358263 [details]
Added fallback else for unknown option

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

Thanks for the patch. Can you please rebase it (it seems it does not apply on tip of tree) and add the associated ChangeLog?

Also, have you actually tested your patch with a currently unknown option? If you find-grep tests, I'm sure some of them already use such unhandled options and you might need to add them into TestOptions.cpp and update the expectations. Also, I would be curious to see if the LOG_ERROR actually shows up in the test output.

> Tools/DumpRenderTree/TestOptions.cpp:113
> +            LOG_ERROR("Unknown option %s",key.c_str());

Missing space after comma (please run check-webkit-style before uploading the patch)

> Tools/DumpRenderTree/TestOptions.cpp:114
> +            break;

I think this break is not needed. We can just ignore the option (as we do now) and continue to the next pair.

[darshan]

Created attachment 358456 [details]
Patch

[darshan]

Following are the options which are not handled in TestOptions.cpp
applicationManifest
experimental:CSSCustomPropertiesAndValuesEnabled
useFlexibleViewport
useThreadedScrolling
experimental:DarkModeCSSEnabled
experimental:CSSTypedOMEnabled
punchOutWhiteBackgrounds
enableEditableImages
useCharacterSelectionGranularity
spellCheckingDots
experimental:CSSPaintingAPIEnabled
experimental:PointerEventsEnabled
useMockScrollbars
language
ignoresViewportScaleLimits
shouldIgnoreMetaViewport
enableProcessSwapOnNavigation
runSingly
internal:WebAPIStatisticsEnabled
internal:SourceBufferChangeTypeEnabled

[Frédéric Wang (:fredw)]

Comment on attachment 358456 [details]
Patch

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

> Following are the options which are not handled in TestOptions.cpp

Can you please give details about:
(1) How did you get that list? And what are the associated tests (or the amount of such tests)
(2) The changes you experimented in TestOptions.cpp and how they affect the webkit-test-runner results for the tests in (1). None of the patches you uploaded seem to have any effect on the EWS bots.

> Tools/ChangeLog:10
> +

ChangeLog still does not contain any useful information.

> Tools/DumpRenderTree/TestOptions.cpp:111
> +            fprintf(stderr, "Unknown option %s\n", key.c_str());

I think ideally we should re-use some logging feature provided in WTF. This does not seem better than LOG_ERROR (which e.g. provides file and line number).

[darshan]

Created attachment 360474 [details]
Patch

[darshan]

> Can you please give details about:
> (1) How did you get that list? And what are the associated tests (or the
> amount of such tests)

So I did it manually searched for" webkit-test-runner [" in all test files,
noted down all the options and checked whether they are in if else condition in Testopstions.cpp

[darshan]

Created attachment 360733 [details]
Patch

[EWS Watchlist]

Comment on attachment 360733 [details]
Patch

Attachment 360733 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10974231

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360737 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360733 [details]
Patch

Attachment 360733 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10973955

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360738 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Frédéric Wang (:fredw)]

Comment on attachment 360733 [details]
Patch

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

So now some tests are crashing, which is good news: Your patch really has an effect. Let's add support for the condition you mentioned in comment 6 now.

> Tools/DumpRenderTree/TestOptions.cpp:115
> +            RELEASE_ASSERT(false);

It does not seem nice to use a hardcoded boolean here. Can you please use RELEASE_ASSERT_NOT_REACHED instead?

[Frédéric Wang (:fredw)]

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

So now some tests are crashing, which is good news: Your patch really has an effect. Let's add support for the condition you mentioned in comment 6 now.

> Tools/DumpRenderTree/TestOptions.cpp:115
> +            RELEASE_ASSERT(false);

It does not seem nice to use a hardcoded boolean here. Can you please use RELEASE_ASSERT_NOT_REACHED instead?

[darshan]

(In reply to Frédéric Wang (:fredw) from comment #16)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360733&action=review
> 
> So now some tests are crashing, which is good news: Your patch really has an
> effect. Let's add support for the condition you mentioned in comment 6 now.
> 
> > Tools/DumpRenderTree/TestOptions.cpp:115
> > +            RELEASE_ASSERT(false);
> 
> It does not seem nice to use a hardcoded boolean here. Can you please use
> RELEASE_ASSERT_NOT_REACHED instead?

Okay I will add

[darshan]

> So now some tests are crashing, which is good news: Your patch really has an
> effect. Let's add support for the condition you mentioned in comment 6 now.

Do I need to implement all the conditions? What about all the different action that need to take on each condition?

[Frédéric Wang (:fredw)]

(In reply to darshan from comment #18)
> > So now some tests are crashing, which is good news: Your patch really has an
> > effect. Let's add support for the condition you mentioned in comment 6 now.
> 
> Do I need to implement all the conditions? What about all the different
> action that need to take on each condition?

I think most of them are boolean values so it should be easy to implement the parsing and properly set the corresponding preferences. Alternatively, maybe you can handle them in a else with a FIXME comment and handle that in a separate bug.

[darshan]

(In reply to Frédéric Wang (:fredw) from comment #19)
> (In reply to darshan from comment #18)
> > > So now some tests are crashing, which is good news: Your patch really has an
> > > effect. Let's add support for the condition you mentioned in comment 6 now.
> > 
> > Do I need to implement all the conditions? What about all the different
> > action that need to take on each condition?
> 
> I think most of them are boolean values so it should be easy to implement
> the parsing and properly set the corresponding preferences. Alternatively,
> maybe you can handle them in a else with a FIXME comment and handle that in
> a separate bug.

Yeah its seems okay! Thanks

[EWS Watchlist]

Comment on attachment 360733 [details]
Patch

Attachment 360733 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10974618

New failing tests:
fast/multicol/pagination/RightToLeft-rl-hittest.html
js/dom/regress-157246.html
fast/scrolling/rtl-scrollbars-animation-property.html
css-custom-properties-api/registerProperty.html
fast/text/combining-character-sequence-vertical.html
fast/text/descent-clip-in-scaled-page.html
fast/block/positioning/vertical-rl/fixed-positioning.html
fast/dom/horizontal-scrollbar-in-rtl.html
fast/text/vertical-quotation-marks.html
fast/dom/horizontal-scrollbar-when-dir-change.html
fast/dom/vertical-scrollbar-when-dir-change.html
fast/block/positioning/rtl-fixed-positioning.html
fast/dom/scroll-reveal-top-overflow.html
fast/dom/scroll-reveal-left-overflow.html

[EWS Watchlist]

Created attachment 360743 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[darshan]

Created attachment 360746 [details]
Patch

[EWS Watchlist]

Comment on attachment 360746 [details]
Patch

Attachment 360746 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10976681

New failing tests:
js/dom/regress-157246.html

[EWS Watchlist]

Created attachment 360760 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 360746 [details]
Patch

Attachment 360746 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10977474

New failing tests:
fast/canvas/webgl/hide-some-renderer-info.html
media/modern-media-controls/media-documents/insert-style-should-not-crash.html

[EWS Watchlist]

Created attachment 360762 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360746 [details]
Patch

Attachment 360746 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10977364

New failing tests:
fast/canvas/webgl/hide-some-renderer-info.html
media/modern-media-controls/media-documents/insert-style-should-not-crash.html

[EWS Watchlist]

Created attachment 360768 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360746 [details]
Patch

Attachment 360746 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10977843

New failing tests:
applicationmanifest/display-mode-subframe.html

[EWS Watchlist]

Created attachment 360769 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

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

[EWS Watchlist]

Comment on attachment 360746 [details]
Patch

Attachment 360746 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10977745

New failing tests:
applicationmanifest/display-mode-subframe.html

[EWS Watchlist]

Created attachment 360779 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[darshan]

Created attachment 360865 [details]
Patch

[darshan]

Created attachment 360913 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 360913 [details]
Patch

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

> Tools/ChangeLog:10
> +        Added and assigned testoptions which are currently used in tests.

test options

> Tools/DumpRenderTree/TestOptions.cpp:117
> +            experimentalCSSCustomPropertiesAndValuesEnabled = parseBooleanTestHeaderValue(value);

Looking at the rest of the code, it seems keys starting with "experimental:" and "internal:" don't include the prefix in the variable name. Also it seems that sometimes the variable name is of the form enableXXX rather than XXXEnabled (although that's not very consistent).

> Tools/DumpRenderTree/TestOptions.h:49
> +    bool experimentalCSSCustomPropertiesAndValuesEnabled { false };

Ideally, you should check that the default value is consistent with the rest of the code (especially what is used by the WebKiTestRunner). Have you tried to do a search to see how the values are initialized elsewhere?

Currently the new values are not used at all, so you should add a FIXME and open a separate bug if you don't plan to fix that in this bug.

For Web Preferences, you can probably get the default values from Source/WebKit/Shared/WebPreferences.yaml and only need to initialize them in setWebPreferencesForTestOptions from Tools/DumpRenderTree/mac/DumpRenderTree.mm

[darshan]

Created attachment 361201 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 361201 [details]
Patch

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

> Tools/DumpRenderTree/TestOptions.h:49
> +    bool enableCSSCustomPropertiesAndValues { false };

Please address add a FIXME:

(In reply to Frédéric Wang (:fredw) from comment #36)
> Currently the new values are not used at all, so you should add a FIXME and
> open a separate bug if you don't plan to fix that in this bug.
> 
> For Web Preferences, you can probably get the default values from
> Source/WebKit/Shared/WebPreferences.yaml and only need to initialize them in
> setWebPreferencesForTestOptions from
> Tools/DumpRenderTree/mac/DumpRenderTree.mm

[darshan]

Created attachment 361302 [details]
Patch

[darshan]

(In reply to darshan from comment #39)
> Created attachment 361302 [details]
> Patch

I tried to add default values of variables by referring  Source/WebKit/Shared/WebPreferences.yaml 
and 
Tools/DumpRenderTree/mac/DumpRenderTree.mm

[Frédéric Wang (:fredw)]

Comment on attachment 361302 [details]
Patch

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

> Tools/DumpRenderTree/TestOptions.h:50
> +    // Fixme: support following test options is not implemented

nit: it should be uppercase FIXME. And ideally refer to a but number https://webkit.org/b/????

[darshan]

Created attachment 361683 [details]
Patch

[darshan]

(In reply to darshan from comment #42)
> Created attachment 361683 [details]
> Patch

Created https://bugs.webkit.org/show_bug.cgi?id=194396

[Frédéric Wang (:fredw)]

Comment on attachment 361683 [details]
Patch

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

> Tools/DumpRenderTree/TestOptions.h:50
> +    // FIXME: support following test options is not implemented,

I guess you mean something like "FIXME: Implement the following test options".

[darshan]

Created attachment 361687 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361687 [details]
Patch

Clearing flags on attachment: 361687

Committed r241269: <https://trac.webkit.org/changeset/241269>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47969574>

[Truitt Savell]

It appears that the changes in https://trac.webkit.org/changeset/241269/webkit

made 5 tests on Mac WK1 crash:
fast/text/international/system-language/arabic-glyph-cache-fill-combine.html
fast/text/international/system-language/declarative-language.html 
fast/text/international/system-language/han-quotes.html 
fast/text/international/system-language/hindi-system-font-punctuation.html
fast/text/international/system-language/system-font-punctuation.html

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Ftext%2Finternational%2Fsystem-language%2Farabic-glyph-cache-fill-combine.html%20fast%2Ftext%2Finternational%2Fsystem-language%2Fdeclarative-language.html%20fast%2Ftext%2Finternational%2Fsystem-language%2Fhan-quotes.html%20fast%2Ftext%2Finternational%2Fsystem-language%2Fhindi-system-font-punctuation.html%20fast%2Ftext%2Finternational%2Fsystem-language%2Fsystem-font-punctuation.html

Test Results: 
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r241273%20(11135)/results.html

Confirmed this by running the 5 tests together on a checkout of 241269 which caused the tests to constantly crash, and no crashes occurred on the revision before. 

This will need to be rolled out.

[Truitt Savell]

Reverted r241269 for reason:

Caused 5 layout tests crashes on Mac WK1

Committed r241277: <https://trac.webkit.org/changeset/241277>

[Frédéric Wang (:fredw)]

(In reply to darshan from comment #9)
> > Can you please give details about:
> > (1) How did you get that list? And what are the associated tests (or the
> > amount of such tests)
> 
> So I did it manually searched for" webkit-test-runner [" in all test files,
> noted down all the options and checked whether they are in if else condition
> in Testopstions.cpp

What do you mean by "manually searched"? I really hope you've used a command line or any automated process to extract all these options. For example, in LayoutTests/fast/text we don't handle the "language" key and that's why the tests crash: 

find LayoutTests/fast/text -type f | xargs grep 'webkit-test-runner \['  | sed 's/.*webkit-test-runner \[ \(.\+\)\].*/\1/' | sort | uniq
language=ar 
language=hi 
language=ko,zh 
language=zh-Hans 
useThreadedScrolling=false

(probably there is a better command line option but you got the idea)

[darshan]

(In reply to Frédéric Wang (:fredw) from comment #51)
> (In reply to darshan from comment #9)
> > > Can you please give details about:
> > > (1) How did you get that list? And what are the associated tests (or the
> > > amount of such tests)
> > 
> > So I did it manually searched for" webkit-test-runner [" in all test files,
> > noted down all the options and checked whether they are in if else condition
> > in Testopstions.cpp
> 
> What do you mean by "manually searched"? I really hope you've used a command
> line or any automated process to extract all these options. For example, in
> LayoutTests/fast/text we don't handle the "language" key and that's why the
> tests crash: 
> 
>

I used VS code to search 'webkit-test-runner [' and listed down all the option,

I am confused why it did not fail on ews.
I will add the options and submit the patch

[darshan]

Created attachment 362238 [details]
Patch

[Frédéric Wang (:fredw)]

(In reply to darshan from comment #52)
> I used VS code to search 'webkit-test-runner [' and listed down all the
> option,
> I am confused why it did not fail on ews.

EWS compiles the patch but does not run the tests on all platform. Have you tried to search the 'webkit-test-runner' instances using a more reliable and less error-prone method than "VS code search" + manual copy / sorting?

[Darin Adler]

Comment on attachment 362238 [details]
Patch

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

> Tools/DumpRenderTree/TestOptions.cpp:161
> +            RELEASE_ASSERT_NOT_REACHED();

This is not "reporting" unknown options, it's *aborting* if passed an unknown option. That doesn’t seem like a good interface for a command line tool.

[Darin Adler]

Comment on attachment 362238 [details]
Patch

Why is it useful to parse and ignore so many options?

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #55)
> This is not "reporting" unknown options, it's *aborting* if passed an
> unknown option. That doesn’t seem like a good interface for a command line
> tool.

I agree. I'm not sure exactly what Darshan tried, but he was not able to log the text in a way that test authors are made aware of the missing parsing.

(In reply to Darin Adler from comment #56)
> Comment on attachment 362238 [details]
> Patch
> 
> Why is it useful to parse and ignore so many options?

I believe they should not be ignored. Darshan proposed to do it in a separate patch as that would lead to more changes (in test expectations?)...

But actually it's possible that we just want to ignore some WK2-specific options in DumpRenderTree? If so we don't need to parse them.

[darshan]

(In reply to Frédéric Wang (:fredw) from comment #54)
> (In reply to darshan from comment #52)
> > I used VS code to search 'webkit-test-runner [' and listed down all the
> > option,
> > I am confused why it did not fail on ews.
> 
> EWS compiles the patch but does not run the tests on all platform. Have you
> tried to search the 'webkit-test-runner' instances using a more reliable and
> less error-prone method than "VS code search" + manual copy / sorting?

Well I did not run any other command to do it and I agree this wasn't the best way to do this.

[darshan]

(In reply to Darin Adler from comment #55)
> Comment on attachment 362238 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362238&action=review
> 
> > Tools/DumpRenderTree/TestOptions.cpp:161
> > +            RELEASE_ASSERT_NOT_REACHED();
> 
> This is not "reporting" unknown options, it's *aborting* if passed an
> unknown option. That doesn’t seem like a good interface for a command line
> tool.

Idea was to fail the tests with unknown options. Getting the proper message why its failing would be the ideal solution for this!

[Fujii Hironori]

(In reply to darshan from comment #58)
> Well I did not run any other command to do it and I agree this wasn't the
> best way to do this.

FWIW, you can compare with the corresponding code of WebKitTestRunner.
https://trac.webkit.org/browser/webkit/trunk/Tools/WebKitTestRunner/TestController.cpp?rev=241755#L1221