Skip to content

Remove the flaky spinAll executor test. - #3399

Merged
ahcorde merged 1 commit into
rollingfrom
clalancette/remove-flaky-spinall-test
Nov 4, 2026
Merged

ahcorde merged 1 commit into
rollingfrom
clalancette/remove-flaky-spinall-test

Conversation

@clalancette

Copy link
Copy Markdown
Contributor

Description

The test triggered the waitable only after starting the spinner thread. Every executor returns from spin_all() as soon as no work is ready, so if spin_all() started before the first trigger it returned immediately and the waitable count stayed at 0. This happens most often with the EventsExecutor.

The test cannot be fixed without overlapping existing tests, which already cover what it was meant to check on the same executor types:

  • TestBusyWaiting.test_spin_all and test_spin_some (test_executors_busy_waiting.cpp) check that spin_all() picks up work that becomes ready during the spin and spin_some() does not.
  • cancelRacingSpinDoesNotHang and cancelBeforeNonBlockingSpinVariantsDoNotHang check cancellation.

Is this user-facing behavior change?

No.

Did you use Generative AI?

Yes, Claude Opus 5.5

Additional Information

@mjcarroll @skyegalaxy @jmachowinski I could use your thoughts/advice on this one.

The test triggered the waitable only after starting the spinner thread.
Every executor returns from spin_all() as soon as no work is ready, so
if spin_all() started before the first trigger it returned immediately
and the waitable count stayed at 0.  This happens most often with the
EventsExecutor.

The test cannot be fixed without overlapping existing tests, which
already cover what it was meant to check on the same executor types:

- TestBusyWaiting.test_spin_all and test_spin_some
  (test_executors_busy_waiting.cpp) check that spin_all() picks up work
  that becomes ready during the spin and spin_some() does not.
- cancelRacingSpinDoesNotHang and
  cancelBeforeNonBlockingSpinVariantsDoNotHang check cancellation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@ahcorde

ahcorde commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Pulls: #3399
Gist: https://gist.githubusercontent.com/ahcorde/8fd83a7406bd1a5644ec3e25018e0543/raw/4f46ec330097c16bbeca5dc948edbced8de3f217/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/20996

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

ABI Compliance Check

✅ Verdict: compatible

Library Verdict Summary
libcomponent_manager.so ✅ compatible No ABI changes detected.
librclcpp.so ✅ compatible No ABI changes detected.
librclcpp_action.so ✅ compatible No ABI changes detected.
librclcpp_lifecycle.so ✅ compatible No ABI changes detected.
✅ libcomponent_manager.so — full abidiff report

Compared:

  • Base: lib-base/libcomponent_manager.so
  • Head: lib-pr/libcomponent_manager.so @ ec45eff
(empty report — no differences printed by abidiff)
✅ librclcpp.so — full abidiff report

Compared:

  • Base: lib-base/librclcpp.so
  • Head: lib-pr/librclcpp.so @ ec45eff
(empty report — no differences printed by abidiff)
✅ librclcpp_action.so — full abidiff report

Compared:

  • Base: lib-base/librclcpp_action.so
  • Head: lib-pr/librclcpp_action.so @ ec45eff
(empty report — no differences printed by abidiff)
✅ librclcpp_lifecycle.so — full abidiff report

Compared:

  • Base: lib-base/librclcpp_lifecycle.so
  • Head: lib-pr/librclcpp_lifecycle.so @ ec45eff
(empty report — no differences printed by abidiff)

Updated for commit ec45eff · check run · suppressions: /home/runner/work/_temp/ros2-abi-suppressions.txt

@ahcorde
ahcorde merged commit 542de1e into rolling Nov 4, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants