Bug 90671 - [CMake][EFL] Build and run TestWebKitAPI unit tests
Summary: [CMake][EFL] Build and run TestWebKitAPI unit tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago Marcos P. Santos
URL:
Keywords:
Depends on: 90602
Blocks: 90451
  Show dependency treegraph
 
Reported: 2012-07-06 02:58 PDT by Thiago Marcos P. Santos
Modified: 2012-07-18 08:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.67 KB, patch)
2012-07-06 08:13 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (21.54 KB, patch)
2012-07-12 08:20 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (22.12 KB, patch)
2012-07-16 05:06 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (22.28 KB, patch)
2012-07-18 07:05 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch

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]
Patch
Comment 3 Gyuyoung Kim 2012-07-12 22:23:44 PDT
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.
Comment 4 Thiago Marcos P. Santos 2012-07-16 05:06:36 PDT
Created attachment 152513 [details]
Patch
Comment 5 Daniel Bates 2012-07-17 10:42:53 PDT
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.
Comment 6 Thiago Marcos P. Santos 2012-07-18 07:05:21 PDT
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.
Comment 7 WebKit Review Bot 2012-07-18 08:07:45 PDT
Comment on attachment 153005 [details]
Patch

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.