search

ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
536 stars 417 forks source link

fix spin_some_max_duration unit-test for events-executor #2465

Closed alsora closed 6 months ago

alsora commented 6 months ago

This PR partially fixes https://github.com/ros2/rclcpp/issues/2462

The EventsExecutor and the StaticSingleThreadedExecutor were failing a unit-test. This PR fixes the test for the EventsExecutor.

The waitables were not triggered, so the events executor was not executing them.

This poses the question... Why do the other executors "execute" a waitable that was not triggered? EDIT: the other executors worked because there was a bug in the is_ready method of the TestWaitable class. This was returning "true" regardless of whether the waitable was triggered. Fixed it as a separate commit 0e6c78d

FYI @wjwwood

mjcarroll commented 6 months ago
alsora commented 6 months ago

There's a few test failures, but they are all unrelated.

Can we still merge this PR? I remember reading about some Timers Manager Windows failures in another ticket, but I can't remember where, is this the same one? rclcpp.TestTimersManager.check_one_timer_cancel_doesnt_affect_other_timers

C:\ci\ws\src\ros2\rclcpp\rclcpp\test\rclcpp\test_timers_manager.cpp:405
Expected: (t1_runs + 5) < (t2_runs), actual: 10 vs 11
clalancette commented 6 months ago
  • uncrustify errors in linux x86_64 and arm64

Yeah, these are unrelated, but should be fixed now.

  • a timers manager error in Windows

Yes, this one has been plaguing us for a while. If you have any time to look into it, it would be most appreciated.

wjwwood commented 6 months ago

Thanks @alsora, sorry I put in the broken test (missed the triggering of the waitables), indeed the TestWaitable was broken by design (for this test).

I'll re-evaluate the tests w.r.t. https://github.com/ros2/rclcpp/pull/2142 based on that.

alsora commented 6 months ago

@clalancette I opened a PR with a proposed fix for the timers manager unit-test. https://github.com/ros2/rclcpp/pull/2468 The test expectation was relying on the test being executed fast enough, which is often not the case on Windows.