Bug 90671

Summary: [CMake][EFL] Build and run TestWebKitAPI unit tests
Product: WebKit Reporter: Thiago Marcos P. Santos <tmpsantos>
Component: WebKit EFLAssignee: Thiago Marcos P. Santos <tmpsantos>
Severity: Normal CC: dbates, gyuyoung.kim, gyuyoung.kim, k.czech, lucas.de.marchi, rakuco, rwlbuis, sw0524.lee, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90602    
Bug Blocks: 90451    
Description Flags
Patch none

Description Thiago Marcos P. Santos 2012-07-06 02:58:41 PDT
Add some free tests to our test suite.
Comment 1 Thiago Marcos P. Santos 2012-07-06 08:13:21 PDT
Created attachment 151084 [details]

The current WIP is enough for running WTF tests.
Comment 2 Thiago Marcos P. Santos 2012-07-12 08:20:22 PDT
Created attachment 151960 [details]
Comment 3 Gyuyoung Kim 2012-07-12 22:23:44 PDT
Comment on attachment 151960 [details]

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

> Tools/TestWebKitAPI/CMakeLists.txt:55

Nit : Place this line as below.

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

Comment 4 Thiago Marcos P. Santos 2012-07-16 05:06:36 PDT
Created attachment 152513 [details]
Comment 5 Daniel Bates 2012-07-17 10:42:53 PDT
Comment on attachment 152513 [details]

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.
Comment 6 Thiago Marcos P. Santos 2012-07-18 07:05:21 PDT
Created attachment 153005 [details]

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.
Comment 7 WebKit Review Bot 2012-07-18 08:07:45 PDT
Comment on attachment 153005 [details]

Clearing flags on attachment: 153005

Committed r122973: <http://trac.webkit.org/changeset/122973>
Comment 8 WebKit Review Bot 2012-07-18 08:07:53 PDT
All reviewed patches have been landed.  Closing bug.