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

Wait for metadata to be written to disk #283

Merged
merged 7 commits into from
Feb 7, 2020

Conversation

piraka9011
Copy link
Contributor

Adds a method to the rosbag2 record E2E test fixture to wait until the metadata has been written to disk.
This resolves test flakiness on systems that are slow to flush all file system data to disk.

Signed-off-by: Anas Abou Allaban allabana@amazon.com

Copy link
Contributor

@zmichaels11 zmichaels11 left a comment

Choose a reason for hiding this comment

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

LGTM. Consider making the assert statement more verbose

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.

@piraka9011 this looks fine, can you rebase and add DCO signoff so it passes checks?

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, one nit

@@ -87,6 +87,24 @@ class RecordFixture : public TemporaryDirectoryFixture
return root_bag_path_ / (get_bag_file_name(split_index) + ".db3");
}

void wait_for_metadata(int timeout_in_sec = 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that timeout_in_sec is never used, so we don't need this parameter, we can just set 5 as a constant.
https://martinfowler.com/bliki/Yagni.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to define it as a constant somewhere or just use 5 directly?

Anas Abou Allaban and others added 7 commits February 7, 2020 10:06
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

Co-Authored-By: Zachary Michaels <zmichaels11@users.noreply.github.com>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

piraka9011 commented Feb 7, 2020

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

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

@piraka9011 piraka9011 merged commit 3934be7 into ros2:master Feb 7, 2020
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