Remove ready_fn from launch_testing tests

[pbaughman]

This is part of the effort here to replace the OpaqueFunction and ready_fn with a launch_testing action.

Signed-off-by: Pete Baughman peter.c.baughman@gmail.com

[pbaughman]

@wjwwood Hey William - sorry to bug you but it's hard to tell who normally maintains this repo. Can you suggest someone to ping to take a look at this? Thanks.

[wjwwood]

Sorry, it likely would have sat here until our triage process picked it up. Everyone is really busy right now. lgtm though

[ahcorde]

• Linux
• Linux-aarch64
• macOS
• Windows

[pbaughman]

@ahcorde Is the macOS failure a known issue for you? That problem it not caused by my change to the test.

[wjwwood]

@pbaughman can this be tested separately from ros2/launch#346? @ahcorde did you test just this pr or the other related prs too? Just glancing at the repos file it looks like these CI were with master of ros2/launch.

Thanks for starting CI though!

[pbaughman]

@wjwwood - Yes, it should be possible to test this with 'master' launch. https://github.com/ros2/launch/pull/346/files only adds warnings. The ReadyToTest action is already in there.

We wanted to update all of the ROS2 repos to use the ReadyToTest action before merging launch PR 346 because it would be confusing to have warnings in packages that were "up to date"

[ahcorde]

Test is failing sereval time here https://ci.ros2.org/job/ci_osx/7843/console#console-section-12. I will try to reproduce this locally

@wjwwood I tested only this PR, let me know which ones I should take in account and I will take care of it

[wjwwood]

Sounds like this one can be tested (and merged) separately.

[pbaughman]

@ahcorde It's almost certainly a synchronization issue with the start-up of the processes under test. It doesn't look like this test does anything to make sure the processes start up in the right order or the resources the test process intends to use are ready before it tries to use them.

I've found the easiest way to reproduce this type of failure is to add 'sleep' to the beginning of the different processes under test - particularly the ones providing the necessary resources to the other processes. I can dig into this deeper tomorrow, but if that's the case then the test was always broken, even before this PR.

[pbaughman]

Yeah - it's a race. I can make this fail on my machine. If I add some delays to the test so the processes under test run longer, I can look at the TF2 stuff while the test is running manually.

terminal 1> ros2 test test_tf2/test/static_publisher.launch.py --disable-isolation

terminal 2> ros2 run tf2_ros tf2_echo b c
Warning: Invalid frame ID "b" passed to canTransform argument target_frame - frame does not exist
         at line 133 in /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/geometry2/tf2/src/buffer_core.cpp
Warning: Invalid frame ID "b" passed to canTransform argument target_frame - frame does not exist
         at line 133 in /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/geometry2/tf2/src/buffer_core.cpp
. . .
. . .
# After a few dozen lines of the warning:
At time 0.0                                           
- Translation: [0.000, 1.000, 0.000]                  
- Rotation: in Quaternion [0.000, 0.000, 0.000, 1.000]
At time 0.0
- Translation: [0.000, 1.000, 0.000]
- Rotation: in Quaternion [0.000, 0.000, 0.000, 1.000]
At time 0.0
- Translation: [0.000, 1.000, 0.000]
- Rotation: in Quaternion [0.000, 0.000, 0.000, 1.000]
. . .

Most of the tests look a little like

TEST(StaticTransformPublisher, a_b_different_times)                                          
{                                                                                            
  auto node = rclcpp::Node::make_shared("StaticTransformPublisher_a_b_different_times_test");
                                                                                             
  rclcpp::Clock::SharedPtr clock = std::make_shared<rclcpp::Clock>(RCL_SYSTEM_TIME);         
  tf2_ros::Buffer mB(clock);                                                                 
  tf2_ros::TransformListener tfl(mB, node, false);                                           
                                                                                             
  rclcpp::executors::SingleThreadedExecutor executor;                                        
  executor.add_node(node);                                                                   
  // Start spinning in a thread                                                              
  std::thread spin_thread = std::thread(                                                     
    std::bind(&rclcpp::executors::SingleThreadedExecutor::spin, &executor));                 
                                                                                             
  std::this_thread::sleep_for(std::chrono::milliseconds(200));  // <---- Problematic sleep
                                                                                             
  EXPECT_TRUE(mB.canTransform("a", "b", tf2::timeFromSec(0)));                               
  EXPECT_TRUE(mB.canTransform("a", "b", tf2::timeFromSec(100)));                             
  EXPECT_TRUE(mB.canTransform("a", "b", tf2::timeFromSec(1000)));                            

If I change the sleep from '200' to '500' then the tests start to pass again.

@wjwwood @ahcorde How would you like me to move forward with this? The test is inherently racy. Does TF2 provide a mechanism that can tell me when it's got its subscriptions plumbed? Should I put the EXPECT_TRUE stuff in a retry with a delay? Something else?

[jacobperron]

The test that is failing on macOS has failed in a previous nightly (March 9th), so I don't think we should block this PR because of that failure. The failure hasn't happened recently and it's been a while since the last CI run for this PR, so I've retriggered a new batch of jobs:

• Linux
• Linux-aarch64
• macOS
• Windows (edit: the same failures occur on the latest nightly)

[jacobperron]

LGTM, thanks for the PR!