test_graph expects 3 nodes to be alive after calling rcl_shutdown on one of their contexts

[Yadunund]

The NodeGraphMultiNodeFixture::SetUp() in test_graph.cpp, invokes TestGraphFixture::SetUp() which initializes an rcl_context_t old_context_ptr, creates a node from it, and then shuts down the context all in the SetUp method. This also shuts down the middleware.

Then jumping back to NodeGraphMultiNodeFixture::SetUp, the function expects all the nodes to be discoverable and specifically 3 nodes including the one whose context was shutdown above.

Perhaps the ROS graph is unchanged for DDS-based middlewares but with rmw_zenoh, shutting down the middleware automatically removes any previously registered nodes from the ROS graph.

What is the best way to update the test here? Personally I'm inclined to updating the test expectation to look for 2 nodes but would also be fine relying on rmw_get_implementation_identifier() to selectively set the expected number of nodes.

[clalancette]

What is the best way to update the test here? Personally I'm inclined to updating the test expectation to look for 2 nodes but would also be fine relying on rmw_get_implementation_identifier() to selectively set the expected number of nodes.

Generally I'm not a fan of using rmw_get_implementation_identifier, so I'd only use that as a last resort.

But I see what you are saying about this test; it seems somewhat illogical to be able to discover a node whose context has ostensibly been shut down. Actually, it is worse than that; some of the other tests in here actively call methods using the old_node_ptr.

It looks like this test was originally added in #57 . @wjwwood I know this was 8 years ago, but do you have any recollection on why we are testing calling methods on a node whose context has been shutdown?

[sloretz]

It looks like at #57 any use of old_node_ptr is expected to return RCL_RET_NODE_INVALID everywhere it's used https://github.com/ros2/rcl/blob/818b15463b736e2198a47b2818b2630a59afbe12/rcl/test/rcl/test_graph.cpp

The expecation that old_node should still be discoverable appears to have been added in #333 , which added the node graph APIs. I wonder if the test expectation was set because that was the observed behavior the RMW implementations at the time.

Just looking at the APIs in rcl, I think the least surprising behavior would be if old_node is no longer discoverable after it's context is shutdown, but I would be supportive of relaxing the test expectation to only wait for the two living nodes and not care if old_node is present or not.

[sloretz]

Closing as fixed by #1192