-
Notifications
You must be signed in to change notification settings - Fork 124
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 introspection typesupport tests for C/C++ messages #651
Add introspection typesupport tests for C/C++ messages #651
Conversation
@j-rivero @gbiggs this is the HEAD of the work, including all other changes in this repository currently in PRs. I'll add a few more message tests tomorrow. Once all this is in, I'll work on a few service and action tests -- infrastructure shouldn't change much once we've settled with message tests. |
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.
I don't know the rosidl_runtime_*
packages, so I've only looked at the rosidl_typesupport_introspection_*
files.
…ospection_c Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Includes testing infrastructure. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
c30af5c
to
692751e
Compare
@ros-pull-request-builder retest this please |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Windows CI after 6545f13: |
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 is a really great work. I can not find any single obvious error with my level of knowledge. I leave some comments to improve and some clarifications.
rosidl_typesupport_introspection_tests/include/rosidl_typesupport_introspection_tests/api.hpp
Outdated
Show resolved
Hide resolved
rosidl_typesupport_introspection_tests/include/rosidl_typesupport_introspection_tests/api.hpp
Show resolved
Hide resolved
...esupport_introspection_tests/include/rosidl_typesupport_introspection_tests/gtest/macros.hpp
Outdated
Show resolved
Hide resolved
...on_tests/include/rosidl_typesupport_introspection_tests/gtest/message_introspection_test.hpp
Show resolved
Hide resolved
static constexpr const MessageTypeSupportSymbolRecord messages[] = { | ||
MESSAGE_TYPESUPPORT_SYMBOL_RECORD( | ||
rosidl_typesupport_introspection_c, | ||
rosidl_typesupport_introspection_tests, msg, Arrays), |
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.
We are reproducing the same code pattern for the sequence of "Array, BasicTypes, BoundedSequences, ..." in this file. Would be possible to somehow define the sequence in one place and iterate over it?
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.
Not sure I follow you here.
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.
Sorry, let me explain it:
In https://github.com/ros2/rosidl/pull/651/files#diff-31e6f9e3f2b7b5c3a3250552def63bc483cbbcd37ccd184b226445523af638d1R82-R108 we are calling
MESSAGE_TYPESUPPORT_SYMBOL_RECORD(
rosidl_typesupport_introspection_c,
rosidl_typesupport_introspection_tests, msg, $$VALUE$$),
Where $$VALUE$$
is in the list of "Array, BasicTypes, BoundedSequences, ...". Not sure if it would be possible to replace the duplication with a loop somehow.
Just below https://github.com/ros2/rosidl/pull/651/files#diff-31e6f9e3f2b7b5c3a3250552def63bc483cbbcd37ccd184b226445523af638d1R122-R205, for the traits we are doing the same by duplicating:
template<>
struct introspection_traits<rosidl_typesupport_introspection_tests__msg__Arrays>
{
static constexpr const MessageTypeSupportSymbolRecord typesupport =
MESSAGE_TYPESUPPORT_SYMBOL_RECORD(
rosidl_typesupport_introspection_c,
rosidl_typesupport_introspection_tests, msg, $$VALUE$$);
using TypeSupportLibraryT = IntrospectionCTypeSupportTestLibrary;
};
Is not that I have something clear in mind to reduce the duplication, just asking. Feel free to drop if you no easy way.
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.
Oh I see.
Not sure if it would be possible to replace the duplication with a loop somehow.
These are compile time definitions, so looping isn't possible. We could add some variadic macros to recurse though. Duplication will go away but the macros are tricky.
for the traits we are doing the same by duplicating:
Right. Same answer. It is doable with variadic macros.
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.
Thanks for the feedback, proposal was not a blocker for this PR.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
LGTM with green in CI. We would need to document the issue with the actions to take care of it at some point. That we can done in new PRs. |
@j-rivero agreed. In fact, we should probably revisit action testing in this entire repo. |
This patch generalizes the testing infrastructure in #641 to validate introspection APIs for both C and C++ interfaces.