Set envars to run tests with rmw_zenoh_cpp with multicast discovery#1218
Set envars to run tests with rmw_zenoh_cpp with multicast discovery#1218
Conversation
Signed-off-by: Yadunund <yadunund@gmail.com>
fujitatomoya
left a comment
There was a problem hiding this comment.
just out of curiosity, the default discovery behavior of zenoh is multicast discovery?
i mean that, if that is not default, doing all the tests for zenoh with non-default configuration would be kinda strange, just to pass the tests...? because that is actually used in default configuration (probably with zenoh router configuration), which is supposed to be tested by CI/CD pipeline in the 1st place to make sure that all the tests are pass? i understand the situation and development for zenoh, but it feels like missing the core purpose of CI/CD test for me...
just sharing my thought, not blocking this PR.
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: yadunund <yadunund@gmail.com>
|
@fujitatomoya, that's a very fair criticism. This PR effectively tests in CI with a different configuration/topology than the default. Our goal is to enable starting the router in CI. @cottsay has been working on a middleware-agnostic way to launch processes during isolated tests. These processes could include the Zenoh router or the FastDDS discovery server. However, we may not have these features in place before the RMW freeze (but aim to before overall code freeze). Therefore, we're resorting to changing the CI configuration to demonstrate that rmw_zenoh can pass these tests. (Locally, these tests pass with the default configuration and router.) We intend to revert this change as soon as the necessary test features are available. Happy to discuss this further. cc: @mjcarroll |
|
@Yadunund i did not mean to block this PR, so if that is the plan, i think that is okay to merge this work-around for now.
this is so true. actually Fast-DDS also introduces discovery server in ros2 documentation, so it can be also tested with this process. that is very good enhancement for the test cases. |
|
When running the tests locally on Ubuntu I'm getting errors in
98: /home/ahcorde/ros2_rolling/src/ros2/rcl/rcl/test/rcl/test_graph.cpp:1078: Failure
98: Expected: (attempts) <= (max_attempts), actual: 101 vs 100
98: Unable to attain all required nodes
121: [service_fixture-1] [ERROR] [1742575407.925265153] [rcl]: Service never became ready
121: [client_fixture-2] [ERROR] [1742575407.932023887] [rcl]: Server never became available |
|
@Yadunund my bad, my |
|
@fujitatomoya sounds good. @ahcorde thanks for testing. we will be opening a few more PRs like this across various core repos. Let's coordinate their merge once all the PRs have been reviewed / pass CI. So let's not merge this yet. |
|
Gist (default ros2.repos with changes from this branch): https://gist.githubusercontent.com/Yadunund/c4074b12ea5bdedcddae7b60f60c34bc/raw/a0b16854e15efcc60c8551e816ae5881102485ec/ros2_ci.repos
Gist (with rmw_zenoh and changes from this branch) : https://raw.githubusercontent.com/ros2/ros2/3f873e9c5a79ce3b47f70199708b21c07007cd7d/ros2.repos
|
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-pmc-minutes-for-march-25-2025/42807/1 |
See ros2/rmw_zenoh#567
To test this PR, we can run the usual CI jobs to show there are no regressions and additionally run a CI job with a copy of the ros2.repos file from ros2/ros2#1649 with CI Scripts branch set to yadu/cargo