Bug 187872 – test-webkitpy should take configuration command line options

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

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>

[Daniel Bates]

Once we fix this bug then we can land the tests included in the patch for bug #187776.

[Daniel Bates]

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]

Created attachment 345532 [details]
Patch

[Daniel Bates]

Once we fix this bug we should look to land the tests in attachment #345523 [details] (bug #187884).

[Daniel Bates]

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]

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]

(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 on attachment 345532 [details]
Patch

r=me

[Daniel Bates]

Comment on attachment 345532 [details]
Patch

Clearing flags on attachment: 345532

Committed r234099: <https://trac.webkit.org/changeset/234099>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42503722>

[Alexey Proskuryakov]

> 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]

(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.