Fix and clarify logic in test_play filter test#690
Conversation
The test was allowing the filter to be incorrect by not checking for the correct number of messages, while setting the filter in a way that was overridden by play_options_. This doesn't fix the API confusion - I don't have time for that right now. The `Reader.set_filter` call will always be overridden in `rosbag2_transport.play`, which uses `PlayOptions.topics_to_filter`. Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
jaisontj
left a comment
There was a problem hiding this comment.
Nit: We could do a SizeIs(Ge(1u) -> check for "Does atleast one message arrive"
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
|
Gist: https://gist.githubusercontent.com/emersonknapp/532afc479a05ca8e823d55cd04b7ac9f/raw/324d2087b557a72928c09ff8c2166fbb3507c091/ros2.repos
|
|
Windows CI is currently broken in the infrastructure - see ros-infrastructure/ros2-cookbooks#30 - I don't want to be blocked by that, we can fix any issues that arise later, if they do come up |
Just as a point of order; I might suggest not doing this. If any platform is going to fail, it is going to be Windows. |
The test was allowing the filter to be incorrect by not checking for the correct number of messages, while setting the filter in a way that was overridden by
play_options_. This PR restructures the existing test to be more readable, clear in its intention, and correct in its final expectations.This doesn't fix the API confusion - I want to do that but it will have to wait for the moment - in the meantime this change at least fixes the test. The
Reader.set_filtercall will always be overridden inrosbag2_transport.play, which usesPlayOptions.topics_to_filter.Note to the reviewer - the most important part of this change is
EXPECT_THAT(topics, SizeIs(0u))on the filtered tests - it wasSizeIs(Ge(0u))which was essentially not testing the filter