Bug 68510

Created attachment 108122 [details] Added conditions for building gtest library and building unit tests.

Comment on attachment 108122 [details] Added conditions for building gtest library and building unit tests. Attachment 108122 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9773598

Attachment 108122 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/CMakeLists.txt', u'So..." exit_code: 1 Source/WebKit/efl/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 108123 [details] Patch

Attachment 108123 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/CMakeLists.txt', u'So..." exit_code: 1 Source/WebKit/efl/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 108123 [details] Patch Attachment 108123 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9765667

Comment on attachment 108123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108123&action=review > ChangeLog:3 > + [EFL] build scripts modifications to support unit tests. Capital 'b' in build. > ChangeLog:8 > + Added conditions for building gtest library and building unit tests. This could be moved to the * Source/CMakeLists.txt line. > Source/CMakeLists.txt:147 > +# Add google unit tests (gtest) I'd rather have all this in Source/WebKit/efl instead. > Source/WebKit/efl/tests/CMakeListsEfl.txt:37 > +ADD_DEFINITIONS(-DGTEST_TEST_FRAMEWORK) Is this needed for gtest to work? > Source/WebKit/efl/tests/CMakeListsEfl.txt:39 > +ADD_LIBRARY(efl_test_launcher What links against this library?

(In reply to comment #7) > (From update of attachment 108123 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108123&action=review > > > ChangeLog:3 > > + [EFL] build scripts modifications to support unit tests. > > Capital 'b' in build. > > > ChangeLog:8 > > + Added conditions for building gtest library and building unit tests. > > This could be moved to the * Source/CMakeLists.txt line. > > > Source/CMakeLists.txt:147 > > +# Add google unit tests (gtest) > > I'd rather have all this in Source/WebKit/efl instead. > > > Source/WebKit/efl/tests/CMakeListsEfl.txt:37 > > +ADD_DEFINITIONS(-DGTEST_TEST_FRAMEWORK) > > Is this needed for gtest to work? This might be needed when gtest will be replaced by the other framework. unit test launcher should be a separate component. > > > Source/WebKit/efl/tests/CMakeListsEfl.txt:39 > > +ADD_LIBRARY(efl_test_launcher > > What links against this library? Unit tests links against this library.

Created attachment 108813 [details] Patch

Created attachment 108819 [details] Patch

Comment on attachment 108819 [details] Patch Attachment 108819 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10145307

Build fails because this patch depends on 68509.

Dependency has been corrected. Could be tested again

(In reply to comment #13) > Dependency has been corrected. Could be tested again I'm wondering if this patch can be built without Bug 68509's patch. If this patch is only able to be built based on Bug 68509's patch, you should wait for landing the Bug 68509's patch.

This patch cannot be build separately. It should wait till 68509 will be applied.

Comment on attachment 108819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108819&action=review I don't see why this change should be in a separate patch instead of being sent together with bug 68509: this one will fail without the code, and the buildbot will not really test the patch in bug 68509 without the buildsystem changes. Could you please close this report and merge this patch with the one in bug 68509 (further discussions could then go in that report)? > Source/CMakeLists.txt:27 > +SET(GTEST_DIR "${THIRDPARTY_DIR}/gtest") No need to set this for all ports. > Source/CMakeLists.txt:146 > # ----------------------------------------------------------------------------- > +# GTest headers > +# ----------------------------------------------------------------------------- > +INCLUDE_DIRECTORIES(${GTEST_DIR} ${GTEST_DIR}/include) Ditto. > Source/CMakeLists.txt:156 > +# Add EFL unit tests > +# ----------------------------------------------------------------------------- > +INCLUDE_IF_EXISTS(${WEBKIT_DIR}/efl/tests/CMakeLists${PORT}.txt) This should not be here. Just add a `add_subdirectory(tests)` call in Sources/WebKit/efl/CMakeListsEfl.txt (you need to rename tests/CMakeListsEfl.txt to tests/CMakeLists.txt for this to work, which actually makes sense). > Source/WebKit/efl/CMakeListsEfl.txt:267 > +INCLUDE_DIRECTORIES(${GTEST_DIR} ${GTEST_DIR}/include) > + > +SET(GTEST_SOURCES ${THIRDPARTY_DIR}/gtest/src) > +ADD_LIBRARY(gtest > + ${GTEST_SOURCES}/gtest.cc > + ${GTEST_SOURCES}/gtest-death-test.cc > + ${GTEST_SOURCES}/gtest_main.cc > + ${GTEST_SOURCES}/gtest-filepath.cc > + ${GTEST_SOURCES}/gtest-port.cc > + ${GTEST_SOURCES}/gtest-test-part.cc > + ${GTEST_SOURCES}/gtest-typed-test.cc > +) This could go into tests/CMakeLists.txt. > Source/WebKit/efl/tests/CMakeListsEfl.txt:35 > +SET(DEFAULT_THEME_PATH ${CMAKE_BINARY_DIR}/WebKit/efl/DefaultTheme) > +ADD_DEFINITIONS(-DDEFAULT_THEME_PATH=\"${DEFAULT_THEME_PATH}\") There is now THEME_BINARY_DIR with the same value. > Source/WebKit/efl/tests/CMakeListsEfl.txt:37 > +ADD_DEFINITIONS(-DGTEST_TEST_FRAMEWORK) What is this used for?

I will merge this patch.

*** This bug has been marked as a duplicate of bug 90606 ***