Add publisher/subscription matched count API test coverage. #119
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
Matching pub/sub tests are not passing for |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic
changed the title
| } | ||
|
|
||
| TEST_F( | ||
| CLASSNAME(TestPublisherUse, RMW_IMPLEMENTATION), |
There was a problem hiding this comment.
This alignment is strange since it aligns with the code. Is there not a way to adjust the indentation, or move the curly brace to a new line?
This happens elsewhere as well.
There was a problem hiding this comment.
That's uncrustify... I ended up shortening the test case name in da38188.
| */ | ||
| #define SLEEP_AND_RETRY_UNTIL(delay, timeout) for ( \ | ||
| auto loop_start_time = std::chrono::steady_clock::now(), \ | ||
| iteration_start_time = loop_start_time; \ |
There was a problem hiding this comment.
Why is iteration_start_time needed? Why can't you just use std::this_thread::sleep_for(delay) instead of sleep_until?
There was a problem hiding this comment.
To avoid the time it takes to actually execute the loop code from adding up. Also, std::this_thread::sleep_for makes no guarantees as to which clock is used.
There was a problem hiding this comment.
I guess I mean something like:
#define SLEEP_AND_RETRY_UNTIL(delay, timeout) \
for (auto start = std::chrono::steady_clock::now(); \
std::chrono::steady_clock::now() - start < timeout; \
std::this_thread::sleep_for(delay))If you'd rather leave it as is, then I'm fine with it. It just seems like your use case doesn't require such precision.
There was a problem hiding this comment.
I didn't want to lose retries due to non-negligible execution times, but it is true that these timeouts are pretty arbitrary anyways.
Sure, why not. See 4d85be9.
| @@ -34,6 +34,7 @@ if(BUILD_TESTING) | |||
| ) | |||
| target_compile_definitions(test_init_shutdown${target_suffix} | |||
| PUBLIC "RMW_IMPLEMENTATION=${rmw_implementation}") | |||
|
|
|||
There was a problem hiding this comment.
This added line can be removed
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
CI up to
|
|
@ros-pull-request-builder retest this please |
1 similar comment
|
@ros-pull-request-builder retest this please |
|
Finally! PR job's green. Merging. |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Connected to ros2/rmw#262. Depends on ros2/rmw_fastrtps#424, ros2/rmw_connext#451, and ros2/rmw_cyclonedds#223.