[Webkit-unassigned] [Bug 68509] [EFL] Implementation of Unit Tests framework for the WebKit-Efl port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68509





--- Comment #28 from Thiago Marcos P. Santos <tmpsantos at gmail.com>  2012-05-24 07:52:18 PST ---
(From update of attachment 142656)
View in context: https://bugs.webkit.org/attachment.cgi?id=142656&action=review

One general problem that I see here is that our tests should be written in C, not C++, since the main customer of ours API are C programs. Not sure if this is really a big deal or not, but writing tests in C makes us consumers of our own API and might influence some design decisions.

> Source/WebKit/PlatformEfl.cmake:297
> +    ${EFLDEPS_LIBRARIES}

I don't think we need to link with all these libraries directly since we are not using it.

> Source/WebKit/PlatformEfl.cmake:305
> +    "${JAVASCRIPTCORE_DIR}/wtf"

Should use "${WTF_DIR}" here instead.

> Source/WebKit/PlatformEfl.cmake:306
> +    "${JAVASCRIPTCORE_DIR}/ForwardingHeaders"

Ditto.

> Source/WebKit/PlatformEfl.cmake:315
> +    ${EFLDEPS_LDFLAGS}

IMO not all are needed here.

> Source/WebKit/PlatformEfl.cmake:329
> +SET(DEFAULT_TEST_PAGE_DIR ${CMAKE_SOURCE_DIR}/Source/WebKit/efl/tests)

I would put it on resources like GTK.

> Source/WebKit/PlatformEfl.cmake:333
> +

I don't know if efl_test_launcher is a good name for this. If I understood correctly, this is a convenience around gtest, that loads a page for you. Maybe the class should be named EFLTestBase (or EWK?) instead of EFLTestLauncher and the library something like efl_test_utils.

> Source/WebKit/PlatformEfl.cmake:336
> +    ${WEBKIT_DIR}/efl/tests/src/UnitTestLauncher/EFLTestView.cpp

I don't see a need for this ./src/ folder here.

> Source/WebKit/PlatformEfl.cmake:339
> +SET(EUnitTests_SOURCES

This is a test example right?

I like the approach of having a file named after the subject of the test. The idea here is to test ewk_*.h right? And also would be nice to have one executable per tested API. It will make it easier to debug and find memory leaks. CMake + CTest can take care of creating a global test runner.

What about test_ewk_settings.cpp, test_ewk_view.cpp, etc.?

> Source/WebKit/efl/tests/src/EFLTestsRun.cpp:17
> +*/

Nit, needs a blank line here.

> Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestLauncher.cpp:42
> +    return true;

What if ewk_init() fails?

return ewk_init(); maybe?

> Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.cpp:24
> +

Nit. I think these headers should be packed and alpha sorted.

> Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.cpp:66
> +{ }

{
}

> Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.cpp:74
> +{ }

Ditto, same for the others bellow.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.