I would appreciate having this test actually happening in rosbag2_cpp so it can be applied to more than just SQLite3.

I had trouble creating the test. Can you suggest a test file in rosbag2_cpp that I can look at as an example of how to put messages into a reader? I tried adding it in rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp, but I'm not able to get a valid object returned from read_next() - I get null pointers. Maybe because the object is a MockStorage?

The other problem is since the actual filtering is only implemented in sqlite3 right now, we wouldn't be able to get the right filtering behavior from a SequentialReader using MockStorage, right?

What I have done locally is I added a new class to test_sequential_reader.cpp, changed the reader_ object from a Reader to a SequentialReader, since the set_filter() is only in SequentialReader right now. If we want it to be in Reader, it also needs to be in BaseReadInterface and rosbag2_compression::SequentialCompressionReader. Do we want to define set_filter() beyond SequentialReader?

I guess you have two options here. Either you modify the mock functions in a way to take the filter logic into account or you shift the test over to rosbag2_tests where we have a fully set up architecture. This also runs the sqlite3 plugin and still gives you the opportunity to test the c++ API.

Do you think it makes sense to apply the filter to each individual reader instance? Then we should add the two functions (set_filter()/reset_filter()). I would assume that these filters are generic enough (filter by topic etc) to apply for each reader.

I spent some time putting the tests in rosbag2_cpp and rosbag2_tests. I did both halfway, and I think I finally figured out what the tests in these 3 packages do.

In the rosbag2_cpp tests, test_sequential_reader.cpp is intentionally using a mock object in mock_storage.cpp so that it tests the functions in the SequentialReader only, just to verify that reader_ calls functions inside storage_. The tests here are independent of the Storage object.

So with re/set_filter(), in the current setup, all that should be tested in rosbag2_cpp is that reader_.set_filter() will call storage_.set_filter(), and no checks on whether the returned objects are the correct topic names. That can be added - I have this locally.

The tests in rosbag2_storage test the actual function of the storage. So set_filter() here should be tested with the actual returned messages' topic names. This is the present state of the PR. I think we should keep these tests, because there's nowhere else that does unit test on the actual functionality of the Storage objects.

The tests in rosbag2_tests execute the command line, which would require implementing the topic filter in rosbag2_transport. That has not been implemented yet. It would require enough changes that maybe it should be in a separate PR? With rosbag2_transport tests, test_play.cpp uses a mock_sequential_reader.cpp mock object, which has its functions implemented. This can be updated with set_filter() to mock the functionality of the storage - I have this locally.

So my question is what we want to go into this PR - I have a mess of both routes on my local computer, with new tests in all 3 packages, and I haven't pushed anything. I think both the rosbag2_storage and rosbag2_cpp tests should be kept. In that case, I would just add the few lines to rosbag2_cpp tests as mentioned above to this PR, and leave this test in rosbag2_storage as well.

If we also want to add end-to-end tests and implement a new flag on rosbag2_transport, I can put that in a new branch and open a new PR. Otherwise I would just get rid of those changes. That option did not exist in ROS 1 bag playback and I don't know what flag we want to make it - maybe like ROS 1 bag record where -O is for the file name, and a subsequent list is the selective topics.

I agree with basically everything you said here.

I would still appreciate some basic tests in rosbag2_cpp for things like what happens if you call the set_filter function twice in a row or test for the exception to be thrown if the storage is not initialized. That shouldn't take too much time, I think.

The functionality can be tested in the sqlite3 implementation of it (I think that's what you meant with rosbag2_storage, but I think it's indeed the default storage plugin).
But I would also assume we could get around rosbag2_transport or the ros2cli in the system tests (rosbag2_tests) by directly calling the rosbag2_cpp API. But I am okay with putting this in a separate PR to address once the option is exposed through the command line interface. I would ask you though in this case to open a new ticket for it to keep track of the outstanding work. It's up to you, really, which way you want to go. I am okay with both.

Okay. I added basic tests in rosbag2_cpp in 5e09b58.

I tried making a new test in rosbag2_tests and calling the SequentialReader::open() in rosbag2_cpp to open the bag in rosbag2_tests/resources/cdr_test. For some reason it doesn't open:

[22.820s] 4: [ERROR] [1585629564.565606326] [rosbag2_storage]: Could not open 'cdr_test.db3' with 'sqlite3'. Error: Failed to read from bag: File 'cdr_test.db3' does not exist!
[22.820s] 4: [ERROR] [1585629564.565750830] [rosbag2_storage]: Could not load/open plugin with storage id 'sqlite3'.

In fact, I cannot even open cdr_test and wrong_rmw_test with ros2 bag play on the command line. I get the same error:

$ ros2 bag play cdr_test
[ERROR] [rosbag2_storage]: Could not open 'cdr_test.db3' with 'sqlite3'. Error: Failed to read from bag: File 'cdr_test.db3' does not exist!
[ERROR] [rosbag2_storage]: Could not load/open plugin with storage id 'sqlite3'.
[ERROR] [rosbag2_transport]: Failed to play: No storage could be initialized. Abort

But ros2 bag info cdr_test works.
Is this expected? If so, then I won't be able to call Reader directly from rosbag2_tests. I will open a new ticket to expose the topic selection to rosbag2_transport, and then test using the existing test_rosbag2_play_end_to_end.cpp.

total bike shedding, but I somehow feel it reads easier if you invert the logic:

if (nullptr == storage_) {
  throw ...
}

storage_->set_filter(storage_filter);

same below

So here I just copied the pattern in the three existing functions above these lines...