Bug 181043 – API test harness should be Python

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

[Jonathan Bedard]

The harness for our API tests should be in Python, not Perl. This will also allow re-using much of the code running layout tests, adopt improved simctl code and use multiple processes.

Note that some of these new features (such as using multiple processes) require changes to the tests as well, which will be tracked in different bugs.

[Radar WebKit Bug Importer]

<rdar://problem/36164410>

[Jonathan Bedard]

Created attachment 330054 [details]
Patch

[Jonathan Bedard]

Created attachment 330579 [details]
Patch

[Jonathan Bedard]

After <https://trac.webkit.org/changeset/226263>, we could fully replace the existing run-api-tests with a Python implementation.  Patch still needs some polish, I don't think it's ready to land.

A few example outputs:


run-api-tests --simulator TestWTF.WTF_WeakPtr:
Running tests
Ran 9 tests of 9 with 9 successful               
------------------------------
All tests successfully passed!


run-api-tests --simulator TestWTF.WTF_WeakPtr -v --no-build:
13:54:20.993 26571 Checking build ...
13:54:21.218 26571 "perl Tools/Scripts/webkit-build-directory --configuration --debug --ios-simulator" took 0.22s
13:54:21.301 26571 "/usr/bin/xcrun simctl list --json" took 0.08s
13:54:21.307 26571 The request for 'iPhone running iOS' matched <Device "iPhone 5s": 6762BB09-4651-4EB2-97C9-ADD707B57CCB. State: BOOTED. Type: iPhone 5s running iOS 12> exactly
13:54:21.307 26571 Attached to running simulator <Device "iPhone 5s": 6762BB09-4651-4EB2-97C9-ADD707B57CCB. State: BOOTED. Type: iPhone 5s running iOS 12>
13:54:21.307 26571 Collecting tests ...
13:54:21.467 26571 "/usr/bin/xcrun simctl spawn 6762BB09-4651-4EB2-97C9-ADD707B57CCB /Volumes/Data/Code/Stable/OpenSource/WebKitBuild/Debug-iphonesimulator/TestWTF --gtest_list_tests" took 0.16s
13:54:21.728 26571 "/usr/bin/xcrun simctl spawn 6762BB09-4651-4EB2-97C9-ADD707B57CCB /Volumes/Data/Code/Stable/OpenSource/WebKitBuild/Debug-iphonesimulator/TestWebKitAPI --gtest_list_tests" took 0.26s
13:54:21.733 26571 Found 9 tests
13:54:21.733 26571 Running tests
13:54:21.733 26571 Sharding tests ...
13:54:21.734 26571 worker/0 starting
13:54:21.902 26571 worker/0 TestWTF.WTF_WeakPtr.Assignment Passed
13:54:22.065 26571 worker/0 TestWTF.WTF_WeakPtr.Basic Passed
13:54:22.227 26571 worker/0 TestWTF.WTF_WeakPtr.Dereference Passed
13:54:22.397 26571 worker/0 TestWTF.WTF_WeakPtr.DerivedConstructAndAssign Passed
13:54:22.556 26571 worker/0 TestWTF.WTF_WeakPtr.Downcasting Passed
13:54:22.720 26571 worker/0 TestWTF.WTF_WeakPtr.Forget Passed
13:54:22.885 26571 worker/0 TestWTF.WTF_WeakPtr.MultipleFactories Passed
13:54:23.061 26571 worker/0 TestWTF.WTF_WeakPtr.Operators Passed
13:54:23.224 26571 worker/0 TestWTF.WTF_WeakPtr.RevokeAll Passed
13:54:23.224 26571 worker/0 exiting
13:54:23.224 26571 Ran 9 tests of 9 with 9 successful
13:54:23.224 26571 ------------------------------
13:54:23.224 26571 All tests successfully passed!
13:54:23.224 26571 Testing completed, Exit status: 0


run-api-tests --simulator UIPasteboardTests -q
worker/0 TestWebKitAPI.UIPasteboardTests.DataTransferGetDataWhenPastingPlatformRepresentations Failed
    <stdout + stderr from failed test>
------------------------------                                                             
Test suite failed

Failed

    TestWebKitAPI.UIPasteboardTests.DataTransferGetDataWhenPastingPlatformRepresentations
        <stdout + stderr from failed test>

[Jonathan Bedard]

Created attachment 333700 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #5)
> Created attachment 333700 [details]
> Patch

Updated the patch with the correctSimulatedDeviceManager.

[Jonathan Bedard]

Created attachment 337731 [details]
Patch

[Sam Weinig]

I find the new code much more confusing than the old code, which was quite hackable. Has an attempt to add multiple processes to the old perl code been attempted and determined to be impossible?

[Jonathan Bedard]

(In reply to Sam Weinig from comment #8)
> I find the new code much more confusing than the old code, which was quite
> hackable. Has an attempt to add multiple processes to the old perl code been
> attempted and determined to be impossible?

The bigger problem here would be attempting to port the Python simulator code to Perl. Booting multiple simulators has been a pain point for us in the past, and we have quite a bit of logic surrounding the creation and cleaning up of the managed simulators.

I've done my best to simplify this code, but our use of multi-process has this Manager/Runner paradigm, which is what I feel makes this code feel less hackable than the Perl version.

[Sam Weinig]

(In reply to Jonathan Bedard from comment #9)
> (In reply to Sam Weinig from comment #8)
> > I find the new code much more confusing than the old code, which was quite
> > hackable. Has an attempt to add multiple processes to the old perl code been
> > attempted and determined to be impossible?
> 
> The bigger problem here would be attempting to port the Python simulator
> code to Perl. Booting multiple simulators has been a pain point for us in
> the past, and we have quite a bit of logic surrounding the creation and
> cleaning up of the managed simulators.
> 
> I've done my best to simplify this code, but our use of multi-process has
> this Manager/Runner paradigm, which is what I feel makes this code feel less
> hackable than the Perl version.

Ok, that makes sense.

[EWS Watchlist]

Comment on attachment 337731 [details]
Patch

Attachment 337731 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7288041

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 337747 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[David Kilzer (:ddkilzer)]

Comment on attachment 337731 [details]
Patch

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

r=me if you make sure Windows ports are fixed before landing, but I really only did a high-level sanity check here.  I assume that you ran tests with the old and then the new script locally to verify the output is similar.  You may want to get a second review as well.

> Tools/Scripts/run-api-tests:3
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

Nit: It's now 2018.

> Tools/Scripts/webkitpy/api_tests/manager.py:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

Nit:  It's now 2018.

> Tools/Scripts/webkitpy/api_tests/manager.py:15
> +#     * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.

Please grab Source/WebCore/LICENSE-APPLE and replace the Google-specific text with that text (unless this was largely derived from Google source in another file).

> Tools/Scripts/webkitpy/api_tests/manager.py:46
> +    SUCCESS = 0
> +    FAILED_BUILD_CHECK = 1
> +    FAILED_COLLECT_TESTS = 2
> +    TEST_FAILURES = 3

Nit: Maybe change TEST_FAILURES to FAILED_TESTS to make it match the other enum values?

> Tools/Scripts/webkitpy/api_tests/run_api_tests.py:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

Nit:  It's now 2018.

> Tools/Scripts/webkitpy/api_tests/run_api_tests.py:15
> +#     * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.

Please grab Source/WebCore/LICENSE-APPLE and replace the Google-specific text with that text (unless this was largely derived from Google source in another file).

> Tools/Scripts/webkitpy/api_tests/run_api_tests.py:41
> +INTERUPT_EXIT_STATUS = -2

Nit:  Type INTERUPT_ => INTERRUPT_

> Tools/Scripts/webkitpy/api_tests/run_api_tests.py:124
> +        # FIXME: Default should be false, API tests should not be forced to run the singly

Nit:  Grammar:  "run the singly" => "run singly"

> Tools/Scripts/webkitpy/api_tests/runner.py:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

Nit:  It's now 2018.

> Tools/Scripts/webkitpy/api_tests/runner.py:15
> +#     * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.

Please grab Source/WebCore/LICENSE-APPLE and replace the Google-specific text with that text (unless this was largely derived from Google source in another file).

> Tools/Scripts/webkitpy/api_tests/runner.py:41
> +    PASSED = 0
> +    FAILED = 1
> +    CRASHED = 2
> +    TIMEOUT = 3
> +    DISABLED = 4

Prefix these enum names with "STATUS_" to hint that they are index values for NAME_FOR_STATUS?

> Tools/Scripts/webkitpy/port/base.py:1485
> +    def _build_api_tests(self):
> +        environment = self.host.copy_current_environment()
> +        env = environment.to_dictionary()

Nit:  Why not make this one line since environment is not used after these two lines?

[Jonathan Bedard]

Created attachment 338375 [details]
Patch

[Jonathan Bedard]

Created attachment 338456 [details]
Patch

[EWS Watchlist]

Comment on attachment 338456 [details]
Patch

Attachment 338456 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7390653

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 338504 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Jonathan Bedard]

Comment on attachment 338456 [details]
Patch

Windows layout tests are already red. This is a purely Python change, layout tests are still running.

[WebKit Commit Bot]

Comment on attachment 338456 [details]
Patch

Clearing flags on attachment: 338456

Committed r230998: <https://trac.webkit.org/changeset/230998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

How does this work? API tests aren't safe to run concurrently because they depend on the system-wide global states.

[Jonathan Bedard]

(In reply to Ryosuke Niwa from comment #21)
> How does this work? API tests aren't safe to run concurrently because they
> depend on the system-wide global states.

At the moment, we use one child process by default because many API tests aren't safe to run concurrently.  Note that this is actually not a problem on iOS, we could safely run concurrent API tests in two separate simulators.

[Daniel Bates]

Comment on attachment 338456 [details]
Patch

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

> Tools/Scripts/run-api-tests:-83
> -  --wtf-only            Only build and run TestWTF

As far as I can tell we no longer have an equivalent to this in the new code :( The lack of this options makes is more burdensome to iterate quickly on WTF as the new tool only allows a person to build the world or not build the world.

[Daniel Bates]

(In reply to Daniel Bates from comment #23)
> Comment on attachment 338456 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338456&action=review
> 
> > Tools/Scripts/run-api-tests:-83
> > -  --wtf-only            Only build and run TestWTF
> 
> As far as I can tell we no longer have an equivalent to this in the new code
> :( The lack of this options makes is more burdensome to iterate quickly on
> WTF as the new tool only allows a person to build the world or not build the
> world.

Filed bug #187893 for this.