Skip to content

Do not expect empty StorageOptions URI to work in *CompressionWriterTest#526

Merged
ivanpauno merged 1 commit intoros2:masterfrom
christophebedard:rosbag2-compression-writer-test-fix-create-directories-with-empty-string
Oct 5, 2020
Merged

Do not expect empty StorageOptions URI to work in *CompressionWriterTest#526
ivanpauno merged 1 commit intoros2:masterfrom
christophebedard:rosbag2-compression-writer-test-fix-create-directories-with-empty-string

Conversation

@christophebedard
Copy link
Copy Markdown
Member

@christophebedard christophebedard commented Sep 25, 2020

For ros2/rcpputils#94, I proposed ros2/rcpputils#95, which made it so that rcpputils::fs::create_directories() returns false if the directory path is invalid, e.g. if the path is empty, invalid, not a directory, etc.

This was the case for SequentialCompressionWriterTest, which assumed that using an empty rosbag2_cpp::StorageOptions URI would work. That test then started failing and ros2/rcpputils#95 was reverted. This PR changes SequentialCompressionWriterTest so that ros2/rcpputils#95 can be merged back in.

See ros2/rcpputils#97 (review)

@christophebedard
Copy link
Copy Markdown
Member Author

SequentialCompressionWriter::open should perhaps be modified to throw std::invalid_argument{"URI is empty."} if storage_options.uri.empty().

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard force-pushed the rosbag2-compression-writer-test-fix-create-directories-with-empty-string branch from 7b129ec to 4dd6af9 Compare September 25, 2020 16:33
Copy link
Copy Markdown
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

std::move(metadata_io_));
writer_ = std::make_unique<rosbag2_cpp::Writer>(std::move(sequential_writer));

EXPECT_THROW(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@christophebedard
Copy link
Copy Markdown
Member Author

@emersonknapp / @jaisontj could you take a look at this? It's needed for other PRs.

@christophebedard
Copy link
Copy Markdown
Member Author

CI is at ros2/rcpputils#98 (comment)

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Oct 5, 2020

CI passed (see ros2/rcpputils#98 (comment)), merging together with ros2/rcpputils#98.

(I will release the rcpputils repo to make the PR checker pass again)

@ivanpauno ivanpauno merged commit 6e43912 into ros2:master Oct 5, 2020
@christophebedard christophebedard deleted the rosbag2-compression-writer-test-fix-create-directories-with-empty-string branch October 5, 2020 18:33
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
…est (#526)

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
…est (#526)

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
skudryas pushed a commit to skudryas/rosbag2 that referenced this pull request Mar 12, 2021
…est (ros2#526)

Signed-off-by: Christophe Bedard <bedard.christophe@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.

4 participants