Add support for gtest if testing is enabled. #451
Conversation
cmake/modules/RootNewMacros.cmake
Outdated
| include_directories(${GTEST_INCLUDE_DIR} ${GMOCK_INCLUDE_DIR}) | ||
|
|
||
| # Note we cannot use ROOT_EXECUTABLE because it requires to pass LIBRARIES to link with. | ||
| # The test suites should chose this in their specific CMakeLists.txt file. |
There was a problem hiding this comment.
Could you clarify in the comment why ROOT_EXECUTABLE is used here despite what the comment says?
There was a problem hiding this comment.
That's not clear, I don't remember why I put this comment there. I think it means to say, we cannot just call ROOT_EXECUTABLE from the outside because we need to link against the gtest libraries or we will have to explicitly specify them outside.
|
@phsft-bot build! |
1 similar comment
|
@phsft-bot build! |
Axel-Naumann
left a comment
Axel-Naumann
left a comment
There was a problem hiding this comment.
Thanks for doing this, Vassil - I agree that this will be nice to have!
core/test/base/CMakeLists.txt
Outdated
| @@ -0,0 +1,6 @@ | |||
| set (link_libs Core) | |||
|
|
|||
| ROOT_ADD_GTEST(CoreBaseTests | |||
There was a problem hiding this comment.
I'd prefer to have src/, inc/, test/ at the same level. E.g. sql/mysql would only be enabled if we have the mysql library in the system; that would automatically enable / disable the tests with it.
There was a problem hiding this comment.
I agree. It makes much more sense to have the test for a module under that module's directory rather than under the category (and core is a bad example to talk about the differences between the two :)).
| # | ||
| function(ROOT_ADD_GTEST test_suite test_name) | ||
| CMAKE_PARSE_ARGUMENTS(ARG "" "" "LIBRARIES" ${ARGN}) | ||
|
|
There was a problem hiding this comment.
Can we add the source directory to the include dirs, or does that happen by default? (I want to be able to #include a header in the tests.)
|
|
||
| #---------------------------------------------------------------------------- | ||
| # function ROOT_ADD_GTEST(<testsuite> <name> LIBRARIES) | ||
| # |
There was a problem hiding this comment.
Is test_name TQObjectTests.cxx in the example below? If so, can you rename this to source then? And can we have multiple sources?
There was a problem hiding this comment.
This is the name of the Binary (i.e. test suite). Yeah we can have multiple files.
There was a problem hiding this comment.
How do I specify multiple sources? Can you add this to the doc of the CMake function? (I don't see how binary name and source files map to <testsuite> and <name>).
|
|
||
| #---------------------------------------------------------------------------- | ||
| # function ROOT_ADD_GTEST(<testsuite> <name> LIBRARIES) | ||
| # |
There was a problem hiding this comment.
In roottest, I found the following parameters useful:
CMAKE_PARSE_ARGUMENTS(ARG
"WILLFAIL"
""
"COPY_TO_BUILDDIR;DEPENDS;OPTS;LABELS;ENVIRONMENT"
${ARGN})
Also, I just glob the test sources:
file(GLOB unittests_SRC
"*.h"
"*.hh"
"*.hpp"
"*.hxx"
"*.cpp"
"*.cxx"
"*.cc"
"*.C"
)
Just drop a source file into test/ and it's part of the unit test.
There was a problem hiding this comment.
Yeah, that could be done when we start adding tests and need it.
I don't like globbing because it causes quite a few issues if the source folder is unclean, etc. In general, it would be much more often editing tests rather than adding new ones.
There was a problem hiding this comment.
I would request that we do this from day 0. Else it'll be a mess, with people adding their files by hand, until you find the time to do it.
There was a problem hiding this comment.
Thanks for the suggestion. I don't see it that dramatic, I will clean up all eventual mess if there is any.
There was a problem hiding this comment.
The team converged on globbing.
| # Note we cannot use ROOT_EXECUTABLE because it requires to pass LIBRARIES to link with. | ||
| # The test suites should chose this in their specific CMakeLists.txt file. | ||
| ROOT_EXECUTABLE(${test_suite} ${test_name} LIBRARIES ${ARG_LIBRARIES}) | ||
| target_link_libraries(${test_suite} gtest gtest_main gmock gmock_main) |
There was a problem hiding this comment.
I believe this is missing some environment:
set(environment ENVIRONMENT
${ROOTTEST_ENV_EXTRA}
${ARG_ENVIRONMENT}
ROOTSYS=${ROOTSYS}
PATH=${_path}:$ENV{PATH}
PYTHONPATH=${_pythonpath}:$ENV{PYTHONPATH}
${ld_library_path}=${_librarypath}:$ENV{${ld_library_path}})
is what I pass to ROOT_ADD_TEST in roottest/cmake/modules/RootCTestMacros.cmake's ROOTTEST_ADD_UNITTEST_DIR
There was a problem hiding this comment.
Maybe I am missing context but it seems this is out of the scope of this PR.
There was a problem hiding this comment.
This is needed for runing the tests without setting thisroot.sh before. I.e. no, AFAIK this is a prerequisite.
There was a problem hiding this comment.
IIUC, this was resolved internally. There was something to check, outside of this PR.
core/CMakeLists.txt
Outdated
| add_subdirectory(lzma) | ||
|
|
||
| if(testing) | ||
| ROOT_ADD_TEST_SUBDIRECTORY(test) |
There was a problem hiding this comment.
The nice thing about moving this next to src/ is that we can now systematically include the test/ subdirs in ROOT_LINKER_LIBRARY in cmake/modules/RootNewMacros.cmake. And yes that macro is a misnomer since a while already, it should probably be called ROOT_ADD_MODULE.
There was a problem hiding this comment.
It seems this refactoring is for another PR.
core/test/base/TQObjectTests.cxx
Outdated
| #include "RQ_OBJECT.h" | ||
|
|
||
| // FIXME: We should think of a common place to put the mock objects. | ||
| class TQConnectionMock : public TQConnection { |
There was a problem hiding this comment.
Where do you use TQConnectionMock?
core/test/base/TQObjectTests.cxx
Outdated
| #include "RQ_OBJECT.h" | ||
|
|
||
| // FIXME: We should think of a common place to put the mock objects. | ||
| class TQConnectionMock : public TQConnection { |
There was a problem hiding this comment.
Can you (to get us started) include a very simple test - something without mocks etc, but where you literally check whether say TNamed::GetName() returns what was set in the TNamed constructor?
|
The comments have been addressed. |
|
Starting build on |
2 similar comments
|
Starting build on |
|
Starting build on |
|
Starting build on |
1 similar comment
|
Starting build on |
|
@phsft-bot build! |
|
Starting build on |
|
Starting build on |
|
Starting build on |
|
Starting build on |
|
@phsft-bot build! |
|
Starting build on |
|
|
||
| endfunction() | ||
|
|
||
| #---------------------------------------------------------------------------- |
There was a problem hiding this comment.
Is that LIBRARIES <lib1> <lib2> or how does that work? Could you clarify in the comment/doc?
|
Starting build on |
|
Starting build on |
|
Starting build on |
| # The test suites should choose this in their specific CMakeLists.txt file. | ||
| # FIXME: For better coherence we could restrict the libraries the test suite could link | ||
| # against. For example, tests in Core should link only against libCore. This could be tricky | ||
| # to implement because some ROOT components create more than one library. |
There was a problem hiding this comment.
@peremato, this is the problematic line that puts the test binary in the ROOT install dir.
There was a problem hiding this comment.
The default output directory for binaries is controlled by this variable CMAKE_RUNTIME_OUTPUT_DIRECTORY. On possibility is to clear the variable before calling ROOT_EXECUTABLE(...) or setting the specific target property afterwards with
set_property(TARGET ${test_suite}
PROPERTY RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
|
Re the question "do we need to set env or not": That in turn sets So as long as these tests run through RootTestDriver.cmake we should be good. Do they? |
|
@axel, yes they will always go through the RootTestDriver.cmake because is using ROOT_ADD_TEST(...). The need to define LD_LIBRARY_PATH and PATH comes from dynamically loaded libraries and not to load the test executable. The test executable has the RPATH pointing to build tree. See for example: |
|
IMO, the unittest should depend as little as possible on environment. They should be written in such a manner that they don't need to tweak environment variables and so on. |
Add unit tests for TQObject as an example for further reference.
* Move test folder in every module; * Remove mock objects; * Clarify comment; * Add a simple test.
Using the BUILD_BYPRODUCTS directive we tell ninja that those libs will be created and it is ok to use them as dependencies. Further info: * ninja-build/ninja#760 * https://cmake.org/Bug/view.php?id=14963 * https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=557aef0b
|
Starting build on |
6 participants
Add unit tests for TQObject as an example for further reference.