update tests to use separated action types #340

dirk-thomas · 2018-11-24T17:29:08Z

jacobperron · 2019-02-14T18:53:56Z

rcl_action/test/rcl_action/test_action_communication.cpp


  // Initialize goal request
-  init_test_uuid0(outgoing_goal_request.action_goal_id.uuid);
-  outgoing_goal_request.order = 10;
+  init_test_uuid0(outgoing_goal_request.goal_id);


I would have expected this to be outgoing_goal_request.goal_id.uuid, as outgoing_goal_request.action_goal_id was of type unique_identifer_msgs/UUID.

https://github.com/ros2/rosidl/blob/7c5c7d3af98fb8f5fd8ae9112ee278d21f899df1/rosidl_adapter/rosidl_adapter/parser.py#L888

The referenced change has landed a long time after the IDL branches were created. So atm this branch uses a uint8[16] (see https://github.com/ros2/rosidl/blob/6c44fd023b5503e174bafaeee2544b063fd654e6/rosidl_parser/rosidl_parser/definition.py#L505).

This certainly needs to be updated to keep using the uuid message.

Sorry for jumping into the discussion, and this is absolutely tangential to the IDL work, but IMHO only a small portion of the implementation should need to know about the uuid field in a goal_id, while the rest could just use goal_id as an opaque entity.

I agree. But I believe in this case the code should know about the UUID (or uint8[16]) since it is setting it to a known value.

Currently, client libraries rely on the knowledge that a goal ID is a UUID for tracking goal handles, e.g.

https://github.com/ros2/rclcpp/blob/10d7b7c72bc243e4e6da0546f9d17e39460df55c/rclcpp_action/include/rclcpp_action/types.hpp#L32

https://github.com/ros2/rclcpp/blob/10d7b7c72bc243e4e6da0546f9d17e39460df55c/rclcpp_action/include/rclcpp_action/client.hpp#L321

IMO ideally, we could limit the heavy usage of UUIDs to rcl_action and try to keep most of the goal tracking logic abstracted there. I've created an feature request as a better venue for this discussion: #389

Significant changes since initial review

jacobperron

LGTM with green CI

Increasing version of package(s) in repository `rcl` to `0.7.0-3`: - upstream repository: https://github.com/ros2/rcl.git - release repository: https://github.com/ros2-gbp/rcl-release.git - file: `dashing/distribution.yaml` - bloom version: `0.8.0` - previous version for package: `0.7.0-2` ## rcl ``` * Added more test cases for graph API + fix bug. (#404 <ros2/rcl#404>) * Fixed missing include. (#413 <ros2/rcl#413>) * Updated to use pedantic. (#412 <ros2/rcl#412>) * Added function to get publisher actual qos settings. (#406 <ros2/rcl#406>) * Refactored graph API docs. (#401 <ros2/rcl#401>) * Updated to use ament_target_dependencies where possible. (#400 <ros2/rcl#400>) * Fixed regression around fully qualified node name. (#402 <ros2/rcl#402>) * Added function rcl_names_and_types_init. (#403 <ros2/rcl#403>) * Fixed uninitialize sequence number of client. (#395 <ros2/rcl#395>) * Added launch along with launch_testing as test dependencies. (#393 <ros2/rcl#393>) * Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>) * Updated to split test_token to avoid compiler note. (#392 <ros2/rcl#392>) * Dropped legacy launch API usage. (#387 <ros2/rcl#387>) * Improved security directory lookup. (#332 <ros2/rcl#332>) * Enforce non-null argv values on rcl_init(). (#388 <ros2/rcl#388>) * Removed incorrect argument documentation. (#361 <ros2/rcl#361>) * Changed error to warning for multiple loggers. (#384 <ros2/rcl#384>) * Added rcl_node_get_fully_qualified_name. (#255 <ros2/rcl#255>) * Updated rcl_remap_t to use the PIMPL pattern. (#377 <ros2/rcl#377>) * Fixed documentation typo. (#376 <ros2/rcl#376>) * Removed test circumvention now that a bug is fixed in rmw_opensplice. (#368 <ros2/rcl#368>) * Updated to pass context to wait set, and fini rmw context. (#373 <ros2/rcl#373>) * Updated to publish logs to Rosout. (#350 <ros2/rcl#350>) * Contributors: AAlon, Dirk Thomas, Jacob Perron, M. M, Michael Carroll, Michel Hidalgo, Mikael Arguedas, Nick Burek, RARvolt, Ross Desmond, Sachin Suresh Bhat, Shane Loretz, William Woodall, ivanpauno ``` ## rcl_action ``` * Added Action graph API (#411 <ros2/rcl#411>) * Updated to use ament_target_dependencies where possible. (#400 <ros2/rcl#400>) * Fixed typo in Doxyfile. (#398 <ros2/rcl#398>) * Updated tests to use separated action types. (#340 <ros2/rcl#340>) * Fixed minor documentation issues. (#397 <ros2/rcl#397>) * Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>) * Fixed rcl_action documentation. (#380 <ros2/rcl#380>) * Removed now unused test executable . (#382 <ros2/rcl#382>) * Removed unused action server option 'clock_type'. (#382 <ros2/rcl#382>) * Set error message when there is an invalid goal transition. (#382 <ros2/rcl#382>) * Updated to pass context to wait set, and fini rmw context (#373 <ros2/rcl#373>) * Contributors: Dirk Thomas, Jacob Perron, Sachin Suresh Bhat, William Woodall, ivanpauno ``` ## rcl_lifecycle ``` * Updated to use ament_target_dependencies where possible. (#400 <ros2/rcl#400>) * Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>) * Contributors: Sachin Suresh Bhat, ivanpauno ``` ## rcl_yaml_param_parser ``` * Corrected bool reading from yaml files. (#415 <ros2/rcl#415>) * Added launch along with launch_testing as test dependencies. (#393 <ros2/rcl#393>) * Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>) * Contributors: Michel Hidalgo, Sachin Suresh Bhat, ivanpauno ```

Increasing version of package(s) in repository `rcl` to `0.7.0-3`: - upstream repository: https://github.com/ros2/rcl.git - release repository: https://github.com/ros2-gbp/rcl-release.git - file: `dashing/distribution.yaml` - bloom version: `0.8.0` - previous version for package: `0.7.0-2` ## rcl ``` * Added more test cases for graph API + fix bug. (ros#404 <ros2/rcl#404>) * Fixed missing include. (ros#413 <ros2/rcl#413>) * Updated to use pedantic. (ros#412 <ros2/rcl#412>) * Added function to get publisher actual qos settings. (ros#406 <ros2/rcl#406>) * Refactored graph API docs. (ros#401 <ros2/rcl#401>) * Updated to use ament_target_dependencies where possible. (ros#400 <ros2/rcl#400>) * Fixed regression around fully qualified node name. (ros#402 <ros2/rcl#402>) * Added function rcl_names_and_types_init. (ros#403 <ros2/rcl#403>) * Fixed uninitialize sequence number of client. (ros#395 <ros2/rcl#395>) * Added launch along with launch_testing as test dependencies. (ros#393 <ros2/rcl#393>) * Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>) * Updated to split test_token to avoid compiler note. (ros#392 <ros2/rcl#392>) * Dropped legacy launch API usage. (ros#387 <ros2/rcl#387>) * Improved security directory lookup. (ros#332 <ros2/rcl#332>) * Enforce non-null argv values on rcl_init(). (ros#388 <ros2/rcl#388>) * Removed incorrect argument documentation. (ros#361 <ros2/rcl#361>) * Changed error to warning for multiple loggers. (ros#384 <ros2/rcl#384>) * Added rcl_node_get_fully_qualified_name. (ros#255 <ros2/rcl#255>) * Updated rcl_remap_t to use the PIMPL pattern. (ros#377 <ros2/rcl#377>) * Fixed documentation typo. (ros#376 <ros2/rcl#376>) * Removed test circumvention now that a bug is fixed in rmw_opensplice. (ros#368 <ros2/rcl#368>) * Updated to pass context to wait set, and fini rmw context. (ros#373 <ros2/rcl#373>) * Updated to publish logs to Rosout. (ros#350 <ros2/rcl#350>) * Contributors: AAlon, Dirk Thomas, Jacob Perron, M. M, Michael Carroll, Michel Hidalgo, Mikael Arguedas, Nick Burek, RARvolt, Ross Desmond, Sachin Suresh Bhat, Shane Loretz, William Woodall, ivanpauno ``` ## rcl_action ``` * Added Action graph API (ros#411 <ros2/rcl#411>) * Updated to use ament_target_dependencies where possible. (ros#400 <ros2/rcl#400>) * Fixed typo in Doxyfile. (ros#398 <ros2/rcl#398>) * Updated tests to use separated action types. (ros#340 <ros2/rcl#340>) * Fixed minor documentation issues. (ros#397 <ros2/rcl#397>) * Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>) * Fixed rcl_action documentation. (ros#380 <ros2/rcl#380>) * Removed now unused test executable . (ros#382 <ros2/rcl#382>) * Removed unused action server option 'clock_type'. (ros#382 <ros2/rcl#382>) * Set error message when there is an invalid goal transition. (ros#382 <ros2/rcl#382>) * Updated to pass context to wait set, and fini rmw context (ros#373 <ros2/rcl#373>) * Contributors: Dirk Thomas, Jacob Perron, Sachin Suresh Bhat, William Woodall, ivanpauno ``` ## rcl_lifecycle ``` * Updated to use ament_target_dependencies where possible. (ros#400 <ros2/rcl#400>) * Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>) * Contributors: Sachin Suresh Bhat, ivanpauno ``` ## rcl_yaml_param_parser ``` * Corrected bool reading from yaml files. (ros#415 <ros2/rcl#415>) * Added launch along with launch_testing as test dependencies. (ros#393 <ros2/rcl#393>) * Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>) * Contributors: Michel Hidalgo, Sachin Suresh Bhat, ivanpauno ```

dirk-thomas added the in progress Actively being worked on (Kanban column) label Nov 24, 2018

dirk-thomas self-assigned this Nov 24, 2018

dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 26, 2018

dirk-thomas force-pushed the idl-stage-7 branch 2 times, most recently from 31dc9ab to 9b32a2a Compare December 1, 2018 02:31

jacobperron previously approved these changes Dec 4, 2018

View reviewed changes

dirk-thomas force-pushed the idl-stage-7 branch from 9b32a2a to 7870544 Compare December 4, 2018 21:24

dirk-thomas force-pushed the idl-stage-7 branch from 4ae4fa4 to 60439e2 Compare February 5, 2019 21:12

dirk-thomas mentioned this pull request Feb 12, 2019

Migrate to IDL ros2/rosidl#346

Closed

8 tasks

dirk-thomas force-pushed the idl-stage-7 branch from 16b93d7 to d77b0be Compare February 12, 2019 21:06

jacobperron reviewed Feb 14, 2019

View reviewed changes

dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Feb 15, 2019

jacobperron approved these changes Feb 19, 2019

View reviewed changes

dirk-thomas changed the title ~~update test to IDL-based action generation~~ update tests to use separated action types Feb 19, 2019

dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 21, 2019

dirk-thomas force-pushed the idl-stage-7 branch 2 times, most recently from f94feb4 to 4b89cf4 Compare March 4, 2019 18:14

dirk-thomas force-pushed the idl-stage-7 branch from 4b89cf4 to 6591343 Compare March 7, 2019 23:05

dirk-thomas added 8 commits March 9, 2019 21:14

update test to IDL-based action generation

a8df5b5

update test to IDL-based action generation

82d0f0a

fix merge conflict

4ce3dc0

update new tests

9b8aea9

update action types

a6ed604

match renamed action types

41eb88f

style fix

cc4822b

update types using unique_identifier_msgs

73f2a5b

dirk-thomas force-pushed the idl-stage-7 branch from 6591343 to 73f2a5b Compare March 10, 2019 05:15

dirk-thomas merged commit 756b065 into master Mar 12, 2019

dirk-thomas deleted the idl-stage-7 branch March 12, 2019 04:12

dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update tests to use separated action types #340

update tests to use separated action types #340

dirk-thomas commented Nov 24, 2018 •

edited

Loading

jacobperron Feb 14, 2019

dirk-thomas Feb 14, 2019

hidmic Feb 15, 2019

jacobperron Feb 15, 2019

jacobperron left a comment

update tests to use separated action types #340

update tests to use separated action types #340

Conversation

dirk-thomas commented Nov 24, 2018 • edited Loading

jacobperron Feb 14, 2019

Choose a reason for hiding this comment

dirk-thomas Feb 14, 2019

Choose a reason for hiding this comment

hidmic Feb 15, 2019

Choose a reason for hiding this comment

jacobperron Feb 15, 2019

Choose a reason for hiding this comment

jacobperron left a comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 24, 2018 •

edited

Loading