-
Notifications
You must be signed in to change notification settings - Fork 418
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
Fix flaky lifecycle node tests #1606
Conversation
Apparently, the topics and services that LifecycleNode provides are not available immediately after creating a node. Fix flaky tests by accounting for some delay between the LifecycleNode constructor and queries about its topics and services. Signed-off-by: Jacob Perron <[email protected]>
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
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.
One nit inline, but I don't feel strongly about it. Seems good to me with green CI.
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
I just noticed that this rclcpp/rclcpp/test/rclcpp/test_node.cpp Lines 2558 to 2561 in f986ca3
Seems like this pattern is not reliable since the switch to CycloneDDS. I'm not sure we guarantee anywhere that the graph query API will contain entities that were created right before the query, even if they were created by the same node making the query. |
Hm, good point. @eboasson if you get a chance, could you share some thoughts here? We don't have any guarantees around this, but the behavior is different than we used to see. So we're wondering if this is expected, and we should update the tests, or if it indicates an issue elsewhere. |
@clalancette I checked, the reason is that the Cyclone RMW layer updates the graph layer asynchronously from the discovery thread. There's no particular reason the create/destroy subscription and publisher operations can't update the graph synchronously, one would just have to make sure to filter out any local entities in the discovery thread. |
Thanks for looking into it, it is appreciated. So it sounds this isn't unexpected, it is just different from what we had before. I guess in order to be robust to different implementations, the tests really shouldn't expect that the topics and services are immediately available. @jacobperron does that make sense to you? |
Makes sense to me. I'm working on adding API to |
|
Apparently, the topics and services that LifecycleNode provides are not
available immediately after creating a node.
Fix flaky tests by accounting for some delay between the LifecycleNode
constructor and queries about its topics and services.
Fixes #1605