ros-perception / vision_msgs

Algorithm-agnostic computer vision message types for ROS.
Apache License 2.0
155 stars 74 forks source link

Fix dependency of unit test #14

Closed RonaldEnsing closed 5 years ago

RonaldEnsing commented 6 years ago

This PR fixes the unit test, making it able to run tests with catkin_make run_tests directly, without running catkin_make first. [1]

[1] http://wiki.ros.org/catkin/CMakeLists.txt#Important_Prerequisites.2BAC8-Constraints

Kukanani commented 6 years ago

This looks great, I'll merge it as soon as I have a chance to test it locally. Thanks for the code!

Kukanani commented 5 years ago

@RonaldEnsing, I tested locally without this PR and was able to catkin_make run_tests and catkin run_tests with no issues on a clean workspace/repository. In addition, this page does not show an add_dependencies call in the example. In my experience, run_tests automatically builds before testing.

I'm going to close this PR, but if I'm mistaken please let me know and we can revisit it.

RonaldEnsing commented 5 years ago

I have this issue on Ubuntu 16.04.5 LTS with ros-indigo-desktop-full installed. The ROS workspace consists of only the vision_msgs package with the commit corresponding to tag 0.0.1: main.cpp:17:39: fatal error: vision_msgs/BoundingBox2D.h: No such file or directory compilation terminated.

I think this is a race condition between generating the header files from the defined messages (BoundingBox2d) and executing the unit tests, and therefore does not have to happen. I also could not reproduce this on another system running Ubuntu 14.04.

By adding this dependency as in this PR this is solved. I tested this, it solves the issue and adding this dependency for the unit test is a guarantee that this race condition could not occur.

mintar commented 5 years ago

I agree with @RonaldEnsing; this PR is correct and should be merged. It's a race condition, so only because it sometimes doesn't crash doesn't mean it never will. Especially if you've built the workspace before, there will already be (stale) generated messages from the last compilation. If you want any chance of reproducing the race condition, you'll have to start with a clean workspace, but even then it's not guaranteed to happen.

In most unit tests, you will have a target_link_libraries from your test against a library that you're building, and that library will have a dependency on the message generation (${${PROJECT_NAME}_EXPORTED_TARGETS}). Therefore, the test will have a transitive dependency on the generated messages. This is not the case here: the test directly depends on the generated messages. This is why you need to specify the dependency explicitly, as is done in this PR.

Kukanani commented 5 years ago

@mintar's explanation makes sense, thanks for that clarification! Unfortunately I was unable to reproduce, even when testing with a 100% fresh workspace on multiple computers. Ideally we would have someone else who could reproduce the failure. But seeing as it's a pretty safe PR we can move forward.

mintar commented 5 years ago

For some reason, I can usually reproduce this kind of errors with catkin (from catkin_tools), but not with vanilla catkin_make.