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

Jump to bottom

trajectory_tracker_msgs: rename test target #37

Merged
merged 2 commits into from
Aug 26, 2020
Merged

trajectory_tracker_msgs: rename test target #37

merged 2 commits into from
Aug 26, 2020

Conversation

f-fl0
Copy link
Collaborator

@f-fl0 f-fl0 commented Aug 25, 2020

A tf2 test in the geometry2 package is also named test_convert.
https://github.com/ros/geometry2/blob/7d95bb259cff8f7b914266b04b6621b7f92f92e4/test_tf2/CMakeLists.txt#L25-L26
This leads to the following error when compiling both this package and geometry2 in the same workspace:

CMake Error at /opt/ros/noetic/share/catkin/cmake/test/gtest.cmake:180 (add_executable):
   add_executable cannot create target "test_convert" because another target
   with the same name already exists.  The existing target is an executable
   created in source directory "/home/user/catkin_ws/src/geometry2/test_tf2".
   See documentation for policy CMP0002 for more details.
 Call Stack (most recent call first):
   /opt/ros/noetic/share/catkin/cmake/test/gtest.cmake:89 (_catkin_add_executable_with_google_test)
   /opt/ros/noetic/share/catkin/cmake/test/gtest.cmake:37 (_catkin_add_google_test)
   neonavigation_msgs/trajectory_tracker_msgs/CMakeLists.txt:39 (catkin_add_gtest)

@f-fl0
Rename test target
b1684e2 
- A tf2 test in geometry2 package is also named test_convert.
- Rename the source file for consistency.
@f-fl0 f-fl0 requested a review from at-wat August 25, 2020 13:18
@at-wat
Copy link
Owner

at-wat commented Aug 25, 2020 •
edited
Loading

[#88] PASSED on melodic

All tests passed 
build/test_results/trajectory_tracker_msgs/gtest-test_conversion.xml: 4 tests
build/test_results/trajectory_tracker_msgs/roslint-trajectory_tracker_msgs.xml: 1 tests
Summary: 5 tests, 0 errors, 0 failures, 0 skipped

[#88] PASSED on kinetic

All tests passed 
build/test_results/trajectory_tracker_msgs/gtest-test_conversion.xml: 4 tests
build/test_results/trajectory_tracker_msgs/roslint-trajectory_tracker_msgs.xml: 1 tests
Summary: 5 tests, 0 errors, 0 failures, 0 skipped

[#88] PASSED on noetic

Tested on Alpine ROS

at-wat
at-wat reviewed Aug 26, 2020
View reviewed changes
trajectory_tracker_msgs/CMakeLists.txt Outdated
@@ -36,8 +36,8 @@ if(CATKIN_ENABLE_TESTING)

add_compile_options(-std=c++11)

catkin_add_gtest(test_convert test/src/test_convert.cpp)
target_link_libraries(test_convert ${catkin_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT})
catkin_add_gtest(test_conversion test/src/test_conversion.cpp)
Copy link
Owner

Choose a reason for hiding this comment

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

Something like test_path_with_velocity_conversion would be better to avoid any other naming collision.

@f-fl0
Address PR comments
b628e48 
- rename test target and source file to test_path_with_velocity_conversion.
@at-wat
Copy link
Owner

at-wat commented Aug 26, 2020 •
edited
Loading

[#89] PASSED on kinetic

All tests passed 
build/test_results/trajectory_tracker_msgs/gtest-test_path_with_velocity_conversion.xml: 4 tests
build/test_results/trajectory_tracker_msgs/roslint-trajectory_tracker_msgs.xml: 1 tests
Summary: 5 tests, 0 errors, 0 failures, 0 skipped

[#89] PASSED on melodic

All tests passed 
build/test_results/trajectory_tracker_msgs/gtest-test_path_with_velocity_conversion.xml: 4 tests
build/test_results/trajectory_tracker_msgs/roslint-trajectory_tracker_msgs.xml: 1 tests
Summary: 5 tests, 0 errors, 0 failures, 0 skipped

[#89] PASSED on noetic

Tested on Alpine ROS

@at-wat at-wat changed the title Rename test target trajectory_tracker_msgs: rename test target Aug 26, 2020
at-wat
at-wat approved these changes Aug 26, 2020
View reviewed changes
Copy link
Owner

@at-wat at-wat left a comment

Choose a reason for hiding this comment

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

LGTM

@at-wat at-wat merged commit 861ff92 into master Aug 26, 2020
@at-wat at-wat deleted the avoid-name-collision-in-test branch August 26, 2020 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@at-wat at-wat at-wat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@f-fl0 @at-wat