Trait tests for generated actions

[mjcarroll]

Adds test coverage for the changes in ros2/rosidl#412.

Currently, action_msgs are required in order to be able to generate action, so the tests are here to prevent any sort of circular dependency in rosidl_generator_cpp.

[dirk-thomas]

action_msgs are required in order to be able to generate action

Can you point to what exactly requires action_msgs to generate actions? Afaik it only requires that package when you want to include the action__struct.hpp which is not necessary to use/test the traits.

[jacobperron]

Can you point to what exactly requires action_msgs to generate actions? Afaik it only requires that package when you want to include the action__struct.hpp which is not necessary to use/test the traits.

When rosidl_generate_interfaces is called:

https://github.com/ros2/rosidl/blob/07a2ab5c452c462ae067769b752a1811cfb3612e/rosidl_cmake/cmake/rosidl_generate_interfaces.cmake#L145-L151

[dirk-thomas]

Wouldn't an exec dependency be sufficient? What is it needed for at build time?

[mjcarroll]

Maybe I'm unclear, but doesn't message (and action) generation happen during the build phase?

Edit: Or are you proposing to modify the rosidl_cmake?

[dirk-thomas]

Currently the CMake code calls find_package(action_msgs) - the question is why? I don't see any reason for it. When removed (if not necessary) a message should only require an exec_depend to make sure the recursively needed headers are available.

[jacobperron]

We are referencing action_msgs interfaces when generating code for actions:

https://github.com/ros2/rosidl/blob/07a2ab5c452c462ae067769b752a1811cfb3612e/rosidl_generator_cpp/resource/action__struct.hpp.em#L91-L94

AFAICT, this is the only place, so maybe it's easy to avoid.

[mjcarroll]

What happens in the case when you need to generate an action and then immediately use it in the same package? In that case, it seems that action_msgs to need to be a build_depend. Is that something we want to leave up to the user or should it be handled by the generator itself?

[dirk-thomas]

What happens in the case when you need to generate an action and then immediately use it in the same package? In that case, it seems that action_msgs to need to be a build_depend.

Yes, if a package does want to do that it needs to declare that extra dependency.

[jacobperron]

I tried removing action_msgs as a dependency from rosidl_cmake (ie. the find_package call) and the action_msgs includes from the action struct template. But, action_msgsis transitively bringing in a needed dependency to unique_identifier_msgs. When I try to build an action, includes are added for unique_identfier_msgs, e.g.:

/home/jacob/ws/latest_ws/build/test_msgs/rosidl_generator_cpp/test_msgs/action/fibonacci__struct.hpp:356:10: fatal error: unique_identifier_msgs/msg/uuid__struct.hpp: No such file or directory
 #include "unique_identifier_msgs/msg/uuid__struct.hpp"

Maybe we could add a special case for them in the template? I'll try for a little longer to see if it can be done.

[jacobperron]

LGTM, just some nitpicks

[jacobperron]

nit: actionlib refers to the ROS 1 implementation.

Suggested change

	// Test traits on some of the internal implementation of actionlib
	// Test traits on some of the internal implementation of actions

[jacobperron]

What about adding some ASSERT_FALSE cases for completeness (like in the previous test)?

[jacobperron]

nit: using EXPECT_* instead, we'd see multiple failures instead of just the first to fail.