Added tests for bounded sequences serialization by MiguelCompany · Pull Request #193 · ros2/rmw_implementation

MiguelCompany · 2021-06-08T08:19:31Z

Regression test for ros2/rmw_fastrtps#540

Depends on ros2/rcl_interfaces#123
Depends on ros2/test_interface_files#14

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic

About ros2/test_interface_files#14 and ros2/rcl_interfaces#123, it doesn't seem like test_msgs/msg/UnboundedSequencesNoStrings is used at all. Also, why is test_msgs/msg/BoundedSequencesNoStrings necessary? Why wouldn't test_msgs/msg/BoundedSequences work?

hidmic · 2021-06-08T13:21:43Z

test_rmw_implementation/test/test_serialize_deserialize.cpp

+  test_msgs__msg__BoundedSequencesNoStrings__fini(&input_message);
+  test_msgs__msg__BoundedSequencesNoStrings__fini(&output_message);
+
+  EXPECT_EQ(RMW_RET_OK, rmw_serialized_message_fini(&serialized_message)) <<
+    rmw_get_error_string().str;


@MiguelCompany consider moving these lines to OSRF_TESTING_SCOPE_EXIT blocks.

MiguelCompany · 2021-06-08T15:02:04Z

Why wouldn't test_msgs/msg/BoundedSequences work?

Because it has strings, which are unbounded

it doesn't seem like test_msgs/msg/UnboundedSequencesNoStrings is used at all

I just added it for symmetry

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic

Overall LGTM but for one comment

hidmic · 2021-06-15T20:06:00Z

test_rmw_implementation/test/test_serialize_deserialize.cpp

+  ASSERT_EQ(
+    RMW_RET_OK, rmw_serialized_message_init(
+      &serialized_message, 0lu, &default_allocator)) << rmw_get_error_string().str;
+


@MiguelCompany an OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT block finalizing serialized_message is missing here.

Signed-off-by: Miguel Company <miguelcompany@eProsima.com>

hidmic · 2021-06-16T13:14:41Z

test_rmw_implementation/test/test_serialize_deserialize.cpp


+TEST_F(CLASSNAME(TestSerializeDeserialize, RMW_IMPLEMENTATION), clean_round_trip_for_c_bounded_message) {
+  const rosidl_message_type_support_t * ts{
+    ROSIDL_GET_MSG_TYPE_SUPPORT(test_msgs, msg, BoundedSequencesNoStrings)};


@MiguelCompany hmm, did you mean

Suggested change

ROSIDL_GET_MSG_TYPE_SUPPORT(test_msgs, msg, BoundedSequencesNoStrings)};

ROSIDL_GET_MSG_TYPE_SUPPORT(test_msgs, msg, BoundedPlainSequences)};

? Same elsewhere.

Ups! Fixed on f776a16

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic · 2021-06-16T14:29:19Z

CI up to rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp, rcl_interfaces and test_rmw_implementation:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2021-06-16T14:59:48Z

test_rmw_implementation/test/test_serialize_deserialize.cpp


 #include <gtest/gtest.h>

+#include "osrf_testing_tools_cpp/scope_exit.hpp"


It looks like we need an ament_target_dependencies on osrf_testing_tools_cpp for this test now.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic · 2021-06-17T13:02:35Z

Once again, CI up to rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp, rosidl_typesupport_fastrtps, rcl_interfaces, and test_rmw_implementation:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2021-06-18T14:55:40Z

@hidmic I fixed the linter issues. Would you mind running CI again?

hidmic · 2021-06-18T17:02:32Z

Third's the charm hopefully:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2021-06-22T07:30:42Z

@hidmic Last linter issue fixed.

hidmic · 2021-06-22T12:43:10Z

~~Ok, let's start small this time~~ Alright, let's go.

Linux
Linux-aarch64
macOS
Windows

fujitatomoya · 2021-07-01T23:20:00Z

test_rmw_implementation/test/test_serialize_deserialize.cpp

+  ASSERT_EQ(
+    RMW_RET_OK, rmw_serialized_message_init(
+      &serialized_message, 0lu, &default_allocator)) << rmw_get_error_string().str;
+


nit, but i would also use OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT.

Suggested change

OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(

{

EXPECT_EQ(

RMW_RET_OK, rmw_serialized_message_fini(

&serialized_message)) << rmw_get_error_string().str;

});

@MiguelCompany nit: check block indentation

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic · 2021-07-30T12:10:47Z

test_rmw_implementation/test/test_serialize_deserialize.cpp

+  CLASSNAME(
+    TestSerializeDeserialize,
+    RMW_IMPLEMENTATION), clean_round_trip_for_cpp_bounded_message) {
+  using test_message = test_msgs::msg::BoundedPlainSequences;


@MiguelCompany nit: use camel case for C++ type names, as per GSG.

hidmic · 2021-07-30T12:12:27Z

MacOS CI, one more time:

macOS

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic · 2021-08-04T13:18:48Z

CI again as it's been a while:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2021-08-04T18:31:03Z

Almost there. We'll need to release a few packages for the Rpr job to pass here.

hidmic · 2021-08-09T22:00:49Z

@ros-pull-request-builder retest this please

hidmic · 2021-08-10T13:10:45Z

@ros-pull-request-builder retest this please

hidmic · 2021-08-10T13:17:34Z

Alright, finally green. Going in!

Added tests for bounded sequences serialization.

6340971

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany mentioned this pull request Jun 8, 2021

Correctly recalculate serialized size on bounded sequences ros2/rmw_fastrtps#540

Merged

hidmic reviewed Jun 8, 2021

View reviewed changes

MiguelCompany force-pushed the test-bounded-serialization branch 2 times, most recently from f2851b5 to 72223af Compare June 8, 2021 15:20

Using OSRF_TESTING_SCOPE_EXIT

f2031b0

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the test-bounded-serialization branch from 72223af to f2031b0 Compare June 8, 2021 15:21

hidmic reviewed Jun 15, 2021

View reviewed changes

Add missing OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT

5f4182d

Signed-off-by: Miguel Company <miguelcompany@eProsima.com>

hidmic reviewed Jun 16, 2021

View reviewed changes

Update message name

f776a16

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the test-bounded-serialization branch from 6b5d8f6 to f776a16 Compare June 16, 2021 13:33

hidmic approved these changes Jun 16, 2021

View reviewed changes

clalancette reviewed Jun 16, 2021

View reviewed changes

Add dependency on osrf_testing_tools_cpp

2712e41

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Linters.

74bc767

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Fixed line length.

b752fc7

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

fujitatomoya reviewed Jul 1, 2021

View reviewed changes

fujitatomoya approved these changes Jul 1, 2021

View reviewed changes

MiguelCompany added 2 commits July 2, 2021 07:51

Using scope exit for message fini

84d3637

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Fixing test execution on MacOS

e314708

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic reviewed Jul 30, 2021

View reviewed changes

Linters and style

cfffa5e

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

hidmic mentioned this pull request Aug 6, 2021

Add tests for BoundedPlainSequences ros2/system_tests#481

Merged

hidmic merged commit 0fcac11 into ros2:master Aug 10, 2021

sloretz mentioned this pull request Sep 14, 2021

remove extra parenthesis, closes #143 ros2/rosidl_python#144

Closed

	ROSIDL_GET_MSG_TYPE_SUPPORT(test_msgs, msg, BoundedSequencesNoStrings)};
	ROSIDL_GET_MSG_TYPE_SUPPORT(test_msgs, msg, BoundedPlainSequences)};


		#include <gtest/gtest.h>

		#include "osrf_testing_tools_cpp/scope_exit.hpp"

Conversation

MiguelCompany commented Jun 8, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiguelCompany commented Jun 8, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic commented Jun 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiguelCompany commented Jun 18, 2021

Uh oh!

hidmic commented Jun 18, 2021

Uh oh!

MiguelCompany commented Jun 22, 2021

Uh oh!

hidmic commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic commented Jul 30, 2021

Uh oh!

hidmic commented Aug 4, 2021

Uh oh!

hidmic commented Aug 4, 2021

Uh oh!

hidmic commented Aug 9, 2021

Uh oh!

hidmic commented Aug 10, 2021

Uh oh!

hidmic commented Aug 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hidmic commented Jun 17, 2021 •

edited

Loading

hidmic commented Jun 22, 2021 •

edited

Loading