cmake // Added Ctest support for unittests #882

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@snikulov
Member

Hi all,
I've enabled ctest verification for cmake builds and tests/unit folder.
Now it is possible after build run
on Linux
make test
on Windows
ctest -VV -C <Debug|Release>

@mention-bot

By analyzing the blame information on this pull request, we identified @billhoffman, @Sukender and @Lekensteyn to be potential reviewers

@snikulov
Member

@bradking Brad, could you please review and comment? Thank you!

@bradking bradking and 1 other commented on an outdated diff Jun 16, 2016
CMakeLists.txt
@@ -1042,6 +1042,8 @@ if(BUILD_CURL_EXE)
add_subdirectory(src)
endif()
if(BUILD_CURL_TESTS)
+ include(CTest)
+ enable_testing()
@bradking
bradking Jun 16, 2016 Contributor

The CTest module calls enable_testing for us so we don't need to call it here.

@snikulov
snikulov Jun 16, 2016 Member

@bradking From which version? Looks I'm a bit outdated here.

@bradking
bradking Jun 16, 2016 Contributor

It has always done this.

@bradking
bradking Jun 16, 2016 Contributor

Note that it depends on BUILD_TESTING.

@bradking bradking and 1 other commented on an outdated diff Jun 16, 2016
tests/unit/CMakeLists.txt
+ ${CURL_SOURCE_DIR}/tests/libtest
+ ${CURL_SOURCE_DIR}/src
+ ${CURL_BINARY_DIR}/lib # To be able to reach "curl_config.h"
+ ${CURL_BINARY_DIR}/include # To be able to reach "curl/curlbuild.h"
+)
+
+foreach(_testfile ${UT_SRC})
+
+ get_filename_component(_testname ${_testfile} NAME_WE)
+ add_executable(${_testname} ${_testfile} ${UT_COMMON_FILES})
+ target_link_libraries(${_testname} libcurl ${CURL_LIBS})
+ set_target_properties(${_testname}
+ PROPERTIES COMPILE_DEFINITIONS "UNITTESTS")
+
+ add_test(NAME ${_testname}
+ WORKING_DIRECTORY ${EXECUTABLE_OUTPUT_PATH}
@bradking
bradking Jun 16, 2016 Contributor

I don't think EXECUTABLE_OUTPUT_PATH is set anywhere. I think this line can just be dropped.

@snikulov
snikulov Jun 16, 2016 Member

@bradking Agreed. It was copy-paste from my project. But anyway, sometimes it is required for DLL when shared library used in build.

@snikulov
Member

@bradking updated and rebased into single commit.

@bradking
Contributor

The CTest module defines a BUILD_TESTING option that controls most of its functionality, including its call to enable_testing. The intended way to use the CTest module is to include it unconditionally and then honor its BUILD_TESTING option:

include(CTest)
if(BUILD_TESTING)
  add_subdirectory(tests)
endif()

However, curl is currently defining its own BUILD_CURL_TESTS option. It also has a code path that includes the CTest module earlier in a BUILD_DASHBOARD_REPORTS condition. These should all be reconciled.

@snikulov
Member

@bradking So what is your proposal, exactly? Should I remove BUILD_CURL_TESTS and BUILD_DASHBOARD_REPORTS anywhere in scripts?

@bradking
Contributor

Should I remove BUILD_CURL_TESTS and BUILD_DASHBOARD_REPORTS anywhere in scripts?

That would be the simplest solution. Since the CMake files still warn that they are poorly maintained I don't think anyone expects stability in the current options.

@snikulov
Member

@bradking updated.

@bradking
Contributor

The change itself looks good. Please squash in the fixups and revise the commit message to explain the new logic change.

@snikulov snikulov CMake build now using BUILD_TESTING=ON/OFF (default is OFF) to build
tests and enabling CTest integration.
Options BUILD_CURL_TESTS and BUILD_DASHBOARD_REPORTS was removed.
0fe6e3c
@snikulov
Member

@bradking Done.

@bagder Daniel, Could you please merge?

@bagder bagder added a commit that closed this pull request Jun 21, 2016
@snikulov @bagder snikulov + bagder cmake: now using BUILD_TESTING=ON/OFF
CMake build now using BUILD_TESTING=ON/OFF (default is OFF) to build
tests and enabling CTest integration. Options BUILD_CURL_TESTS and
BUILD_DASHBOARD_REPORTS was removed.

Closes #882

Reviewed-by: Brad King
12e21fa
@bagder bagder closed this in 12e21fa Jun 21, 2016
@bagder
Member
bagder commented Jun 21, 2016

Done, thanks!

Small nit: please make the commit message to follow our commit styles and I'll have an easier job merging your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment