[taskd 4/5] tests: Refactor CMakeLists.txt to use CTest

[aszlig]

So far all the tests were run by a helper script called run_all, which
is generated from run_all.in and populated by all the .t files found in
the test directory.

CMake however has it's own testing functionality called CTest, with a
lot of features, particularly it allows running specific tests only and
it should be cross-platform compatible as well. Especially taskd should
in theory be cross-platform compatible, so in order to let's say run
tests on Windows, we only need Perl and Python (once the template.t test
is working) as additional dependencies without the need for cygwin or
similar.

The new implementation now matches the tests based on its filename, so
for example if you want to have a Python test, you simply put it in
test/yourtest.t.py, if you want a Perl test it's test/yourtest.t and if
it's a C++ test, put it in test/yourtest.t.cpp.

This also now should get rid of an ugly CMake warning, which was yielded
because we were using a reserved target called "test":

Policy CMP0037 is not set: Target names should not be reserved and
should match a validity pattern. Run "cmake --help-policy CMP0037"
for policy details. Use the cmake_policy command to set the policy
and suppress this warning.

The target name "test" is reserved or not valid for certain CMake
features, such as generator expressions, and may result in undefined
behavior.

More information about CMP0037 can be found at the following URL:

https://cmake.org/cmake/help/latest/policy/CMP0037.html

Signed-off-by: aszlig <asz...@redmoonstudios.org>
---
CMakeLists.txt | 1 +
test/CMakeLists.txt | 93 ++++++++++++++++++++++++++---------------------------
2 files changed, 46 insertions(+), 48 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0a83ffa..4ae419c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -164,6 +164,7 @@ add_subdirectory (src)
add_subdirectory (doc)

if (BUILD_TESTS)
+ enable_testing ()
add_subdirectory (test)
endif (BUILD_TESTS)

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 91df975..a733f6e 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -4,53 +4,50 @@ include_directories (${CMAKE_SOURCE_DIR}
${CMAKE_SOURCE_DIR}/test
${TASKD_INCLUDE_DIRS})

-set (test_SRCS color.t config.t date.t directory.t duration.t file.t msg.t
- nibbler.t path.t rx.t text.t util.t utf8.t json.t json_test)
-
-message ("-- Configuring run_all")
-if (${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
-
-set (TESTBLOB "./*.t")
-if (CYGWIN)
- set (TESTBLOB "./*.t ./*.t.exe")
-endif (CYGWIN)
-
-else (${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
-
-set (TESTBLOB "${CMAKE_SOURCE_DIR}/test/*.t ${CMAKE_BINARY_DIR}/test/*.t")
-if (CYGWIN)
- set (TESTBLOB "${CMAKE_SOURCE_DIR}/test/*.t ${CMAKE_BINARY_DIR}/test/*.t.exe")
-endif (CYGWIN)
-
-endif (${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
-
-configure_file (run_all.in run_all)
-
-add_custom_target (test ./run_all --verbose
- DEPENDS ${test_SRCS}
- WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/test)
-
-add_custom_target (build_tests DEPENDS ${test_SRCS}
- WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/test)
-
-foreach (src_FILE ${test_SRCS})
- add_executable (${src_FILE} "${src_FILE}.cpp"
- ../src/Color.cpp
- ../src/ConfigFile.cpp
- ../src/Date.cpp
- ../src/Directory.cpp
- ../src/Duration.cpp
- ../src/File.cpp
- ../src/JSON.cpp
- ../src/Msg.cpp
- ../src/Nibbler.cpp
- ../src/Path.cpp
- ../src/RX.cpp
- ../src/text.cpp
- ../src/utf8.cpp
- ../src/util.cpp
- ../src/wcwidth6.cpp
- test.cpp)
- target_link_libraries (${src_FILE} ${TASKD_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT})
+message ("-- Configuring tests")
+
+find_program (taskwarrior task)
+
+if (NOT taskwarrior)
+ message ("-- Taskwarrior client not found, skipping tests requiring it.")
+ set (client_env TASKW_SKIP=1)
+else (NOT taskwarrior)
+ message ("-- Found Taskwarrior client: ${taskwarrior}")
+ set (client_env TASK_PATH=${taskwarrior})
+endif (NOT taskwarrior)
+
+macro (add_taskd_test testname)
+ add_test (NAME ${testname}
+ COMMAND ${ARGN}
+ WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/test)
+ set_tests_properties (
+ ${testname} PROPERTIES ENVIRONMENT
+ "TASKD_PATH=$<TARGET_FILE:taskd_executable>;${client_env}"
+ )
+endmacro (add_taskd_test testname)
+
+macro (make_taskd_tests executable pattern)
+ file (GLOB testfiles ${CMAKE_SOURCE_DIR}/test/${pattern})
+ foreach (testfile ${testfiles})
+ get_filename_component (testfile_base ${testfile} NAME_WE)
+ set (testname "${executable}_test_${testfile_base}")
+ add_taskd_test (${testname} ${executable} ${testfile})
+ endforeach (testfile)
+endmacro (make_taskd_tests executable pattern)
+
+add_library (taskd_testing test.cpp)
+
+file (GLOB cc_testfiles *.t.cpp)
+foreach (src_FILE ${cc_testfiles})
+ get_filename_component (testfile_base ${src_FILE} NAME_WE)
+ add_executable (${testfile_base} ${src_FILE})
+ target_link_libraries (${testfile_base} taskd taskd_testing
+ ${TASKD_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT})
+ add_taskd_test ("cc_test_${testfile_base}" ${testfile_base})
endforeach (src_FILE)

+include (FindPerl)
+make_taskd_tests (perl *.t)
+
+include (FindPythonInterp)
+make_taskd_tests (python *.t.py)
--
2.8.0

[Paul Beckingham]

Thank you for the patch.

We do not use CTest at all, but I am curious as to what it can do, so I’ll try this, and see if it offers the same performance and parallelism as what the patch replaces. TBD.

The CMP0037 policy warning is a non-issue if you don’t use CTest. There are other ways of getting rid of it, if ugly warnings bother you:

if(POLICY CMP0037)
cmake_policy(SET CMP0037 OLD)
endif()

Paul

> --
> You received this message because you are subscribed to the Google Groups "taskwarrior-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to taskwarrior-d...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Paul Beckingham]

I see why the CMP0037 came up (because it was already addressed), and the patches won’t apply cleanly - they are patches against the master branch, not the 1.2.0 dev branch. Now I understand.

Paul

[Ben Boeckel]

On Sat, Apr 09, 2016 at 15:18:48 -0400, Paul Beckingham wrote:
> We do not use CTest at all, but I am curious as to what it can do, so
> I’ll try this, and see if it offers the same performance and
> parallelism as what the patch replaces. TBD.
>
> The CMP0037 policy warning is a non-issue if you don’t use CTest.
> There are other ways of getting rid of it, if ugly warnings bother
> you:
>
> if(POLICY CMP0037)
> cmake_policy(SET CMP0037 OLD)
> endif()

FYI, CMake policies are not "feature flags". They are notifications that
the old behavior is deprecated and will be going away.

However, for the "test" target, I have it (low) on my list to make it
"reserved" only if CTest is used. I suppose "package" for CPack is in
the same boat.

--Ben

[Paul Beckingham]

> FYI, CMake policies are not "feature flags". They are notifications that
> the old behavior is deprecated and will be going away.

I understand what they are, but I think making the generic “test” a reserved target for an optional product I never intend to use, arrogant. So I modify the policy so it doesn’t complain, and I continue to use it.

If someone knows how I can have a “make test” build target, and a “test” directory; without the warning, without using CTest, and without disabling the complaint, in the form of a patch that doesn’t make me spend more than thirty seconds on the matter, I’d be happy.

Sounds just as arrogant, huh? I’m just not interesting in tweaking build systems, I’d rather be fixing/improving the software.

P

[Ben Boeckel]

On Wed, Apr 13, 2016 at 15:14:25 -0400, Paul Beckingham wrote:
> I understand what they are, but I think making the generic “test” a
> reserved target for an optional product I never intend to use,
> arrogant. So I modify the policy so it doesn’t complain, and I
> continue to use it.
>
> If someone knows how I can have a “make test” build target, and a
> “test” directory; without the warning, without using CTest, and
> without disabling the complaint, in the form of a patch that doesn’t
> make me spend more than thirty seconds on the matter, I’d be happy.
>
> Sounds just as arrogant, huh? I’m just not interesting in tweaking
> build systems, I’d rather be fixing/improving the software.

Completely agree, but it's not like CMake is a perfect project either… A
bug to CMake is probably the start. As long as hacks like this exist and
the real reason is that the hack is a problem, patches will continue
coming in. I recommend filing a bug and then adding a comment above the
policy setting with a link to the bug so that future developers do not
waste their time on the taskwarrior side of the patch and instead might
look at the CMake side of the fix.

Here's bug to link to:

https://cmake.org/Bug/view.php?id=16062

Thanks,

--Ben

[Paul Beckingham]

> I recommend filing a bug and then adding a comment above the
> policy setting with a link to the bug so that future developers do not
> waste their time on the taskwarrior side of the patch and instead might
> look at the CMake side of the fix.

Good idea. Done. Thanks.

P

[Ben Boeckel]

On Sat, Apr 09, 2016 at 15:18:48 -0400, Paul Beckingham wrote:

> The CMP0037 policy warning is a non-issue if you don’t use CTest.
> There are other ways of getting rid of it, if ugly warnings bother
> you:
>
> if(POLICY CMP0037)
> cmake_policy(SET CMP0037 OLD)
> endif()

Sorry for the necroposting, but here's an update: CMP0037 will not warn
about a conflicting `test` target as of 3.11:

https://gitlab.kitware.com/cmake/cmake/merge_requests/1417

3.11 will start warning about setting this policy to `OLD` since the
plan is to remove the `OLD` behavior in future versions.

The conditional should now be:

if (POLICY CMP0037 AND CMAKE_VERSION VERSION_LESS 3.11)
cmake_policy(SET CMP0037 OLD)
endif ()

--Ben