Bug 187872 - test-webkitpy should take configuration command line options
Summary: test-webkitpy should take configuration command line options
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari Technology Preview
Hardware: Mac macOS 10.13
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks: 187776
  Show dependency treegraph
 
Reported: 2018-07-20 15:00 PDT by Daniel Bates
Modified: 2018-07-23 11:41 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.98 KB, patch)
2018-07-21 20:18 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.