Fix LifecycleNode in tests

[christophfroehlich]

Since ros2/rclcpp#2562 we get lots of warnings in the test logs like

[WARN] [1735845337.264023881] [rclcpp_lifecycle]: LifecycleNode is not shut down: Node still in state(1) in destructor

Triggering the shutdown transition before calling the destructor fixes this.

[saikishor]

The changes look good to me. The range_sensor_broadcaster tests seem to fail. Can you take a look?

    [ RUN      ] RangeSensorBroadcasterTest.Initialize_RangeBroadcaster_Exception
    unknown file: Failure
    C++ exception with description "Lifecycle node hasn't been initialized yet!" thrown in TearDown().
    
    [  FAILED  ] RangeSensorBroadcasterTest.Initialize_RangeBroadcaster_Exception (0 ms)

[codecov]

Codecov Report

Attention: Patch coverage is 78.12500% with 21 lines in your changes missing coverage. Please review.

Project coverage is 83.78%. Comparing base (ad7739f) to head (cb3d66a).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...roller/test/test_ackermann_steering_controller.hpp	80.00%	1 Missing ⚠️
...nce_controller/test/test_admittance_controller.hpp	80.00%	1 Missing ⚠️
...ntroller/test/test_bicycle_steering_controller.hpp	80.00%	1 Missing ⚠️
...ive_controller/test/test_diff_drive_controller.cpp	85.71%	1 Missing ⚠️
...ollers/test/test_joint_group_effort_controller.cpp	75.00%	1 Missing ⚠️
...ster/test/test_force_torque_sensor_broadcaster.cpp	75.00%	1 Missing ⚠️
...ontroller/test/test_forward_command_controller.cpp	75.00%	1 Missing ⚠️
...est_multi_interface_forward_command_controller.cpp	75.00%	1 Missing ⚠️
...pper_controllers/test/test_gripper_controllers.cpp	66.66%	1 Missing ⚠️
...r_broadcaster/test/test_imu_sensor_broadcaster.cpp	75.00%	1 Missing ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1470      +/-   ##
==========================================
- Coverage   83.83%   83.78%   -0.05%     
==========================================
  Files         122      122              
  Lines       11120    11190      +70     
  Branches      944      947       +3     
==========================================
+ Hits         9322     9376      +54     
- Misses       1489     1504      +15     
- Partials      309      310       +1     

Flag	Coverage Δ
unittests	83.78% <78.12%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...broadcaster/test/test_range_sensor_broadcaster.cpp	94.89% <100.00%> (+0.16%)	⬆️
...roller/test/test_ackermann_steering_controller.hpp	86.00% <80.00%> (-0.60%)	⬇️
...nce_controller/test/test_admittance_controller.hpp	94.31% <80.00%> (-0.48%)	⬇️
...ntroller/test/test_bicycle_steering_controller.hpp	82.50% <80.00%> (-0.62%)	⬇️
...ive_controller/test/test_diff_drive_controller.cpp	94.87% <85.71%> (-0.33%)	⬇️
...ollers/test/test_joint_group_effort_controller.cpp	97.01% <75.00%> (-1.43%)	⬇️
...ster/test/test_force_torque_sensor_broadcaster.cpp	97.16% <75.00%> (-0.67%)	⬇️
...ontroller/test/test_forward_command_controller.cpp	98.52% <75.00%> (-0.72%)	⬇️
...est_multi_interface_forward_command_controller.cpp	98.31% <75.00%> (-0.82%)	⬇️
...pper_controllers/test/test_gripper_controllers.cpp	97.72% <66.66%> (-2.28%)	⬇️
... and 12 more

... and 4 files with indirect coverage changes

[saikishor]

Suggested change

	try
	{
	controller_->get_node()->shutdown();
	}
	catch (...)
	{
	// ignore case where node is not initialized
	}
	if(controller_->get_node().get_state() != lifecycle_msgs::msg::State::PRIMARY_STATE_UNKNOWN)
	{
	controller_->get_node()->shutdown();
	}

I think it is better to have it like this no?. trying and catching everything might hide some issues we might add in future. What's your opinion on this @christophfroehlich ?. If you think it's fine, I'm okay with it

[christophfroehlich]

The following code

  if(range_broadcaster_->get_node()->get_current_state().id() != lifecycle_msgs::msg::State::PRIMARY_STATE_UNKNOWN)
  {
    range_broadcaster_->get_node()->shutdown();
  }

still throws

2: [ RUN      ] RangeSensorBroadcasterTest.Initialize_RangeBroadcaster_Exception
2: unknown file: Failure
2: C++ exception with description "Lifecycle node hasn't been initialized yet!" thrown in TearDown().

here

https://github.com/ros-controls/ros2_control/blob/d8a21c4e5effa2ac3b75a3dffbc6b48471e18a1f/controller_interface/src/controller_interface_base.cpp#L205-L212

I don't see any other possibility with the controller_interface API.

[christophfroehlich]

we could only catch std::runtime_error and rethrow others.
Or we change the upstream API to check if the node is initialized or add a shutdown method, which itself checks if the node is initialized.

[saikishor]

Ahh I didn't know that this exception is in the controller interface base. If this is the case, we can add an upstream API to check if the controller is initialized or may be in the destructor of the base class, call shutdown if the node is valid and also the state makes sense. What do you think?

[christophfroehlich]

we never trigger the shutdown transition in the controller_manager, maybe this is a bug we should solve?
Currently we only have virtual default destructors of the ControllerInterfaceBase and ControllerInterface class, where would you add that?

[saikishor]

I would say in the destructor of the controller_interface_base. As that's the common one for both normal and chained controllers

[christophfroehlich]

As suggested by @saikishor we should fix that upstream -> ros-controls/ros2_control#1979