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

Make rosbag2_tests agnostic to storage implementation #1192

Merged
merged 7 commits into from
Dec 3, 2022

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Dec 1, 2022

Closes #1112

Remove all Sqlite3-specific logic from rosbag2_tests, including language around "database"/"db", and explicit usage of the ".db3" storage file extension.

Parameterize the recording tests so that mcap can be easily slotted in to be tested as well.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/rosbag2-tests-nosql branch from d147011 to 5a2ec80 Compare December 1, 2022 23:46
@emersonknapp emersonknapp marked this pull request as ready for review December 1, 2022 23:46
@emersonknapp emersonknapp requested a review from a team as a code owner December 1, 2022 23:46
@emersonknapp emersonknapp requested review from hidmic and james-rms and removed request for a team December 1, 2022 23:46
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.

@emersonknapp Thanks for PR and cleanups for Windows specific.
I am curious if we already can include "mcap" in test parametrization for test_rosbag2_record_end_to_end?

Perhaps as follow up it would be nice to parametrize test_rosbag2_info_end_to_end and add test for checking that we can readout metadata from storage when yaml file not present.

Also the same true for the ReindexTestFixture::test_multiple_files
Will need to parametrize it and add content with multiple mcap files to the /resources/reindex_test_bags/multiple_files

@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/6d3f36d5ea849e932b244cbee4c5f6ae/raw/7f0e81b3863e85c30210e05dab7f0cbb287608b0/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_tests
TEST args: --packages-above rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11205

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

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/rosbag2-tests-nosql branch from 82df0de to 84e3273 Compare December 2, 2022 22:37
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Dec 2, 2022

Was missing a header for the windows build

Gist: https://gist.githubusercontent.com/emersonknapp/d4893e833340f960b9dca832267c3af7/raw/7f0e81b3863e85c30210e05dab7f0cbb287608b0/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_tests
TEST args: --packages-above rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11207

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

@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@MichaelOrlov MichaelOrlov merged commit 3c5bb87 into rolling Dec 3, 2022
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/rosbag2-tests-nosql branch December 3, 2022 01:36
ricardo-manriquez pushed a commit to ricardo-manriquez/rosbag2 that referenced this pull request Dec 7, 2022
* WIP removing sqlite3 specific logic from rosbag2_tests

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Put back the windows stuff, pull out most of the sql in record fixture

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* WIP progress on removing all specific sqlite knowledge in rosbag2_tests

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Finished removing sqlite from all rosbag2_tests

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Parametrize record end to end test to isolate db3 to test param

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Diff massage

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Include default_storage header for windows

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>
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.

Remove rosbag2_tests direct dependence on sqlite3
2 participants