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

rosbag2_cpp: test more than one storage plugin #1196

Merged
merged 4 commits into from
Dec 9, 2022

Conversation

james-rms
Copy link
Contributor

@james-rms james-rms commented Dec 6, 2022

This PR parametrizes rosbag2_cpp unit tests to run using some set of "tested" storage plugins - for now this is just sqlite3 and mcap, but once #1160 is done I expect this would be defined as "the set of default storage plugins".

It also updates rosbag2_storage_mcap to pass those tests. Before this PR the mcap storage plugin reader does not correctly restart iteration from after the last read message.

TODO

@james-rms james-rms requested a review from a team as a code owner December 6, 2022 02:50
@james-rms james-rms requested review from gbiggs and emersonknapp and removed request for a team December 6, 2022 02:50
@james-rms james-rms force-pushed the jrms/test-mcap-rosbag2-cpp branch 7 times, most recently from 18af3f8 to 813d604 Compare December 6, 2022 09:05
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, couple possible suggestions, lmk your thoughts on those

rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/package.xml Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_info.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp Outdated Show resolved Hide resolved
@james-rms james-rms force-pushed the jrms/test-mcap-rosbag2-cpp branch 2 times, most recently from 4c981f4 to 3a87fd8 Compare December 7, 2022 02:46
Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/james-rms/d5d9f03159bffe918986d4b0dde89606/raw/bcc49a4ebbd695559fe6e15dca8538bb2c7aca89/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_test_common mcap_vendor rosbag2_storage_mcap rosbag2_cpp ros2bag rosbag2_transport
TEST args: --packages-above rosbag2_test_common mcap_vendor rosbag2_storage_mcap rosbag2_cpp ros2bag rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11232

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

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.

None yet

3 participants