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.

Sign up for GitHub

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

Jump to bottom

Add splitting e2e tests #247

Merged
merged 11 commits into from
Jan 18, 2020
Merged

Add splitting e2e tests #247

merged 11 commits into from
Jan 18, 2020

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Jan 8, 2020 •
edited
Loading

Changes

  • Add end-to-end test to verify bagfiles split when max-bag-size is exceeded.
  • Add end-to-end test to verify bagfiles don't split if max-bag-size is not reached.
  • Add end-to-end test to verify split bagfiles are at least max-bag-size.

@zmichaels11 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch 4 times, most recently from 065ed4e to 8ff08d5 Compare January 8, 2020 18:49
@zmichaels11 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch from 8ff08d5 to e1b18e7 Compare January 9, 2020 17:15
@piraka9011
Copy link
Contributor

piraka9011 commented Jan 9, 2020 •
edited
Loading

I know we already check whether the size of the split is valid for a certain plugin, but I think this test should also verify that by one or more of the following:

  • Explicitly specifying the storage plugin.
  • Adding a test that fails when a bad split size is passed
  • Making a fake storage plugin that overrides it's minimum split size to make this test more general.

Given that the default plugin is sqlite3, I think we should just focus on checking bad split sizes for that.

@zmichaels11
Copy link
Contributor Author

@piraka9011 I'll add that to the issue tracking this PR.

@zmichaels11 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch 2 times, most recently from 56f2b2a to f4e1c8a Compare January 13, 2020 19:52
@zmichaels11 zmichaels11 marked this pull request as ready for review January 13, 2020 23:00
@zmichaels11 zmichaels11 changed the title [WIP] Add splitting e2e tests Add splitting e2e tests Jan 13, 2020
@zmichaels11
Copy link
Contributor Author

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

piraka9011
piraka9011 approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

One nit but this LGTM :shipit:

@piraka9011
Copy link
Contributor

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

@piraka9011
Copy link
Contributor

Windows builds appear to be offline

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 17, 2020 •
edited
Loading

  • Windows Build Status

@zmichaels11
Copy link
Contributor Author

CI is passing, however this PR will need to rebase onto master.
@ros2/aws-oncall rerun CI after #258 merges

@zmichaels11
Add e2e test to verify bag splits
62d3a2c 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Extract create_string_message and make more const
36b783d 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Add e2e test verify bag doesn't split if its limit isnt reached
86d0b9c 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Add e2e test verify split bagfile is at least split size
85269ca 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Update tests for Windows
f30dba2 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Replace FilesystemHelper with rcpputils::fs or rcutils/filesystem
0e6b0ef 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Move message publishing into publisher_manager
999586f 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
@param -> \param
6b829ba 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Make e2e test less flaky
1f59c65 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Fix windows tests
200cf38 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Write metadata out for Windows e2e tests
9eef582 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

Rebased onto master
@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/6c54d6917b6aa6e822308c6517a5a555/raw/bf35996bf499c27effa7a250780abf11b1d2e676/ros2.repos
BUILD args: --packages-up-to rosbag2_tests
TEST args: --packages-select rosbag2_tests
Job: ci_launcher

@piraka9011
Copy link
Contributor

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

@zmichaels11 zmichaels11 merged commit 81ff7cd into ros2:master Jan 18, 2020
@zmichaels11 zmichaels11 deleted the zmichaels11/splitting/e2etests branch January 18, 2020 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piraka9011 piraka9011 piraka9011 approved these changes

@dabonnie dabonnie Awaiting requested review from dabonnie

@emersonknapp emersonknapp Awaiting requested review from emersonknapp

@mm318 mm318 Awaiting requested review from mm318

@prajakta-gokhale prajakta-gokhale Awaiting requested review from prajakta-gokhale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zmichaels11 @piraka9011