-
Notifications
You must be signed in to change notification settings - Fork 10
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
Ros2 fix battery and manager tests #319
Conversation
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
WalkthroughThe recent updates primarily enhance the quality of service (QoS) settings for ROS 2 publishers and modify error handling mechanisms in test scenarios. Specifically, the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- panther_battery/test/include/test_battery_node.hpp (1 hunks)
- panther_battery/test/test_battery_publisher.cpp (1 hunks)
- panther_manager/behavior_trees/lights.xml (1 hunks)
- panther_manager/behavior_trees/safety.xml (1 hunks)
- panther_manager/test/plugins/test_call_set_bool_service_node.cpp (1 hunks)
- panther_manager/test/plugins/test_call_set_led_animation_service_node.cpp (2 hunks)
- panther_manager/test/test_safety_manager_node.cpp (1 hunks)
Files skipped from review due to trivial changes (3)
- panther_manager/behavior_trees/safety.xml
- panther_manager/test/plugins/test_call_set_led_animation_service_node.cpp
- panther_manager/test/test_safety_manager_node.cpp
Additional comments not posted (4)
panther_battery/test/test_battery_publisher.cpp (1)
90-90
: Updated QoS settings forio_state_pub_
to enhance reliability and behavior.Verification successful
The new QoS settings for
io_state_pub_
have been verified and align with the intended behavior of the tests. The changes ensure reliable delivery ofIOStateMsg
messages.
panther_battery/test/test_battery_node.cpp
panther_battery/test/test_battery_publisher.cpp
panther_battery/test/test_battery_node_dual_bat.cpp
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of new QoS settings on system behavior. # Test: Check for any reported issues or errors related to IOState messages. rg --type python $'io_state_pub_->publish'Length of output: 74
Script:
#!/bin/bash # Description: Verify the impact of new QoS settings on system behavior. # Test: Check for any reported issues or errors related to IOState messages. rg --type cpp 'io_state_pub_->publish'Length of output: 291
Script:
#!/bin/bash # Description: Extract relevant lines to verify the impact of new QoS settings. # Extract lines where `io_state_pub_->publish` is used. rg --type cpp --context 5 'io_state_pub_->publish' panther_battery/test/test_battery_node.cpp rg --type cpp --context 5 'io_state_pub_->publish' panther_battery/test/test_battery_publisher.cpp rg --type cpp --context 5 'io_state_pub_->publish' panther_battery/test/test_battery_node_dual_bat.cppLength of output: 1491
panther_manager/test/plugins/test_call_set_bool_service_node.cpp (1)
151-151
: Replaced old logic withEXPECT_THROW
to enhance error handling in tests.Verification successful
The search results confirm that
EXPECT_THROW
is consistently used across various test files to handle exceptions, including inpanther_manager/test/plugins/test_call_set_bool_service_node.cpp
. This aligns with the change made in the pull request.
panther_manager/test/plugins/test_call_set_bool_service_node.cpp: EXPECT_THROW({CreateTree("CallSetBoolService", service);}, BT::LogicError);
This indicates that the change to use
EXPECT_THROW
for exception handling is appropriate and consistent with the existing codebase practices.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the correct exceptions are being thrown as expected. # Test: Check for the presence of `EXPECT_THROW` in other similar test cases. rg --type python $'EXPECT_THROW'Length of output: 62
Script:
#!/bin/bash # Description: Verify that the correct exceptions are being thrown as expected. # Test: Check for the presence of `EXPECT_THROW` in other similar test cases. rg --type cpp $'EXPECT_THROW'Length of output: 6910
panther_manager/behavior_trees/lights.xml (1)
52-52
: Refined conditional check inservice_name
attribute for better logical clarity.Verification successful
Refined conditional check in
service_name
attribute for better logical clarity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the refined conditional check on behavior tree execution. # Test: Check for any logical errors or unexpected behaviors in the execution of behavior trees. rg --type xml $'service_name'Length of output: 2143
Script:
#!/bin/bash # Description: Extract the specific line in `lights.xml` where the conditional check was refined. # Extract the line containing the refined conditional check. rg --type xml --line-number --context 5 '_skipIf="(!e_stop_state) || (current_anim_id == E_STOP_ANIM_ID)"' panther_manager/behavior_trees/lights.xmlLength of output: 6052
panther_battery/test/include/test_battery_node.hpp (1)
112-112
: Updated QoS settings forio_state_pub_
to enhance reliability and behavior.Verification successful
The updated QoS settings for
io_state_pub_
have been verified, and no issues or errors related toIOState
messages were found in the codebase.
- The
io_state_pub_
publisher is used in multiple test files and the main codebase.- The new QoS settings are applied consistently across these usages.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of new QoS settings on system behavior. # Test: Check for any reported issues or errors related to IOState messages. rg --type python $'io_state_pub_->publish'Length of output: 74
Script:
#!/bin/bash # Description: Verify the impact of new QoS settings on system behavior. # Test: Check for any usage of io_state_pub_ in the codebase. rg 'io_state_pub_' # Test: Check for any reported issues or errors related to IOState messages. rg 'IOState'Length of output: 7617
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- panther_battery/test/include/test_battery_node.hpp (1 hunks)
- panther_battery/test/test_battery_publisher.cpp (1 hunks)
- panther_manager/test/plugins/test_call_set_bool_service_node.cpp (1 hunks)
- panther_manager/test/plugins/test_call_set_led_animation_service_node.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- panther_battery/test/include/test_battery_node.hpp
- panther_battery/test/test_battery_publisher.cpp
- panther_manager/test/plugins/test_call_set_bool_service_node.cpp
- panther_manager/test/plugins/test_call_set_led_animation_service_node.cpp
Description
Test fixes. Pulled this #317
Modifications
Summary by CodeRabbit
Bug Fixes
io_state_pub_
publisher to enhance reliability and transient local durability.Refactor
lights.xml
andsafety.xml
for better logic clarity and correctness.Tests
EXPECT_THROW
statements to manage exceptions more effectively.