Fix NodeOptions assignment operator#2656
Conversation
Also copy the enable_logger_service_ member during the assignment operation Signed-off-by: Romain DESILLE <r.desille@gmail.com>
| EXPECT_EQ(options.use_clock_thread(), copied_options.use_clock_thread()); | ||
| EXPECT_EQ(options.allow_undeclared_parameters(), copied_options.allow_undeclared_parameters()); | ||
| EXPECT_EQ(options.automatically_declare_parameters_from_overrides(), copied_options.automatically_declare_parameters_from_overrides()); | ||
|
|
There was a problem hiding this comment.
can you check all attributes here?
Also, this test isn't testing much, because enable_logger_service_ is false by default and the test does not change its value before copy-assigning it. In other words, this test passes even without your fix.
I'd say move this part to the end of this test, create a NodeOptions object, try to set all options to the non-default value, then copy-assign it to a second object, and then assert equality of each attribute.
There was a problem hiding this comment.
Also, there are some linter failures due to line length, see: https://build.ros2.org/job/Rpr__rclcpp__ubuntu_noble_amd64/351/testReport/junit/rclcpp/uncrustify/test_rclcpp_test_node_options_cpp/
There was a problem hiding this comment.
You're right, I realized the current test was not tested anything after I submitted the PR.
Thanks for the hint, I will modify the test.
|
Nice catch! Looks like this was missed in #2122. I just have one simple comment. Then we should backport this to Jazzy and Iron. |
Signed-off-by: Romain DESILLE <r.desille@gmail.com>
|
I added more tests and a small comment to list attributes that were not / couldn't be tested. How does backports to Jazzy and Iron works? Should I open PR to the said branches after this one is approved? |
|
@rdesille thank you for your contribution!
We should first finish to review and merge this PR. CI run:
|
Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> Signed-off-by: Romain DESILLE <r.desille@gmail.com>
christophebedard
left a comment
There was a problem hiding this comment.
Thanks for making the changes!
|
Pulls: #2656
|
|
@Mergifyio backport jazzy |
✅ Backports have been createdDetails
|
|
i will backport this only to jazzy. (Iron is just one day away from E.O.L and humble does not have this code.) |
* Fix NodeOptions assignment operator Also copy the enable_logger_service_ member during the assignment operation * Add more checks for NodeOptions copy test * Set non default values by avoiding the copy-assignement Signed-off-by: Romain DESILLE <r.desille@gmail.com> Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> (cherry picked from commit 9b65494)
* Fix NodeOptions assignment operator Also copy the enable_logger_service_ member during the assignment operation * Add more checks for NodeOptions copy test * Set non default values by avoiding the copy-assignement Signed-off-by: Romain DESILLE <r.desille@gmail.com> Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> (cherry picked from commit 9b65494) Co-authored-by: Romain DESILLE <r.desille@gmail.com>
While working with NodeOptions, I realized "enable_logger_service" was copied with the assignment operator.
Here is a little PR to fix the issue. I also added some additional checks for primitives members in
TEST(TestNodeOptions, copy).Let me know what you think