Bug 90671 – [CMake][EFL] Build and run TestWebKitAPI unit tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Thiago Marcos P. Santos]

Add some free tests to our test suite.

[Thiago Marcos P. Santos]

Created attachment 151084 [details]
Patch

The current WIP is enough for running WTF tests.

[Thiago Marcos P. Santos]

Created attachment 151960 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 151960 [details]
Patch

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

> Tools/TestWebKitAPI/CMakeLists.txt:55
> +    -DTEST_WEBKIT2_RESOURCES_DIR=\"${TESTWEBKITAPI_DIR}/Tests/WebKit2\"

Nit : Place this line as below.
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/CMakeLists.txt#L116

> Tools/TestWebKitAPI/CMakeLists.txt:56
> +    -DTEST_INJECTED_BUNDLE_PATH=\"${TestWebKitAPIInjectedBundle_PATH}\"

ditto.

[Thiago Marcos P. Santos]

Created attachment 152513 [details]
Patch

[Daniel Bates]

Comment on attachment 152513 [details]
Patch

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

Looks sane to me. I had some minor nits.

> Tools/TestWebKitAPI/CMakeLists.txt:32
> +    ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DocumentStartUserScriptAlertCrash_Bundle.cpp
> +    ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp
> +    ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp

These lines aren't in sorted order according to the Unix sort command. In particular, ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DocumentStartUserScriptAlertCrash_Bundle.cpp should be after ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp.

> Tools/TestWebKitAPI/CMakeLists.txt:77
> +    ${TESTWEBKITAPI_DIR}/Tests/WTF/VectorBasic.cpp
> +    ${TESTWEBKITAPI_DIR}/Tests/WTF/Vector.cpp

These lines should be swapped such that ${TESTWEBKITAPI_DIR}/Tests/WTF/Vector.cpp comes before ${TESTWEBKITAPI_DIR}/Tests/WTF/VectorBasic.cpp so that this list is in sorted order according to the Unix sort command.

> Tools/TestWebKitAPI/PlatformEfl.cmake:33
> +# The list bellow works like a test expectations. Tests at the

bellow => below
expectations => expectation
at => in

> Tools/TestWebKitAPI/PlatformEfl.cmake:35
> +# Tests at test_webkit2_api_fail_BINARIES are compiled and suffixed

"at test_webkit2_api_fail_BINARIES" => "in the test_webkit2_api_fail_BINARIES list"

I'm unclear what the lists test_webkit2_api_BINARIES and test_webkit2_api_fail_BINARIES represent. I take it that they represents the binaries corresponding to the tests that pass and fail, respectively? I suggest that we elaborate on the purpose of these lists.

> Tools/TestWebKitAPI/PlatformEfl.cmake:40
> +# Release builds before adding it to the list.

Which list? I take it that we should add passing tests to the list test_webkit2_api_BINARIES?

> Tools/TestWebKitAPI/PlatformEfl.cmake:45
> +    DocumentStartUserScriptAlertCrash
> +    DOMWindowExtensionNoCache

These lines should be swapped such that DOMWindowExtensionNoCache comes before DocumentStartUserScriptAlertCrash so that this list is in sorted order according to the Unix sort command.

> Tools/TestWebKitAPI/PlatformEfl.cmake:78
> +    CanHandleRequest
> +    NewFirstVisuallyNonEmptyLayoutFrames
> +    RestoreSessionStateContainingFormData
> +    DownloadDecideDestinationCrash
> +    NewFirstVisuallyNonEmptyLayoutForImages
> +    WKPageGetScaleFactorNotZero
> +    DOMWindowExtensionBasic
> +    ShouldGoToBackForwardListItem

Please sort these according to the Unix sort command.

> Tools/TestWebKitAPI/efl/main.cpp:35
> +static bool parseArguments(int argc, char** argv)

Currently this function only parses and returns the value for the useX11Window command line option. I take it that you envision accepting additional command line options in the future given the generalness of the name of this function. Unless you plan to accept additional command line options in the short term, you may want to consider using a more specific name for this function, say checkForUseX11WindowArgument.

> Tools/TestWebKitAPI/efl/main.cpp:37
> +    int ret = 0;

Can we come up with a more descriptive name for this? Maybe shouldUseX11Window? hasUseX11WindowOption? value?

> Tools/TestWebKitAPI/efl/main.cpp:59
> +    int ret = TestWebKitAPI::TestsController::shared().run(argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE;

I suggest that we write out "return code" as returnCode out, for clarity, instead of abbreviating it as ret.

[Thiago Marcos P. Santos]

Created attachment 153005 [details]
Patch

Thanks for reviewing. I was piping from vim to the command line sort on Linux. I'm now using the built-in vim sort which seens to be aligned with Unix sort. Now I see why the style checker always complains about the sorting order of my include headers.

[WebKit Review Bot]

Comment on attachment 153005 [details]
Patch

Clearing flags on attachment: 153005

Committed r122973: <http://trac.webkit.org/changeset/122973>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.