Fix the lifecycle tests on RHEL-9.#2583
Conversation
The full explanation is in the comment, but basically since RHEL doesn't support mocking_utils::inject_on_return, we have to split out certain tests to make sure resources within a process don't collide. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> Signed-off-by: Chris Lalancette <clalancette@gmail.com>
|
CI:
|
wjwwood
left a comment
There was a problem hiding this comment.
lgtm
Might be worth referencing the original issue from the comment.
| auto patch = mocking_utils::patch_and_return( | ||
| "lib:rclcpp_lifecycle", rcl_lifecycle_state_machine_init, RCL_RET_ERROR); | ||
| EXPECT_THROW( | ||
| std::make_shared<EmptyLifecycleNode>("testnode").reset(), |
There was a problem hiding this comment.
I know this is an existing issue with the test and out of scope for this pr, but technically this is testing both the constructor and destructor, which while the latter shouldn't throw, it could and this test would pass if it did I think. Also the .reset() is completely redundant here.
There was a problem hiding this comment.
I know this is an existing issue with the test and out of scope for this pr, but technically this is testing both the constructor and destructor
Since we are patching rcl_lifecycle_state_machine_init to fail, it means that the constructor of LifecycleNodeInterfaceImpl throws an exception. In turn, that means that the constructor of LifecycleNode fails, and finally that means that the constructor of EmptyLifecycleNode fails. Because of that chain of events, as long as things are working as expected the destructor will never run.
I guess in the case where the chain of constructors does not fail, then you are correct in that the destructor could then run, could throw an exception, and thus erroneously pass the test. But I don't see a good way to fix this; in the nominal case we don't get an object back (because the constructor has failed). I guess we could attempt to assign the EmptyLifecycleNode to an object, and if that succeeds then we know that this test failed? Not sure it is worth it.
Also the
.reset()is completely redundant here.
Yeah, that is definitely true, though not a big deal, I guess.
I'm going to go ahead with this PR as-is, because I'm honestly not sure how to solve the other problems. But I'm happy to make another change if you have an idea of how to improve this.
The full explanation is in the comment, but basically since RHEL doesn't support mocking_utils::inject_on_return, we have to split out certain tests to make sure resources within a process don't collide.