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.

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

Add ROS test with CI. #350

Merged
merged 12 commits into from
Dec 5, 2024
Merged

Add ROS test with CI. #350

merged 12 commits into from
Dec 5, 2024

Conversation

evshary
Copy link
Contributor

@evshary evshary commented Dec 3, 2024

In our current CI, there is no test on the transformation between ROS and Zenoh.
This makes our development less efficient and makes it impossible to ensure quality when it comes to more and more combinations of tests.
For example, how to make sure our tests can work in both Humble and Jazzy? When it comes to different environmental variables, how to know it won't break the backward compatibility?
The PR is a trial to see how it works by using r2r.

Copy link

github-actions bot commented Dec 3, 2024

PR missing one of the required labels: {'internal', 'documentation', 'breaking-change', 'enhancement', 'bug', 'new feature', 'dependencies'}

@evshary evshary added the internal Changes not included in the changelog label Dec 3, 2024
@evshary
Copy link
Contributor Author

evshary commented Dec 3, 2024

The failure is related to this #351

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary evshary force-pushed the add_ros_test branch 2 times, most recently from 9b99923 to b030d70 Compare December 4, 2024 02:34
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary evshary marked this pull request as ready for review December 4, 2024 09:49
@evshary
Copy link
Contributor Author

evshary commented Dec 4, 2024

@JEnoch I can test pub/sub between ROS and Zenoh now. Tested both in Humble and Jazzy.
Please let me know if you have any comments.
Also, do you want it merged first, or can I add the service and action test in the same PR?

Copy link
Member

@JEnoch JEnoch left a comment

Choose a reason for hiding this comment

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

I think the setting of RMW_IMPLEMENTATION=rmw_cyclonedds_cpp should be hard-coded in the tests. Otherwise, r2r picks the default RMW which won't fully work with the bridge if not CycloneDDS.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
zenoh-plugin-ros2dds/Cargo.toml Outdated Show resolved Hide resolved
@JEnoch
Copy link
Member

JEnoch commented Dec 4, 2024

Also, do you want it merged first, or can I add the service and action test in the same PR?

I think it's fine to have just pub/sub in a first PR, and to add others later.

But as most of users are using the plugin to bridge 2 distinct DDS domains (different hosts without DDS comms or different domains), I would also like to see such tests.
Do you think it's feasible in the same process ? (2 plugins with different configs, and 2 Nodes using different domains)

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary
Copy link
Contributor Author

evshary commented Dec 5, 2024

Do you think it's feasible in the same process ? (2 plugins with different configs, and 2 Nodes using different domains)

I don't think so. I looked into the ROS code. Indeed, we can use rclcpp::init(argc, argv, options) and put domain ID inside options, but the setting is global, which means we can't have two different domain ID in the same process. Not to mention, r2r doesn't provide such API to do the configuration.
I think a better way here is to put these kinds of tests into the zenoh-test, which is easier to run multiple processes. We've already have something similar in zenoh-bridge-dds. We can do the same thing for ros2dds.

@JEnoch JEnoch enabled auto-merge (squash) December 5, 2024 17:43
@JEnoch JEnoch merged commit f4990b1 into eclipse-zenoh:main Dec 5, 2024
8 checks passed
@evshary evshary deleted the add_ros_test branch December 6, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes not included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants