Skip to content

Add tests for Keyed types#568

Merged
mjcarroll merged 9 commits intoros2:rollingfrom
fgallegosalido:feature/keyed-topics-support
Apr 7, 2025
Merged

Add tests for Keyed types#568
mjcarroll merged 9 commits intoros2:rollingfrom
fgallegosalido:feature/keyed-topics-support

Conversation

@fgallegosalido
Copy link
Contributor

@fgallegosalido fgallegosalido commented Mar 31, 2025

This adds support for the messages added by ros2/rcl_interfaces#173 to the test_communication package.

Part of ros2/ros2#1538.

@fgallegosalido fgallegosalido mentioned this pull request Apr 1, 2025
Merged
@MiguelCompany
Added support for KeyedString.
79acdc2 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Added support for NonKeyedWithNestedKey.
7d318df 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Added support for ComplexNestedKey.
69e1e97 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Make test_communication use idl messages instead of msg ones.
1ce36a3 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Skip keyed messages for RMWs other than fastrtps ones.
5298d3c 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Changed copyright year on new files.
24b1c61 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany
Copy link
Contributor

MiguelCompany commented Apr 2, 2025

@fgallegosalido Would you mind rebasing this on top of #542 with:

  1. A commit that adds support for KeyedLong
  2. A commit with the changes in CMakeLists.txt regarding the rmw_implementation validation

@fgallegosalido
Copy link
Contributor Author

fgallegosalido commented Apr 2, 2025 •
edited
Loading

@MiguelCompany I do not have permission to modify the branch feature/rolling/keys from your fork. I'm not sure what you want me to do. Do you want to merge the pull request #542 instead of this one? In that case, it makes sense for you to add the KeyedLong type to your fork.

@MiguelCompany
Copy link
Contributor

MiguelCompany commented Apr 2, 2025

@fgallegosalido The idea is that your branch (which you can modify) stems from the head of my branch, and contains additional commits with your work. This way, your PR will have the commits from both, and we could close my PR in favor of this one, and when this one is merged, we will both count as contributors.

@fgallegosalido
Added tests for KeyedLong type and test interoperability between Fast…
c9b4cec 
…RTPS and Connext RMWs

Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
@fgallegosalido
Copy link
Contributor Author

fgallegosalido commented Apr 2, 2025

@MiguelCompany done. I've added tests for the KeyedLong message and also generate tests for Connext RMW too.

This was referenced Apr 3, 2025
Closed
Closed
@MiguelCompany
Copy link
Contributor

MiguelCompany commented Apr 3, 2025 •
edited
Loading

CI with this repos file:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

fujitatomoya
fujitatomoya approved these changes Apr 5, 2025
View reviewed changes
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

test_communication/CMakeLists.txt
string_ends_with("${interface_file}" ".msg" is_message)
if(is_message AND interface_ns STREQUAL "msg")
string_ends_with("${interface_file}" ".idl" is_idl)
if(is_idl AND interface_ns STREQUAL "msg")
Copy link
Contributor

@fujitatomoya fujitatomoya Apr 4, 2025

Choose a reason for hiding this comment

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

is the same change required for srv to avoid the duplication?

Copy link
Contributor Author

@fgallegosalido fgallegosalido Apr 6, 2025

Choose a reason for hiding this comment

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

I think this probably related to #549, although it doesn't seem to be necessary for now and can be backported later

fgallegosalido and others added 2 commits April 6, 2025 16:07
@fgallegosalido @fujitatomoya
Update test_communication/test/subscribe_key_types.cpp
2a6a1bd 
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
@fgallegosalido @fujitatomoya
Update test_communication/test/subscribe_key_types.hpp
cdf6b7a 
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
@fgallegosalido
Copy link
Contributor Author

fgallegosalido commented Apr 6, 2025

@fujitatomoya changes applied. Let me know of anything else in order to be able to merge.

@MiguelCompany
Copy link
Contributor

MiguelCompany commented Apr 7, 2025

New CI with this repos file:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@mjcarroll mjcarroll merged commit e5c9a0d into ros2:rolling Apr 7, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahcorde ahcorde ahcorde approved these changes

@fujitatomoya fujitatomoya fujitatomoya 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.

5 participants

@fgallegosalido @MiguelCompany @ahcorde @fujitatomoya @mjcarroll