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

Refactor record_fixture to use rcpputils::fs::path #286

Merged

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Feb 5, 2020

Changes

  • Use rcpputils::fs::path instead of std::string for paths held internally by RecordFixture
  • root_bag_path_ is now an rcpputils::fs::path instead of an std::string
  • Added get_bag_file_name(split_index) which returns the name of the split bag file.
  • Added get_bag_file_path(split_index) which returns the relative path to the split bag file.

This refactoring PR is to reduce the changes required for #281

Signed-off-by: Zachary Michaels zmichaels11@gmail.com

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 marked this pull request as ready for review February 5, 2020 01:16

rcpputils::fs::path get_bag_file_path(int split_index)
{
return root_bag_path_ / (get_bag_file_name(split_index) + ".db3");
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes an SQLite storage plugin only.

Copy link
Contributor Author

@zmichaels11 zmichaels11 Feb 5, 2020

Choose a reason for hiding this comment

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

Correct, however the tests only operate on sqlite storage plugin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that it's just moving from https://github.com/ros2/rosbag2/pull/286/files#diff-3609b823f2d4cae7c0418a8efe81cc96L50 - it's out of scope for this PR but we should think about how this test can run against arbitrary plugins

Copy link
Contributor

Choose a reason for hiding this comment

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

@zmichaels11 can you file an issue?

@zmichaels11
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/c1204f4d177c4b0e9c218b2821022312/raw/e8fd35106ec92185885133627e9ced6e6d550dcd/ros2.repos
BUILD args: --packages-up-to rosbag2_tests
TEST args: --packages-select rosbag2_tests
Job: ci_launcher

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Feb 5, 2020

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

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Feb 5, 2020

Rebuilding with unique bag file paths. See #281

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

Edit:
These were built with retest-until-fail 10 since this PR is a stability fix.

@zmichaels11 zmichaels11 merged commit 00071dc into ros2:master Feb 5, 2020
@zmichaels11 zmichaels11 deleted the zmichaels11/record-e2e-test-refactor branch February 5, 2020 21:30
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.

5 participants