Bug 187872

Summary: test-webkitpy should take configuration command line options
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ddkilzer, dino, ews-watchlist, glenn, lforschler, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac   
OS: macOS 10.13   
See Also: https://bugs.webkit.org/show_bug.cgi?id=187916
Bug Depends on:    
Bug Blocks: 187776    
Attachments:
Description Flags
Patch none

Description Daniel Bates 2018-07-20 15:00:33 PDT 
Today I noticed that the Apple Sierra Debug WK2 Tests bot tries to build lldbWebKitTester for Release even though it is a debug tester and should have used the debug version of lldbWebKitTester that it downloaded:

[[
Failed to find latest selenium, falling back to existing 3.9.0 version
Building lldbWebKitTester ...
Failed to run "['/Volumes/Data/slave/sierra-debug-tests-wk2/build/Tools/Scripts/build-lldbwebkittester', '--release']" exit_code: 65
...
]]
<https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/7313/steps/webkitpy-test/logs/stdio>
Comment 1 Daniel Bates 2018-07-20 15:09:59 PDT 
Once we fix this bug then we can land the tests included in the patch for bug #187776.
Comment 2 Daniel Bates 2018-07-20 15:13:59 PDT 
One way to fix this is to take a similar approach as our other tools and teach test-webkitpy to take an optional --release/--debug command line argument so that it knows whether it must build lldbWebKitTester for release or debug.
Comment 3 Daniel Bates 2018-07-21 20:18:48 PDT 
Created attachment 345532 [details]
Patch
Comment 4 Daniel Bates 2018-07-21 20:21:03 PDT 
Once we fix this bug we should look to land the tests in attachment #345523 [details] (bug #187884).
Comment 5 Daniel Bates 2018-07-21 20:22:08 PDT 
Comment on attachment 345532 [details]
Patch

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

> Tools/Scripts/webkitpy/test/main.py:143
> +        configuration_group.add_option('--debug', action='store_const', const='Debug', dest="configuration",

Will change " => ' before landing.

> Tools/Scripts/webkitpy/test/main.py:145
> +        configuration_group.add_option('--release', action='store_const', const='Release', dest="configuration",

Ditto.
Comment 6 Alexey Proskuryakov 2018-07-22 00:12:59 PDT 
The growing complexity of this approach makes me wonder if test-webkitpy is even the right place to test the formatters. It's Python code for sure, but the tests are clearly not fitting here.
Comment 7 Daniel Bates 2018-07-22 15:19:34 PDT 
(In reply to Alexey Proskuryakov from comment #6)
> The growing complexity of this approach makes me wonder if test-webkitpy is
> even the right place to test the formatters. It's Python code for sure, but
> the tests are clearly not fitting here.

I am open to suggestions on how to reduce the complexity. Would it help reduce complexity to separate out the running of the lldbwebkit tests into its own test runner, say test-lldbwebkit?
Comment 8 David Kilzer (:ddkilzer) 2018-07-23 05:41:31 PDT 
Comment on attachment 345532 [details]
Patch

r=me
Comment 9 Daniel Bates 2018-07-23 10:02:41 PDT 
Comment on attachment 345532 [details]
Patch

Clearing flags on attachment: 345532

Committed r234099: <https://trac.webkit.org/changeset/234099>
Comment 10 Daniel Bates 2018-07-23 10:02:42 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-07-23 10:03:26 PDT 
<rdar://problem/42503722>
Comment 12 Alexey Proskuryakov 2018-07-23 10:16:27 PDT 
> Would it help reduce complexity to separate out the running of the lldbwebkit tests into its own test runner, say test-lldbwebkit?

That's a good idea. Since one needs to run these tests for any WebCore change and not just for webkitpy changes, it's not really a webkitpy test in the first place.
Comment 13 Daniel Bates 2018-07-23 11:40:59 PDT 
(In reply to Alexey Proskuryakov from comment #12)
> > Would it help reduce complexity to separate out the running of the lldbwebkit tests into its own test runner, say test-lldbwebkit?
> 
> That's a good idea.

Filed bug #187916 for this.

> Since one needs to run these tests for any WebCore
> change and not just for webkitpy changes, it's not really a webkitpy test in
> the first place.

For completeness, test-webkitpy is already a misnomer because it runs unit tests for the WebKit message generator code and QueueStatusServer unit tests (when the AppEngine SDK is installed) in addition to running tests for webkitpy.