Fix flaky lifecycle node tests

[jacobperron]

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

[fujitatomoya]

lgtm

[clalancette]

One nit inline, but I don't feel strongly about it. Seems good to me with green CI.

[jacobperron]

PR job failure looks unrelated (a test failure in rclcpp). Here's CI for rclcpp_lifecycle:

• Linux
• Linux-aarch64
• macOS
• Windows (unrelated CMake warning)

[jacobperron]

I just noticed that this rclcpp test is also flaky due to a similar pattern of creating a ROS entity and then immediately checking that it exists:

rclcpp/rclcpp/test/rclcpp/test_node.cpp

Lines 2558 to 2561 in f986ca3

	auto publisher = node->create_publisher<test_msgs::msg::BasicTypes>(topic_name, qos);
	// List should have one item
	auto publisher_list = node->get_publishers_info_by_topic(fq_topic_name);
	ASSERT_EQ(publisher_list.size(), (size_t)1);

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.

[clalancette]

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.

[eboasson]

@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.

[clalancette]

@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?

[jacobperron]

Makes sense to me. I'm working on adding API to rcl to make this kind of testing pattern easier, e.g. rcl_wait_for_publishers(). Stay tuned...

[jacobperron]

Stay tuned...

ros2/rcl#907