Skip to content

Add splitting e2e tests#247

Merged
zmichaels11 merged 11 commits intoros2:masterfrom
aws-ros-dev:zmichaels11/splitting/e2etests
Jan 18, 2020
Merged

Add splitting e2e tests#247
zmichaels11 merged 11 commits intoros2:masterfrom
aws-ros-dev:zmichaels11/splitting/e2etests

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

zmichaels11 commented Jan 9, 2020

@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 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch from f4e1c8a to 2834d8d Compare January 15, 2020 17:34
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 16, 2020

@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

@zmichaels11 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch from 2834d8d to 2bc837b Compare January 16, 2020 17:52
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

piraka9011 commented Jan 16, 2020

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

@zmichaels11 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch from 92528ea to 2324a20 Compare January 16, 2020 23:00
@piraka9011
Copy link
Contributor

piraka9011 commented Jan 16, 2020

Windows builds appear to be offline

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 17, 2020 •
edited
Loading

  • Windows Build Status

@zmichaels11 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch from 72c4890 to 4c37ae4 Compare January 17, 2020 21:23
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 17, 2020

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 zmichaels11 force-pushed the zmichaels11/splitting/e2etests branch from 4c37ae4 to 9eef582 Compare January 17, 2020 23:44
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 17, 2020

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

piraka9011 commented Jan 17, 2020

  • 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

@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

1 more reviewer

@piraka9011 piraka9011 piraka9011 approved these changes

Reviewers whose approvals may not affect merge requirements

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