search

ros2 / rmw_implementation

CMake infrastructure and dependencies for rmw implementations
Apache License 2.0
21 stars 47 forks source link

Add rmw_publisher_wait_for_all_acked #188

Closed Barry-Xu-2018 closed 3 years ago

Barry-Xu-2018 commented 3 years ago

Related to ros2/rmw#295

ivanpauno commented 3 years ago

Could you add a test case in https://github.com/ros2/rmw_implementation/tree/master/test_rmw_implementation?

I don't need a test that actually confirms that all messages were acked (which is a bit hard to test), only test basic things: e.g. passing a nullptr publisher returns an error, passing a valid BEST_EFFORT publisher always return immediatly RMW_RET_OK, etc.

Barry-Xu-2018 commented 3 years ago

Could you add a test case in https://github.com/ros2/rmw_implementation/tree/master/test_rmw_implementation?

I don't need a test that actually confirms that all messages were acked (which is a bit hard to test), only test basic things: e.g. passing a nullptr publisher returns an error, passing a valid BEST_EFFORT publisher always return immediatly RMW_RET_OK, etc.

Okay. I add a simple test case (commit: https://github.com/ros2/rmw_implementation/pull/188/commits/cc154d99abe40ba07995248133a98f16cbd94592).
FYI, I add more test cases (such as timeout) in rcl PR ros2/rcl#913).

ivanpauno commented 3 years ago

@ros-pull-request-builder retest this please (this should pass now)

ivanpauno commented 3 years ago

@Barry-Xu-2018 can you check the test failures?

Barry-Xu-2018 commented 3 years ago

@ivanpauno

The failure is related to test_rmw_implementation.TestPublisherUse__rmw_cyclonedds_cpp.wait_for_all_acked_with_best_effort.
It is because ros2/rmw_cyclonedds#294 isn't merged now.

Could you check and merge it ?

ivanpauno commented 3 years ago

Thanks @Barry-Xu-2018, I was sure I merged that one :smile:

Barry-Xu-2018 commented 3 years ago

@ivanpauno

After ros2/rmw_cyclonedds#294 is merged, we should re-execute CI.
Could you help to execute CI again ?

fujitatomoya commented 3 years ago

@ros-pull-request-builder retest this please

CC: @Barry-Xu-2018 @ivanpauno

ivanpauno commented 3 years ago

The PR checker should pass after https://github.com/ros/rosdistro/pull/29924 is merged. No need to run CI again, this is only wrapping new API and adding tests for it.

Thanks for the patience, but merging changes in rmw_* without breaking the rolling builds is a long process ...

ivanpauno commented 3 years ago

@ros-pull-request-builder retest this please

ivanpauno commented 3 years ago

@ros-pull-request-builder retest this please

ivanpauno commented 3 years ago

finally ... :smiley: going in!!