Bug 197133 - Make it possible to control the renderTreeAsText output by setting options on testRunner
Summary: Make it possible to control the renderTreeAsText output by setting options on...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 197134
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-19 20:58 PDT by Simon Fraser (smfr)
Modified: 2019-04-25 12:48 PDT (History)
14 users (show)

See Also:


Attachments
Patch (67.91 KB, patch)
2019-04-19 20:59 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (82.17 KB, patch)
2019-04-19 22:29 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (68.71 KB, patch)
2019-04-19 22:34 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.70 MB, application/zip)
2019-04-20 06:31 PDT, Build Bot
no flags Details
Patch (76.87 KB, patch)
2019-04-20 10:38 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (78.76 KB, patch)
2019-04-20 11:02 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (78.78 KB, patch)
2019-04-20 11:17 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (78.78 KB, patch)
2019-04-20 11:30 PDT, Simon Fraser (smfr)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.28 MB, application/zip)
2019-04-22 02:48 PDT, Build Bot
no flags Details
Patch (82.87 KB, patch)
2019-04-22 15:38 PDT, Simon Fraser (smfr)
sam: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.24 MB, application/zip)
2019-04-23 18:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (12.20 MB, application/zip)
2019-04-23 19:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.07 MB, application/zip)
2019-04-23 19:41 PDT, Build Bot
no flags Details
Patch (82.81 KB, patch)
2019-04-24 09:38 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (82.78 KB, patch)
2019-04-24 09:52 PDT, Simon Fraser (smfr)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.62 MB, application/zip)
2019-04-24 11:42 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-04-19 20:58:56 PDT
Make it possible to control the renderTreeAsText output by setting options on testRunner
Comment 1 Simon Fraser (smfr) 2019-04-19 20:59:34 PDT Comment hidden (obsolete)
Comment 2 Simon Fraser (smfr) 2019-04-19 21:02:08 PDT
Don't have the JSC C API to do this yet (bug 197134).
Comment 3 Simon Fraser (smfr) 2019-04-19 22:29:47 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2019-04-19 22:32:26 PDT Comment hidden (obsolete)
Comment 5 Simon Fraser (smfr) 2019-04-19 22:34:49 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-04-19 22:37:58 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-04-20 06:31:03 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2019-04-20 06:31:05 PDT Comment hidden (obsolete)
Comment 9 Simon Fraser (smfr) 2019-04-20 10:38:28 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-04-20 10:42:15 PDT Comment hidden (obsolete)
Comment 11 Simon Fraser (smfr) 2019-04-20 11:02:55 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-04-20 11:04:50 PDT Comment hidden (obsolete)
Comment 13 Simon Fraser (smfr) 2019-04-20 11:17:33 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2019-04-20 11:20:22 PDT Comment hidden (obsolete)
Comment 15 Simon Fraser (smfr) 2019-04-20 11:30:13 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2019-04-20 11:32:59 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2019-04-22 02:48:03 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2019-04-22 02:48:13 PDT Comment hidden (obsolete)
Comment 19 Shawn Roberts 2019-04-22 09:09:22 PDT
EWS bots are hanging after processing this patch. 

Obsoleting the Patch so bots will stop processing.
Comment 20 Simon Fraser (smfr) 2019-04-22 15:38:58 PDT
Created attachment 367988 [details]
Patch
Comment 21 Build Bot 2019-04-22 15:42:20 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2019-04-23 18:46:32 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2019-04-23 18:46:34 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2019-04-23 19:08:29 PDT Comment hidden (obsolete)
Comment 25 Build Bot 2019-04-23 19:08:40 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2019-04-23 19:41:31 PDT Comment hidden (obsolete)
Comment 27 Build Bot 2019-04-23 19:41:33 PDT Comment hidden (obsolete)
Comment 28 Sam Weinig 2019-04-23 22:26:33 PDT
Comment on attachment 367988 [details]
Patch

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

> Tools/DumpRenderTree/TestRunner.cpp:115
> +    double options = JSValueToNumber(context, arguments[0], exception);
> +    ASSERT(!*exception);

Seems like this could throw if the value passed in is not convertible to a number. Seems like checking for the exception and returning early would be clearer to test writers than crashing. That may not be what we've done elsewhere, but seems like a better pattern to me.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:95
> +    # Underscores to camel case.
> +    $constantName =~ s{(\w+)}{
> +            ($a = lc $1) =~ s<(^[a-z]|_[a-z])><
> +                ($b = uc $1) =~ s/^_//;
> +                $b;
> +            >eg;
> +            $a;
> +        }eg;

This seems unnecessary. I'd just keep it using underscores, which are totally legal. This is generated code and doesn't have to conform to the normal rules.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:621
> +    my $attributes = [];

Can you call this $attributesAndConstants to make me happier :).

> LayoutTests/fast/harness/render-tree-as-text-options.html:36
> +                    testRunner.RENDER_TREE_SHOW_ALL_LAYERS + 
> +                    testRunner.RENDER_TREE_SHOW_LAYER_NESTING + 

Why '+' rather than the more traditional '|'? Is this the syntax you want, or would rather it be TestRunner.RENDER_TREE_FOO, e.g. on the constructor?
Comment 29 Simon Fraser (smfr) 2019-04-24 09:38:45 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2019-04-24 09:42:27 PDT
Attachment 368132 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestRunner.cpp:2087:  getRENDER_TREE_SHOW_ALL_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2092:  getRENDER_TREE_SHOW_LAYER_NESTING is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2097:  getRENDER_TREE_SHOW_COMPOSITED_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2102:  getRENDER_TREE_SHOW_OVERFLOW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2107:  getRENDER_TREE_SHOW_SVG_GEOMETRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2112:  getRENDER_TREE_SHOW_LAYER_FRAGMENTS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/rendering/RenderTreeAsText.cpp:617:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 7 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Simon Fraser (smfr) 2019-04-24 09:52:21 PDT
Created attachment 368135 [details]
Patch
Comment 32 Build Bot 2019-04-24 09:54:36 PDT
Attachment 368135 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestRunner.cpp:2087:  getRENDER_TREE_SHOW_ALL_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2092:  getRENDER_TREE_SHOW_LAYER_NESTING is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2097:  getRENDER_TREE_SHOW_COMPOSITED_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2102:  getRENDER_TREE_SHOW_OVERFLOW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2107:  getRENDER_TREE_SHOW_SVG_GEOMETRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2112:  getRENDER_TREE_SHOW_LAYER_FRAGMENTS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/rendering/RenderTreeAsText.cpp:617:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 7 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 WebKit Commit Bot 2019-04-24 11:25:32 PDT
Comment on attachment 368135 [details]
Patch

Clearing flags on attachment: 368135

Committed r244599: <https://trac.webkit.org/changeset/244599>
Comment 34 Build Bot 2019-04-24 11:42:47 PDT
Comment on attachment 368135 [details]
Patch

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

New failing tests:
fast/harness/render-tree-as-text-options.html
Comment 35 Build Bot 2019-04-24 11:42:58 PDT
Created attachment 368151 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 36 Darin Adler 2019-04-24 12:56:56 PDT
Comment on attachment 368135 [details]
Patch

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

> Source/WebCore/rendering/RenderTreeAsText.h:41
> +enum class RenderAsTextFlag {

Was nice to modernize this a little. Maybe turn it into an OptionSet later?
Comment 37 Darin Adler 2019-04-24 12:57:26 PDT
Comment on attachment 368135 [details]
Patch

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

>> Source/WebCore/rendering/RenderTreeAsText.h:41
>> +enum class RenderAsTextFlag {
> 
> Was nice to modernize this a little. Maybe turn it into an OptionSet later?

Oh, I see, that’s what you did!
Comment 38 Ryan Haddad 2019-04-25 12:47:25 PDT
This landed in https://trac.webkit.org/changeset/244599/webkit
Comment 39 Radar WebKit Bug Importer 2019-04-25 12:48:30 PDT
<rdar://problem/50215914>