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.

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

Fix and clarify logic in test_play filter test #690

Merged
merged 3 commits into from
Mar 26, 2021

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Mar 24, 2021

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_filter call will always be overridden in rosbag2_transport.play, which uses PlayOptions.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 was SizeIs(Ge(0u)) which was essentially not testing the filter

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>
@emersonknapp emersonknapp marked this pull request as ready for review March 24, 2021 23:50
@emersonknapp emersonknapp requested a review from a team as a code owner March 24, 2021 23:50
@emersonknapp emersonknapp requested review from jaisontj, zmichaels11, mabelzhang and Karsten1987 and removed request for a team March 24, 2021 23:50
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Copy link
Contributor

@jaisontj jaisontj left a comment

Choose a reason for hiding this comment

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

Nit: We could do a SizeIs(Ge(1u) -> check for "Does atleast one message arrive"

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Mar 26, 2021

Gist: https://gist.githubusercontent.com/emersonknapp/532afc479a05ca8e823d55cd04b7ac9f/raw/324d2087b557a72928c09ff8c2166fbb3507c091/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests
TEST args: --packages-select rosbag2_transport rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7995

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

@emersonknapp
Copy link
Collaborator Author

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

@emersonknapp emersonknapp merged commit ce83baf into master Mar 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/clarify-and-fix-play-filter-test branch March 26, 2021 17:45
@clalancette
Copy link
Contributor

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.

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.

3 participants