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.

Sign up for GitHub

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

Adapt unit tests #9

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Adapt unit tests #9

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df6b976
Adapt unit tests according https://everest.github.io/nightly/general/…
andistorm Feb 5, 2024
442904e
Fix default value for options
andistorm Feb 6, 2024
4480ee6
Update CMakeLists.txt
andistorm Feb 27, 2024
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
6 changes: 4 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ find_package(everest-cmake 0.1 REQUIRED
#
option(BUILD_EXAMPLES "enable building of examples" OFF)
option(FSM_INSTALL "Install the library (shared data might be installed anyway)" ${EVC_MAIN_PROJECT})

option(${PROJECT_NAME}_BUILD_TESTING "Build unit tests, used if included as dependency" OFF)
option(BUILD_TESTING "Build unit tests, used if standalone project" OFF)

#
# library declaration
Expand All @@ -39,8 +40,9 @@ endif()
#
# tests
#
if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING)
if((${CMAKE_PROJECT_NAME} STREQUAL ${PROJECT_NAME} OR ${PROJECT_NAME}_BUILD_TESTING) AND BUILD_TESTING)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about the ... AND BUILD_TESTING here, if this is required, we probably will also trigger building tests in our external requirements like libcurl and maybe others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is the result of the discussion here: EVerest/EVerest#129 and and in the WG CI/CD & Testing.
I understand your concern, could be handled by using the options field in dependencies.yaml (setting BUILD_TESTING=OFF)

Copy link
Contributor

Choose a reason for hiding this comment

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

Has the discussion, you mentioned, taken into account, that requiring BUILD_TESTING would trigger unit tests in the dependencies?
Concerning the setting BUILD_TESTING in the options field - I'm not sure if this will work - because it is an option with global scope. So it can be only on or off, but not on for one project and off for another one.

include(CTest)
enable_testing()
Copy link
Contributor

Choose a reason for hiding this comment

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

https://cmake.org/cmake/help/latest/command/enable_testing.html

should not be necessary as we're requiring BUILD_TESTING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

andistorm marked this conversation as resolved.
Show resolved Hide resolved
add_subdirectory(tests)
endif()

Expand Down
11 changes: 6 additions & 5 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,21 @@ if (CODE_COVERAGE)
evc_include(CodeCoverage)
endif()

add_executable(test_state_allocator state_allocator.cpp)
target_link_libraries(test_state_allocator
set(TEST_TARGET_NAME ${PROJECT_NAME}_tests)
Copy link
Contributor

Choose a reason for hiding this comment

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

TEST_TARGET_NAME is to general here, especially when having multiple tests. There should be a TEST_PREFIX or something similar, which gets prepended to state_allocator

Copy link
Contributor

Choose a reason for hiding this comment

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

In other repos we re-use that TEST_TARGET_NAME variable for multiple test cases, so setting it to something like

set(TEST_TARGET_NAME ${PROJECT_NAME}_state_allocator_tests)

and later

set(TEST_TARGET_NAME ${PROJECT_NAME}_some_other_target_name_tests)

If we just use a TEST_PREFIX we still have to repeat the appended test name, but an argument could be made for unique variable names

add_executable(${TEST_TARGET_NAME} state_allocator.cpp)
target_link_libraries(${TEST_TARGET_NAME}
PRIVATE
fsm::fsm
Catch2::Catch2WithMain
)

if (CODE_COVERAGE)
append_coverage_compiler_flags_to_target(test_state_allocator)
append_coverage_compiler_flags_to_target(${TEST_TARGET_NAME})
setup_target_for_coverage_gcovr_html(
NAME coverage
EXECUTABLE test_state_allocator
EXECUTABLE ${TEST_TARGET_NAME}
BASE_DIRECTORY "${PROJECT_SOURCE_DIR}/include"
)
endif()

catch_discover_tests(test_state_allocator)
catch_discover_tests(${TEST_TARGET_NAME})