Bug 181043 - API test harness should be Python
Summary: API test harness should be Python
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-20 09:33 PST by Jonathan Bedard
Modified: 2018-07-22 16:17 PDT (History)
16 users (show)

See Also:


Attachments
Patch (33.45 KB, patch)
2017-12-21 12:47 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (47.22 KB, patch)
2018-01-05 13:51 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (47.27 KB, patch)
2018-02-13 10:22 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (47.59 KB, patch)
2018-04-11 13:39 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.68 MB, application/zip)
2018-04-11 16:32 PDT, Build Bot
no flags Details
Patch (49.37 KB, patch)
2018-04-19 16:18 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (49.47 KB, patch)
2018-04-20 13:25 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.65 MB, application/zip)
2018-04-20 18:21 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-12-20 09:33:26 PST
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.
Comment 1 Radar WebKit Bug Importer 2017-12-20 14:24:06 PST
<rdar://problem/36164410>
Comment 2 Jonathan Bedard 2017-12-21 12:47:52 PST
Created attachment 330054 [details]
Patch
Comment 3 Jonathan Bedard 2018-01-05 13:51:55 PST
Created attachment 330579 [details]
Patch
Comment 4 Jonathan Bedard 2018-01-05 14:05:23 PST
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>
Comment 5 Jonathan Bedard 2018-02-13 10:22:38 PST
Created attachment 333700 [details]
Patch
Comment 6 Jonathan Bedard 2018-02-13 10:23:52 PST
(In reply to Jonathan Bedard from comment #5)
> Created attachment 333700 [details]
> Patch

Updated the patch with the correctSimulatedDeviceManager.
Comment 7 Jonathan Bedard 2018-04-11 13:39:57 PDT
Created attachment 337731 [details]
Patch
Comment 8 Sam Weinig 2018-04-11 14:00:40 PDT
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?
Comment 9 Jonathan Bedard 2018-04-11 14:46:16 PDT
(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.
Comment 10 Sam Weinig 2018-04-11 15:03:35 PDT
(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.
Comment 11 Build Bot 2018-04-11 16:32:36 PDT
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
Comment 12 Build Bot 2018-04-11 16:32:47 PDT
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
Comment 13 David Kilzer (:ddkilzer) 2018-04-18 12:22:12 PDT
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?
Comment 14 Jonathan Bedard 2018-04-19 16:18:25 PDT
Created attachment 338375 [details]
Patch
Comment 15 Jonathan Bedard 2018-04-20 13:25:23 PDT
Created attachment 338456 [details]
Patch
Comment 16 Build Bot 2018-04-20 18:20:59 PDT
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
Comment 17 Build Bot 2018-04-20 18:21:11 PDT
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
Comment 18 Jonathan Bedard 2018-04-25 09:08:25 PDT
Comment on attachment 338456 [details]
Patch

Windows layout tests are already red. This is a purely Python change, layout tests are still running.
Comment 19 WebKit Commit Bot 2018-04-25 09:35:39 PDT
Comment on attachment 338456 [details]
Patch

Clearing flags on attachment: 338456

Committed r230998: <https://trac.webkit.org/changeset/230998>
Comment 20 WebKit Commit Bot 2018-04-25 09:35:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Ryosuke Niwa 2018-04-26 16:28:53 PDT
How does this work? API tests aren't safe to run concurrently because they depend on the system-wide global states.
Comment 22 Jonathan Bedard 2018-04-27 14:14:39 PDT
(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.
Comment 23 Daniel Bates 2018-07-22 16:13:49 PDT
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.
Comment 24 Daniel Bates 2018-07-22 16:17:25 PDT
(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.