-
Notifications
You must be signed in to change notification settings - Fork 312
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
Trigger shutdown transition in destructor #1979
Conversation
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.
The changes look good to me
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1979 +/- ##
==========================================
+ Coverage 89.26% 89.30% +0.04%
==========================================
Files 130 130
Lines 14493 14516 +23
Branches 1257 1258 +1
==========================================
+ Hits 12937 12964 +27
+ Misses 1088 1084 -4
Partials 468 468
Flags with carried forward coverage won't be shown. Click here to find out more.
|
c8d42d9
to
997d5eb
Compare
997d5eb
to
74b26b8
Compare
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.
LGTM
Since ros2/rclcpp#2562 we get lots of warnings in the test logs like
I think we can just trigger the shutdown transition in the destructor of the base class.
We still had some warnings in tests of this repo when
rclcpp::shutdown()
is called before the node is destructed, and the context is invalid. The transition then would fail without therclcpp::ok()
check.