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

Possibility to add multiple tests with different arguments #456

Closed
wants to merge 3 commits into
base: hydro-devel
from

Conversation

Projects
None yet
3 participants
@ct2034
Contributor

ct2034 commented Jul 3, 2014

I needed to add multiple tests with different arguments in one CMakeLists.txt. This led to a conflict as these had the same target name, due to the identical test-file-name. This includes arguments to the target name, if those are stated.

@ros-pull-request-builder

This comment has been minimized.

Show comment
Hide comment
@ros-pull-request-builder

ros-pull-request-builder Jul 3, 2014

Member

Can one of the admins verify this patch?

Member

ros-pull-request-builder commented Jul 3, 2014

Can one of the admins verify this patch?

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jul 3, 2014

Member

Please provide some example CMake lines how you intend to use this to help understanding your patch.

Member

dirk-thomas commented Jul 3, 2014

Please provide some example CMake lines how you intend to use this to help understanding your patch.

@ct2034

This comment has been minimized.

Show comment
Hide comment
@ct2034

ct2034 Jul 3, 2014

Contributor

About ARGN:

ARGN is only set if arguments are passed. This is the output of catkin_make on the test file mentioned below:

-- +++ processing catkin package: 'test_cmake'
-- ==> add_subdirectory(my_navigation_test/test_cmake)
ARGN: 
ARGN: ARGS;test1:=true
-- Added testname WITH arguments: launch_the_test___test1__true.test
ARGN: ARGS;test1:=true;test2:=true;test3:=true
-- Added testname WITH arguments: launch_the_test___test1__true_test2__true_test3__true.test

This line was added to rostest-extras.cmake.em: 'message("ARGN: ${ARGN}")'

Example code:

The following code is from my testpackage: https://github.com/ct2034/test_cmake/blob/master/CMakeLists.txt

add_rostest(launch/the.test) # <- this leads to an empty ARGN - see above
add_rostest(launch/the.test ARGS  test1:=true)
add_rostest(launch/the.test  ARGS test1:=true test2:=true  test3:=true)

This produces the output above. Without this patch it would fail, beacuse all the targets would be called 'launch_the_test'.

Contributor

ct2034 commented Jul 3, 2014

About ARGN:

ARGN is only set if arguments are passed. This is the output of catkin_make on the test file mentioned below:

-- +++ processing catkin package: 'test_cmake'
-- ==> add_subdirectory(my_navigation_test/test_cmake)
ARGN: 
ARGN: ARGS;test1:=true
-- Added testname WITH arguments: launch_the_test___test1__true.test
ARGN: ARGS;test1:=true;test2:=true;test3:=true
-- Added testname WITH arguments: launch_the_test___test1__true_test2__true_test3__true.test

This line was added to rostest-extras.cmake.em: 'message("ARGN: ${ARGN}")'

Example code:

The following code is from my testpackage: https://github.com/ct2034/test_cmake/blob/master/CMakeLists.txt

add_rostest(launch/the.test) # <- this leads to an empty ARGN - see above
add_rostest(launch/the.test ARGS  test1:=true)
add_rostest(launch/the.test  ARGS test1:=true test2:=true  test3:=true)

This produces the output above. Without this patch it would fail, beacuse all the targets would be called 'launch_the_test'.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jul 3, 2014

Member

The function uses cmake_parse_arguments to parse the passed arguments. What you want to use is _rostest_ARGS. Otherwise when passing WORKING_DIRECTORY somepath it would be using that too.

Member

dirk-thomas commented Jul 3, 2014

The function uses cmake_parse_arguments to parse the passed arguments. What you want to use is _rostest_ARGS. Otherwise when passing WORKING_DIRECTORY somepath it would be using that too.

@ct2034

This comment has been minimized.

Show comment
Hide comment
@ct2034

ct2034 Jul 3, 2014

Contributor

By the way: the thing I actually need the change for is this one: https://github.com/ct2034/my_navigation_test/blob/hydro_dev/CMakeLists.txt

Contributor

ct2034 commented Jul 3, 2014

By the way: the thing I actually need the change for is this one: https://github.com/ct2034/my_navigation_test/blob/hydro_dev/CMakeLists.txt

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas
Member

dirk-thomas commented Jul 10, 2014

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jul 10, 2014

Member

The patch should be applied to the latest branch which is indigo-devel. Therefore closing this PR in favor of #462. It might be cherry-picked to previous distro but for new features that depends on the need and chance of introducing regressions.

Member

dirk-thomas commented Jul 10, 2014

The patch should be applied to the latest branch which is indigo-devel. Therefore closing this PR in favor of #462. It might be cherry-picked to previous distro but for new features that depends on the need and chance of introducing regressions.

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