Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add support for gtest if testing is enabled.
Add unit tests for TQObject as an example for further reference.
  • Loading branch information
vgvassilev committed Apr 6, 2017
commit c35b27d7540b54e87204a44e29177a0769da2a7e
16 changes: 16 additions & 0 deletions cmake/modules/RootNewMacros.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1078,6 +1078,22 @@ function(ROOT_ADD_TEST test)

endfunction()

#----------------------------------------------------------------------------
# function ROOT_ADD_GTEST(<testsuite> <name> LIBRARIES)
#
function(ROOT_ADD_GTEST test_suite test_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is test_name TQObjectTests.cxx in the example below? If so, can you rename this to source then? And can we have multiple sources?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the name of the Binary (i.e. test suite). Yeah we can have multiple files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I don't see it that dramatic, I will clean up all eventual mess if there is any.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The team converged on globbing.

CMAKE_PARSE_ARGUMENTS(ARG "" "" "LIBRARIES" ${ARGN})

include_directories(${GTEST_INCLUDE_DIR} ${GMOCK_INCLUDE_DIR})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


# 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})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify in the comment why ROOT_EXECUTABLE is used here despite what the comment says?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

target_link_libraries(${test_suite} gtest gtest_main gmock gmock_main)
ROOT_ADD_TEST(gtest-${test_suite} COMMAND ${test_suite})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing context but it seems this is out of the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for runing the tests without setting thisroot.sh before. I.e. no, AFAIK this is a prerequisite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this was resolved internally. There was something to check, outside of this PR.

endfunction()


#----------------------------------------------------------------------------
# ROOT_ADD_TEST_SUBDIRECTORY( <name> )
#----------------------------------------------------------------------------
Expand Down
53 changes: 53 additions & 0 deletions cmake/modules/SearchInstalledSoftware.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1371,6 +1371,59 @@ if(tmva AND imt)
find_package(BLAS)
endif()


#---Download googletest--------------------------------------------------------------
if (testing)
# FIXME: Remove our version of gtest in roottest. We can reuse this one.
# Add gtest
# http://stackoverflow.com/questions/9689183/cmake-googletest
ExternalProject_Add(
googletest
URL https://github.com/google/googletest/archive/release-1.8.0.zip
# TIMEOUT 10
# # Force separate output paths for debug and release builds to allow easy
# # identification of correct lib in subsequent TARGET_LINK_LIBRARIES commands
# CMAKE_ARGS -DCMAKE_ARCHIVE_OUTPUT_DIRECTORY_DEBUG:PATH=DebugLibs
# -DCMAKE_ARCHIVE_OUTPUT_DIRECTORY_RELEASE:PATH=ReleaseLibs
# -Dgtest_force_shared_crt=ON
# Disable install step
INSTALL_COMMAND ""
# Wrap download, configure and build steps in a script to log output
LOG_DOWNLOAD ON
LOG_CONFIGURE ON
LOG_BUILD ON)

# Specify include dirs for gtest and gmock
ExternalProject_Get_Property(googletest source_dir)
set(GTEST_INCLUDE_DIR ${source_dir}/googletest/include)
set(GMOCK_INCLUDE_DIR ${source_dir}/googlemock/include)

# Libraries
ExternalProject_Get_Property(googletest binary_dir)
set(_G_LIBRARY_PATH ${binary_dir}/googlemock/)

# gtest
add_library(gtest IMPORTED STATIC GLOBAL)
set_property(TARGET gtest PROPERTY IMPORTED_LOCATION ${_G_LIBRARY_PATH}/gtest/libgtest.a)
add_dependencies(gtest googletest)

# gtest_main
add_library(gtest_main IMPORTED STATIC GLOBAL)
set_property(TARGET gtest_main PROPERTY IMPORTED_LOCATION ${_G_LIBRARY_PATH}/gtest/libgtest_main.a)
add_dependencies(gtest_main googletest)

# gmock
add_library(gmock IMPORTED STATIC GLOBAL)
set_property(TARGET gmock PROPERTY IMPORTED_LOCATION ${_G_LIBRARY_PATH}/libgmock.a)
add_dependencies(gmock googletest)

# gmock_main
add_library(gmock_main IMPORTED STATIC GLOBAL)
set_property(TARGET gmock_main PROPERTY IMPORTED_LOCATION ${_G_LIBRARY_PATH}/libgmock_main.a)
add_dependencies(gmock_main googletest)

endif()

#---Report non implemented options---------------------------------------------------
foreach(opt afs glite sapdb srp)
if(${opt})
Expand Down
4 changes: 4 additions & 0 deletions core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ add_subdirectory(thread)
add_subdirectory(zip)
add_subdirectory(lzma)

if(testing)
ROOT_ADD_TEST_SUBDIRECTORY(test)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this refactoring is for another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

endif()

if(NOT WIN32)
add_subdirectory(newdelete)
endif()
Expand Down
5 changes: 5 additions & 0 deletions core/test/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
############################################################################
# CMakeLists.txt file for testing ROOT (global) core package
############################################################################

add_subdirectory(base)
6 changes: 6 additions & 0 deletions core/test/base/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
set (link_libs Core)

ROOT_ADD_GTEST(CoreBaseTests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

TQObjectTests.cxx
LIBRARIES ${link_libs}
)
63 changes: 63 additions & 0 deletions core/test/base/TQObjectTests.cxx
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#include "gtest/gtest.h"
#include "gmock/gmock.h"

#include "Rtypes.h"
#include "TQObject.h"
#include "TQConnection.h"

#include "RQ_OBJECT.h"

// FIXME: We should think of a common place to put the mock objects.
class TQConnectionMock : public TQConnection {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you use TQConnectionMock?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

public:
virtual ~TQConnectionMock() {} // gmock requires it for cleanup on shutdown.
MOCK_METHOD1(SetArg, void(Long_t param));
MOCK_METHOD1(SetArg, void(ULong_t param));
MOCK_METHOD1(SetArg, void(Float_t param));
MOCK_METHOD1(SetArg, void(Double_t param));
MOCK_METHOD1(SetArg, void(Long64_t param));
MOCK_METHOD1(SetArg, void(ULong64_t param));
MOCK_METHOD1(SetArg, void(const char *param));
MOCK_METHOD2(SetArg, void(const Long_t *params, Int_t nparam /* = -1*/));

MOCK_METHOD0(SendSignal, void());

// MOCK_METHOD1(SendSignal, void());
};

#define Stringify(s) Stringifyx(s)
#define Stringifyx(s) #s

#define DICT_CLASS \
class RQ_OBJECTTester : public TQObject { \
/* This will expand, adding signal/slot support to this class */ \
RQ_OBJECT("RQ_OBJECTTester"); \
Int_t fValue = 0; \
\
public: \
void SetValue(Int_t value) \
{ \
/* to prevent infinite looping in the case of cyclic connections */ \
if (value != fValue) { \
fValue = value; \
Emit("SetValue(Int_t)", fValue); \
} \
} \
void PrintValue() const { printf("value=%d\n", fValue); } \
Int_t GetValue() const { return fValue; } \
};

DICT_CLASS;

TEST(TQObject, Emit)
{
gInterpreter->ProcessLine(Stringify(DICT_CLASS));
RQ_OBJECTTester a;
RQ_OBJECTTester b;
a.Connect("SetValue(Int_t)", "RQ_OBJECTTester", &b, "SetValue(Int_t)");

EXPECT_EQ(0, b.GetValue());

a.SetValue(1);
EXPECT_EQ(1, b.GetValue());
}