Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a wrapper module for add_test #240

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

FeignClaims
Copy link
Contributor

from aminya/cpp_vcpkg_project#35.

Additionally, add [WORKING_DIRECTORY <dir>] and [WILL_FAIL] to specify the corresponding test property.

@FeignClaims FeignClaims marked this pull request as draft October 2, 2023 03:19
@FeignClaims FeignClaims marked this pull request as ready for review October 2, 2023 03:28
Copy link
Owner

@aminya aminya left a comment

Choose a reason for hiding this comment

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

Could you use the added functions in some of the test projects?

src/Tests.cmake Outdated
Comment on lines 30 to 36
if(${type} STREQUAL "library_test")
add_executable(${target_name})
set(scope PRIVATE)
elseif(${type} STREQUAL "test_config")
add_library(${target_name} INTERFACE)
set(scope INTERFACE)
endif()
Copy link
Owner

Choose a reason for hiding this comment

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

What happens in the else condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function _configure_target currently only supports type "library_test" and "test_config". Here the if-else checks the type to add the corresponding target, and set the scope correctly.

For instance, a "library_test" is an executable target, it will link all its libraries PRIVATE-ly; a "test_config" is an interface library as it is used to propagate the test configuration, and it will link its libraries libraries INTERFACE-ly.

# Enable coverage reporting for gcc/clang
function(enable_coverage _project_name)
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES ".*Clang")
target_compile_options(${_project_name} INTERFACE --coverage -O0 -g)
target_link_libraries(${_project_name} INTERFACE --coverage)
endif()
endfunction()

function(_configure_target target_name type)
Copy link
Owner

Choose a reason for hiding this comment

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

I like how this function lumps together all the CMake functions making things more declarative. Could you name it as something like add_target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is whether I should split DEPENDENCIES_CONFIG, DEPENDENCIES and LIBRARIES to [PRIVATE|PUBLIC|INTERFACE]_DEPENDENCIES_CONFIG and so on.

Currently this function only supports "library_test" (executable) and "test_config" (header-only library), they are expected to only link one dependency type (PRIVATE for executable and INTERFACE for header-only library).

But for a non-header-only library, it might link a PRIVATE library, a PUBLIC library and a INTERFACE library at the same time. If this function is named as add_blah, users might expect this function also works on non-header-only libraries, so for a correct linkage, DEPENDENCIES_CONFIG must be split to [PRIVATE|PUBLIC|INTERFACE]_DEPENDENCIES_CONFIG.

@FeignClaims
Copy link
Contributor Author

FeignClaims commented Oct 17, 2023

Added these functions to test projects.

@FeignClaims
Copy link
Contributor Author

Changed test projects based on this feature and tested well, ready to merge.

Copy link
Contributor

@ClausKlein ClausKlein left a comment

Choose a reason for hiding this comment

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

Add an example or document the use of add_test() with multiple args please.

@@ -97,7 +97,7 @@ add_subdirectory(libs)

## tests
enable_testing()
add_test(NAME main COMMAND main)
add_executable_test(main no_arg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should I write no_arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The add_executable_test actually creates a test named test.<executable>.<test_name>. Since users might write multiple tests for an executable, no_arg here is just a name saying this test has no arg.

@@ -48,4 +48,4 @@ target_link_libraries(another_main PRIVATE myproj::lib myproj::lib2)

# tests
enable_testing()
add_test(NAME another_main COMMAND another_main)
add_executable_test(another_main no_arg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should I write no_arg?

@FeignClaims
Copy link
Contributor Author

Add an example or document the use of add_test() with multiple args please.

There's already an example in the document: https://github.com/FeignClaims/project_options/blob/9170c3b7b5e68840d01d79beed3b85948cd71d0e/src/Tests.cmake#L350-L355

Additionally, I added a use of add_executable_test() with args in tests.

add_executable_test(main no_arg)
add_executable_test(main with_arg
  EXECUTE_ARGS hello world
)

@FeignClaims FeignClaims changed the title Add a wrapper module for add_test feat: add a wrapper module for add_test Feb 23, 2024
@FeignClaims FeignClaims marked this pull request as draft May 6, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants