Bug 191303 - DumpRenderTree should report unknown options
Summary: DumpRenderTree should report unknown options
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-06 03:04 PST by Frédéric Wang (:fredw)
Modified: 2019-02-18 22:42 PST (History)
11 users (show)

See Also:


Attachments
Added fallback else for unknown option (615 bytes, patch)
2019-01-03 11:19 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (1.86 KB, patch)
2019-01-05 22:59 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2019-01-29 10:42 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2019-01-31 09:09 PST, darshan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.57 MB, application/zip)
2019-01-31 10:07 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.18 MB, application/zip)
2019-01-31 10:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (12.90 MB, application/zip)
2019-01-31 10:55 PST, Build Bot
no flags Details
Patch (3.88 KB, patch)
2019-01-31 11:12 PST, darshan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.81 MB, application/zip)
2019-01-31 12:57 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-highsierra (2.56 MB, application/zip)
2019-01-31 13:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.46 MB, application/zip)
2019-01-31 13:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.55 MB, application/zip)
2019-01-31 13:52 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.40 MB, application/zip)
2019-01-31 14:14 PST, Build Bot
no flags Details
Patch (3.47 KB, patch)
2019-02-01 09:25 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (6.46 KB, patch)
2019-02-01 15:38 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (6.27 KB, patch)
2019-02-05 10:57 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (6.30 KB, patch)
2019-02-06 09:42 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2019-02-11 08:54 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (6.34 KB, patch)
2019-02-11 09:47 PST, darshan
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2019-02-17 10:11 PST, darshan
dkadu: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-11-06 03:04:09 PST
Currently TestOptions::TestOptions just ignores them. One has to implement parsing for new options, contrary to WebKitTestRunner.
Comment 1 Frédéric Wang (:fredw) 2018-11-07 02:54:26 PST
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.
Comment 2 darshan 2019-01-03 11:19:00 PST
Created attachment 358263 [details]
Added fallback else for unknown option

Added else for unknown option logged the option and break the  loop
Comment 3 Build Bot 2019-01-03 11:21:03 PST
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.
Comment 4 Frédéric Wang (:fredw) 2019-01-04 06:24:31 PST
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.
Comment 5 darshan 2019-01-05 22:59:17 PST
Created attachment 358456 [details]
Patch
Comment 6 darshan 2019-01-05 23:29:19 PST
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
Comment 7 Frédéric Wang (:fredw) 2019-01-07 02:02:55 PST
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).
Comment 8 darshan 2019-01-29 10:42:44 PST
Created attachment 360474 [details]
Patch
Comment 9 darshan 2019-01-29 10:46:13 PST
> 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
Comment 10 darshan 2019-01-31 09:09:39 PST
Created attachment 360733 [details]
Patch
Comment 11 Build Bot 2019-01-31 10:07:16 PST
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.
Comment 12 Build Bot 2019-01-31 10:07:18 PST
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
Comment 13 Build Bot 2019-01-31 10:08:06 PST
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.
Comment 14 Build Bot 2019-01-31 10:08:07 PST
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
Comment 15 Frédéric Wang (:fredw) 2019-01-31 10:20:06 PST
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?
Comment 16 Frédéric Wang (:fredw) 2019-01-31 10:20:13 PST
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?
Comment 17 darshan 2019-01-31 10:25:09 PST
(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
Comment 18 darshan 2019-01-31 10:29:31 PST
> 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?
Comment 19 Frédéric Wang (:fredw) 2019-01-31 10:33:34 PST
(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.
Comment 20 darshan 2019-01-31 10:35:22 PST
(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
Comment 21 Build Bot 2019-01-31 10:55:11 PST
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
Comment 22 Build Bot 2019-01-31 10:55:23 PST
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
Comment 23 darshan 2019-01-31 11:12:58 PST
Created attachment 360746 [details]
Patch
Comment 24 Build Bot 2019-01-31 12:57:45 PST
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
Comment 25 Build Bot 2019-01-31 12:57:57 PST
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
Comment 26 Build Bot 2019-01-31 13:21:56 PST
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
Comment 27 Build Bot 2019-01-31 13:21:57 PST
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
Comment 28 Build Bot 2019-01-31 13:48:42 PST
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
Comment 29 Build Bot 2019-01-31 13:48:44 PST
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
Comment 30 Build Bot 2019-01-31 13:52:15 PST
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
Comment 31 Build Bot 2019-01-31 13:52:17 PST
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
Comment 32 Build Bot 2019-01-31 14:14:34 PST
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
Comment 33 Build Bot 2019-01-31 14:14:36 PST
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
Comment 34 darshan 2019-02-01 09:25:04 PST
Created attachment 360865 [details]
Patch
Comment 35 darshan 2019-02-01 15:38:58 PST
Created attachment 360913 [details]
Patch
Comment 36 Frédéric Wang (:fredw) 2019-02-04 12:42:02 PST
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
Comment 37 darshan 2019-02-05 10:57:04 PST
Created attachment 361201 [details]
Patch
Comment 38 Frédéric Wang (:fredw) 2019-02-05 11:14:30 PST
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
Comment 39 darshan 2019-02-06 09:42:48 PST
Created attachment 361302 [details]
Patch
Comment 40 darshan 2019-02-06 09:44:23 PST
(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
Comment 41 Frédéric Wang (:fredw) 2019-02-07 00:59:58 PST
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/????
Comment 42 darshan 2019-02-11 08:54:11 PST
Created attachment 361683 [details]
Patch
Comment 43 darshan 2019-02-11 08:54:43 PST
(In reply to darshan from comment #42)
> Created attachment 361683 [details]
> Patch

Created https://bugs.webkit.org/show_bug.cgi?id=194396
Comment 44 Frédéric Wang (:fredw) 2019-02-11 09:32:12 PST
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".
Comment 45 darshan 2019-02-11 09:47:05 PST
Created attachment 361687 [details]
Patch
Comment 46 WebKit Commit Bot 2019-02-11 10:32:48 PST
Comment on attachment 361687 [details]
Patch

Clearing flags on attachment: 361687

Committed r241269: <https://trac.webkit.org/changeset/241269>
Comment 47 WebKit Commit Bot 2019-02-11 10:32:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 48 Radar WebKit Bug Importer 2019-02-11 10:33:52 PST
<rdar://problem/47969574>
Comment 49 Truitt Savell 2019-02-11 13:04:51 PST
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.
Comment 50 Truitt Savell 2019-02-11 13:48:01 PST
Reverted r241269 for reason:

Caused 5 layout tests crashes on Mac WK1

Committed r241277: <https://trac.webkit.org/changeset/241277>
Comment 51 Frédéric Wang (:fredw) 2019-02-12 01:20:36 PST
(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)
Comment 52 darshan 2019-02-16 09:32:39 PST
(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
Comment 53 darshan 2019-02-17 10:11:25 PST
Created attachment 362238 [details]
Patch
Comment 54 Frédéric Wang (:fredw) 2019-02-17 11:31:31 PST
(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?
Comment 55 Darin Adler 2019-02-18 07:57:25 PST
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.
Comment 56 Darin Adler 2019-02-18 07:57:49 PST
Comment on attachment 362238 [details]
Patch

Why is it useful to parse and ignore so many options?
Comment 57 Frédéric Wang (:fredw) 2019-02-18 08:13:35 PST
(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.
Comment 58 darshan 2019-02-18 09:49:17 PST
(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.
Comment 59 darshan 2019-02-18 09:55:32 PST
(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!
Comment 60 Fujii Hironori 2019-02-18 22:42:47 PST
(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