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

Daniel Bates
Reported 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>
Attachments
Patch (4.98 KB, patch)
2018-07-21 20:18 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 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.
Daniel Bates
Comment 2 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.
Daniel Bates
Comment 3 2018-07-21 20:18:48 PDT
Created attachment 345532 [details] Patch
Daniel Bates
Comment 4 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).
Daniel Bates
Comment 5 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.
Alexey Proskuryakov
Comment 6 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.
Daniel Bates
Comment 7 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?
David Kilzer (:ddkilzer)
Comment 8 2018-07-23 05:41:31 PDT
Comment on attachment 345532 [details] Patch r=me
Daniel Bates
Comment 9 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>
Daniel Bates
Comment 10 2018-07-23 10:02:42 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2018-07-23 10:03:26 PDT
<rdar://problem/42503722>
Alexey Proskuryakov
Comment 12 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.
Daniel Bates
Comment 13 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.
Note You need to log in before you can comment on or make changes to this bug.