Tests for hardware spawner. #1682
Conversation
|
|
||
| int call_spawner(const std::string extra_args) | ||
| { | ||
| std::string spawner_script = "ros2 run controller_manager hardware_spawner "; |
There was a problem hiding this comment.
| std::string spawner_script = "ros2 run controller_manager hardware_spawner "; | |
| std::string spawner_script = "python3 -m coverage run --append --branch " + | |
| "$(ros2 pkg prefix controller_manager)/lib/controller_manager/hardware_spawner "; |
this will create branch coverage data.
|
This pull request is in conflict. Could you fix it @destogl? |
There was a problem hiding this comment.
I think this still has the problem of not fully deciding whether the hardware spawner should be able to spawn multiple components or not.
is_hardware_component_loaded checks for one single string, while activate_components and configure_components expect a list. This PR changes the input argument to a list, but leaves the parameter name and description as singular. I think to make it clear (and to actually make the hardware spawner work) some more work is required, as in fmauch@9a65fc6.
| @@ -117,11 +92,12 @@ def configure_components(node, controller_manager_name, components_to_configure) | |||
| def main(args=None): | |||
| rclpy.init(args=args, signal_handler_options=SignalHandlerOptions.NO) | |||
| parser = argparse.ArgumentParser() | |||
| activate_or_confiigure_grp = parser.add_mutually_exclusive_group(required=True) | |||
| activate_or_configure_grp = parser.add_mutually_exclusive_group(required=True) | |||
|
|
|||
| parser.add_argument( | |||
| "hardware_component_name", | |||
There was a problem hiding this comment.
May I suggest renaming this to hardware_component_names. This will be CLI-breaking but a) is this probably not being used by anybody, since it has been broken since introduced from what I can tell b) it would be good to make the name consistent with the controller spawner and to what it actually expects (a list).
| print(f"CMD Arguments: {command_line_args}") | ||
| print(f"Arguments: {args}") | ||
|
|
There was a problem hiding this comment.
This was probably put in for debugging purposes.
| print(f"CMD Arguments: {command_line_args}") | |
| print(f"Arguments: {args}") |
| EXPECT_EQ(call_spawner("TestSystemHardware --configure -c test_controller_manager"), 1) | ||
| << "could not activate control because not robot description"; |
There was a problem hiding this comment.
Not sure what you wanted to test here, but I think that would make sense:
| EXPECT_EQ(call_spawner("TestSystemHardware --configure -c test_controller_manager"), 1) | |
| << "could not activate control because not robot description"; | |
| EXPECT_EQ( | |
| call_spawner( | |
| "TestSystemHardware --configure -c test_controller_manager --controller-manager-timeout 1.0"), | |
| 256) | |
| << "could not activate control because not robot description"; | |
| EXPECT_EQ( | |
| call_spawner( | |
| "TestSystemHardware --configure -c test_controller_manager --controller-manager-timeout 2.5"), | |
| 0); |
| TestHardwareSpawnerWithoutRobotDescription() | ||
| : ControllerManagerFixture<controller_manager::ControllerManager>("") | ||
| { | ||
| cm_->set_parameter(rclcpp::Parameter("hardware_components_initial_state.unconfigured", "TestSystemHardware")); |
There was a problem hiding this comment.
I think the error rising from this is actually behind a race condition and only shows up with the delay test, since we are waiting here.
| cm_->set_parameter(rclcpp::Parameter("hardware_components_initial_state.unconfigured", "TestSystemHardware")); | |
| cm_->set_parameter(rclcpp::Parameter("hardware_components_initial_state.unconfigured", std::vector<std::string>{"TestSystemHardware"})); |
|
Also note: If we go with the changed parameter name we should probably add migration notes, unless we decide to backport this, as well, since probably nobody ever used it since it isn't working like that, anyway. I've implemented my suggested changes in https://github.com/fmauch/ros2_control/commits/add-tests-for-hw-spawner/ since I needed that on a robot, anyway. |
|
Closing in favor of #1759 |
No description provided.