Skip to content
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

Regression test for FastCDR typesupport. #266

Closed
wants to merge 1 commit into from

Conversation

mjcarroll
Copy link
Member

Adds a regression test for the the bug we saw in Bionic with FastRTPS.

Connects to: ros2/rclcpp#464

@mjcarroll mjcarroll added the in progress Actively being worked on (Kanban column) label May 1, 2018
@mjcarroll
Copy link
Member Author

mjcarroll commented May 1, 2018

@mikaelarguedas I don't know if this is the right place for this to live. It's really only testing FastCDR typesupport, but it also depends on rcl_interfaces and rclcpp.

@mikaelarguedas
Copy link
Member

mikaelarguedas commented May 1, 2018

Location

As this is a very specific test:
testing only C++ typesupport introspection with Fast-CDR, I think we should make it explicit:

  • find another place for it
  • rename the files and test targets to reflect that
  • enforce in CMake that this is built and tested only if Fast-RTPS is present and enabled.

Then the right place to land it will depend on how many dependencies we can get rid of. I'm fine leaving it here for now (in this repository but as in a new package to clarify dependencies and be able to move it somewhere else easily).

The dependency on rcl_interfaces is not a big deal. Ideally it could be removed by using or adding a message with the same characteristics in test_msgs.

I'm surprised we need rclcpp though, I would have expected the c++ typesupport to be enough to reproduce this. Something like: instanciate message, get the relevant c++ type support, serialize/deserialize (something similar to what @Karsten1987 is working on at https://github.com/ros2/system_tests/blob/expose_cdr/test_communication/test/test_message_serialization.cpp).

I'm not sure it's worth adding a test for this as we already had ~60 tests failing because of this. But having more specific tests to pin the exact issue is always valuable.

What to test

Were you able to reproduce the issue with a simple message type support (redo of this comment ros2/rclcpp#464 (comment))? as the bug seems to be a generic typesupport issue I would expect that to be the case.

I would expect that you can reproduce the same failure by pacssin an unbounded array with strings around.
Can you reproduce this failure without the fix ?
and confirm that the test pass with your fix?

This will confirm that we already have messages in test_messages able to reproduce the issue (so no need to depend on rcl_interfaces) and that we can get rid of Service typesupport to trim the test down to a simple serialisation/deserialization of a message.

Then the question is: "as we already had ~60 tests failing because of this particular issue, do we need to add such test to the suite?". I think that's good to have a simplistic test to pin down the issue, on the other hand if this is only to ctch regressions, we will catch regression with our current test suite.

@mjcarroll mjcarroll force-pushed the bugfix/bionic_segfault branch from 77b97c3 to 63f0544 Compare May 1, 2018 15:24
@mjcarroll
Copy link
Member Author

I would be good with dropping the test entirely, this is essentially what I needed to completely isolate the issue, but probably doesn't have enough coverage to justify standing on it's own.

@mjcarroll
Copy link
Member Author

Also, with another look, it doesn't look like it actually depends on rclcpp, that was leftover from my exploration.

@mjcarroll
Copy link
Member Author

I'm going to close this, I don't think it makes sense to add as a test, it's moreso just a reference.

@mjcarroll mjcarroll closed this May 1, 2018
@mjcarroll mjcarroll removed the in progress Actively being worked on (Kanban column) label May 1, 2018
@mjcarroll mjcarroll deleted the bugfix/bionic_segfault branch May 1, 2018 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants