Skip to content

Add test_utils file for transition tests#3048

Open
christophfroehlich wants to merge 7 commits intomasterfrom
add/test/helper
Open

Add test_utils file for transition tests#3048
christophfroehlich wants to merge 7 commits intomasterfrom
add/test/helper

Conversation

@christophfroehlich
Copy link
Member

@christophfroehlich christophfroehlich commented Feb 25, 2026

@codecov
Copy link

codecov bot commented Feb 25, 2026

Codecov Report

❌ Patch coverage is 93.10345% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.36%. Comparing base (152ed06) to head (3015cf3).

Files with missing lines Patch % Lines
controller_interface/test/test_test_utils.cpp 89.69% 7 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3048      +/-   ##
==========================================
+ Coverage   89.35%   89.36%   +0.01%     
==========================================
  Files         158      160       +2     
  Lines       19392    19538     +146     
  Branches     1573     1576       +3     
==========================================
+ Hits        17327    17460     +133     
- Misses       1419     1428       +9     
- Partials      646      650       +4     
Flag Coverage Δ
unittests 89.36% <93.10%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
...erface/include/controller_interface/test_utils.hpp 100.00% <100.00%> (ø)
controller_interface/test/test_test_utils.cpp 89.69% <89.69%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@christophfroehlich christophfroehlich marked this pull request as draft February 25, 2026 09:49
@christophfroehlich christophfroehlich marked this pull request as ready for review February 27, 2026 20:10
Copy link

@jsantoso91 jsantoso91 left a comment

Choose a reason for hiding this comment

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

Looks good to me, I looked at the design document for the lifecycle found here
https://design.ros2.org/articles/node_lifecycle.html

There are some discrepancies (mentioned in the line comments) for deactivate and cleanup transitions, but since test cases written for these are passing I assume it is just the issue of outdated documentation.

{
case State::PRIMARY_STATE_INACTIVE:
return true;
case State::PRIMARY_STATE_ACTIVE:

Choose a reason for hiding this comment

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

Based on the design document found here https://design.ros2.org/articles/node_lifecycle.html
There is no explicit transition for onDeactivate[FAILURE], but since the test case you wrote for this one seems to PASS I assume the document is the outdated one and the behavior ACTIVE STATE -> Deactivate [Failed] -> ACTIVE STATE is expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

there are three return types of the transition callbacks:
https://github.com/ros2/rclcpp/blob/b6e9b4c9b48c48eae09ab79d7b2d4ef95e6c4653/rclcpp_lifecycle/include/rclcpp_lifecycle/node_interfaces/lifecycle_node_interface.hpp#L56-L61

As far as I understood it:

  • error: onError transition is called
  • failure: no transition is performed (staying in old state).

Here this means that after failure in on_deactivate, it will stay in active mode.

Choose a reason for hiding this comment

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

Thank you for sharing the link.
I guess it makes sense when the state transition fails (and no error is thrown), that the state will stay where it is.

{
case State::PRIMARY_STATE_UNCONFIGURED:
return true;
case State::PRIMARY_STATE_INACTIVE:

Choose a reason for hiding this comment

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

Based on the design document found here https://design.ros2.org/articles/node_lifecycle.html
There is no explicit transition for onCleanup[FAILURE], but since the test case you wrote for this one seems to PASS I assume the document is the outdated one and the behavior INACTIVE STATE -> Cleanup [Failed] -> INACTIVE STATE is expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

same here, if transition fails it stays in inactive

case State::PRIMARY_STATE_FINALIZED:
return true;
default:
// if transition returns error or failure, it will anyways end up in the finalized state

Choose a reason for hiding this comment

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

Is this comment accurate? Based on the diagram included in https://design.ros2.org/articles/node_lifecycle.html
Suppose error is raised during onShutdown() then state will go to ErrorProcessing and further to Unconfigured or Finalized depending on the success/failure of onError.

Copy link
Member Author

Choose a reason for hiding this comment

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

this caused some headache, but this was the result of playing around with the tests.

The diagram also shows no [SUCCESS] condition for the shutdown.

Choose a reason for hiding this comment

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

I think you meant to say "The diagram also shows no [FAILURE] condition for the shutdown" ?

I think a more accurate comment for the default branch (at least based on the lifecycle diagram) would be
"if transition returns error, the state will end up in the Unconfigured or Finalized depending on the success/failure of onError"

Copy link

@jsantoso91 jsantoso91 left a comment

Choose a reason for hiding this comment

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

Suggested comment change for L157:

"if transition returns error, the state will end up in the Unconfigured or Finalized depending on the success/failure of onError"

Looks good otherwise.

@github-project-automation github-project-automation bot moved this from Needs review to WIP in Review triage Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: WIP

Development

Successfully merging this pull request may close these issues.

2 participants