-
Notifications
You must be signed in to change notification settings - Fork 48
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 serialize/deserialize API test coverage. #118
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor comments
ASSERT_TRUE(test_msgs__msg__BasicTypes__init(&input_message)); | ||
rcutils_allocator_t failing_allocator = get_failing_allocator(); | ||
rmw_serialized_message_t serialized_message = rmw_get_zero_initialized_serialized_message(); | ||
ASSERT_EQ(RMW_RET_OK, rmw_serialized_message_init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing fini here and below
rcutils_allocator_t failing_allocator = get_failing_allocator(); | ||
rmw_serialized_message_t serialized_message = rmw_get_zero_initialized_serialized_message(); | ||
ASSERT_EQ(RMW_RET_OK, rmw_serialized_message_init( | ||
&serialized_message, 0lu, &failing_allocator)) << rmw_get_error_string().str; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this call fail if buffer capacity weren't zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would. Which makes me think we should have separate set of test for rmw_serialized_message_t
API, regardless of it being currently an alias for rcutils_uint8_array_t
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, they do but not here. Better in rmw
.
ASSERT_EQ(RMW_RET_OK, rmw_serialized_message_init( | ||
&serialized_message, 0lu, &default_allocator)) << rmw_get_error_string().str; | ||
|
||
input_message.bool_value = !output_message.bool_value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe would be helpful adding a comment here to note those statements are to make them different, at first I was trying to see if the "-1" was to expect some sort of overflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
This reverts commit 5690348. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
74d4198
to
6ca224b
Compare
Hmm this is curious, I cannot reproduce that Rolling PR job failure locally. |
@ros-pull-request-builder retest this please. |
1 similar comment
@ros-pull-request-builder retest this please. |
Uh, that test failure's still there. |
Let's try again with ros/rosdistro#26184 merged. @ros-pull-request-builder retest this please! |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Connected to ros2/rmw#258.