Fix flaky spline interpolation test - #556
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #556 +/- ##
==========================================
- Coverage 80.46% 80.27% -0.19%
==========================================
Files 116 116
Lines 6976 6975 -1
Branches 3083 3083
==========================================
- Hits 5613 5599 -14
- Misses 984 992 +8
- Partials 379 384 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
Improves spline interpolation integration-test reliability by monitoring trajectory startup concurrently with upload.
Changes:
- Adds asynchronous trajectory-start confirmation.
- Replaces separate send/wait calls with the combined helper.
- Adds synchronization state for the watcher.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:332
- Correct the spelling of “Continously” to “Continuously”.
// Continously check spline travel time
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:333
- Correct the spelling of “Continously” in this new comment.
// Continously check spline travel time
| // Wait for trajectory to start (should be quick) | ||
| if (confirm_future.wait_for(std::chrono::milliseconds(500)) != std::future_status::ready) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/test_spline_interpolation.cpp:217
- This 500 ms deadline preempts the helper's own 1-second start/reset timeouts, so a valid start that takes 500–1000 ms is forcibly stopped and reported as a failure. That shortens the previous test tolerance and can preserve the flakiness this change is intended to remove; make the outer deadline longer than the internal timeout.
if (confirm_future.wait_for(std::chrono::milliseconds(500)) != std::future_status::ready)
tests/test_spline_interpolation.cpp:336
- Correct the spelling of “Continously” to “Continuously.”
// Continously check spline travel time
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/test_spline_interpolation.cpp:336
- Correct the spelling of “Continuously.”
// Continously check spline travel time
tests/test_spline_interpolation.cpp:342
- This reset can be observed before
sendTrajectory()has sent the newTRAJECTORY_START. Moreover,splineTimerTraveledresets for every spline segment (resources/external_control.urscript:402-404), so an existing trajectory crossing a segment boundary can set this flag and later satisfy the> 0.002check even if the new trajectory was not accepted. Use a separate signal set immediately after sending the new START command, and only treat resets observed after that signal as confirmation;trajectory_sentis too late because it is set after all points are uploaded.
if (spline_travel_time < start_spline_travel_time || spline_travel_time == 0.0)
{
spline_travel_time_reset = true;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:350
- When the register is initially
0.0,spline_travel_time_resetbecomes true before the upload starts, so this worker may send a NOOP concurrently withsendTrajectory()sending its NOOP/START messages. The reverse socket write path does not serialize concurrent writes, which can reorder or interleave control frames and reintroduce flakiness. Gate this keepalive on the existingtrajectory_sentflag (or otherwise serialize all reverse-interface writes).
if (!g_my_robot->getUrDriver()->writeTrajectoryControlMessage(
urcl::control::TrajectoryControlMessage::TRAJECTORY_NOOP))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:327
- When the pre-send travel time is already
0.0(notably on the first test after startup), the worker only considers the reset observed if it receives another zero-valued RTDE package after notifying the sender. If the sender starts the spline before that next package, the first value can already be positive, so the loop never sets this flag and falsely times out. Treat the zero baseline already sampled immediately before signaling readiness as the reset state, then wait for the timer to advance.
bool spline_travel_time_reset = false;
| } | ||
|
|
||
| // Confirm that the new trajectory is running | ||
| if (std::abs(spline_travel_time) > start_spline_travel_time) |
There was a problem hiding this comment.
Wouldn't this be problematic when there was a long trajectory running beforehand? I don't remember by hard when we reset the spline_travel_time, but it seems fragile to rely on it being small in the beginning. Once it has been reset, being larger than a reasonable threshold should be sufficient, right?
There was a problem hiding this comment.
It only gets to this stage if we have seen that it is either zero, or at least has become smaller (where start_spline_travel_time is reset), and then we check that it advances one timestep.
| waitForTrajectoryStarted(); | ||
| ASSERT_TRUE(sendTrajectoryAndConfirmStart(s_pos, s_vel, std::vector<urcl::vector6d_t>(), s_time)); | ||
|
|
||
| g_trajectory_running = true; |
There was a problem hiding this comment.
We could probably set this inside sendTrajectoryAndConfirmStart, right?
There was a problem hiding this comment.
Yes, I think we could
Made new method: sendTrajectoryAndConfirmStart which launches an async thread to confirm that a new trajectory resets the spline_travel_time and then starts executing. This is more robust than the previous implementation. I have replaced all the calls to sendTrajectory where they were immediately followed by waitForTrajectoryStarted, with one call to sendTrajectoryAndConfirmStart. waitForTrajectoryStarted is no longer used, but I havent removed it yet.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Also add debug messages when failing Also make reverse socket read blocking
4e94127 to
4704c98
Compare
| bool trajectory_running = confirm_future.get(); | ||
| g_trajectory_running = trajectory_running; |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 4704c98. Configure here.
| { | ||
| g_trajectory_running = true; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fragile travel-time start detection
Medium Severity
confirmTrajectoryStarted treats a new trajectory as started only after spline_travel_time drops below the sampled baseline (or hits exactly 0.0), then advances past that updated baseline. jointSplineRun increments before the first register write, so a new segment often starts at about one step time, never writing 0.0. If a prior trajectory was only briefly running and the baseline is already near that same small value, the drop is never observed and confirmation times out. That is especially likely for back-to-back sendTrajectoryAndConfirmStart calls.
Reviewed by Cursor Bugbot for commit 4704c98. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_spline_interpolation.cpp:346
- This can confirm the old trajectory instead of the one just sent. The monitor starts before
TRAJECTORY_START, andspline_travel_timeresets at every spline segment, so a segment boundary in an already-running trajectory can satisfy this reset check and the subsequent increase can returntruebefore the new start message is processed. Gate the reset/start state machine on an explicit signal emitted immediately after this helper successfully writes the newTRAJECTORY_STARTmessage;trajectory_sentcurrently only controls NOOP writes and does not prevent the false positive.
// Confirm that a new trajectory was transferred (Travel time = 0.0 or lower than it was before)
if (spline_travel_time < start_spline_travel_time || spline_travel_time == 0.0)
Made new method: sendTrajectoryAndConfirmStart which launches an async thread to confirm that a new trajectory resets the spline_travel_time and then starts executing. This is more robust than the previous implementation.
I have replaced all the calls to sendTrajectory where they were immediately followed by waitForTrajectoryStarted, with one call to sendTrajectoryAndConfirmStart. waitForTrajectoryStarted is no longer used, but I havent removed it yet.
Note
Low Risk
Test-only changes in integration tests; no production library behavior modified beyond how tests call existing driver APIs.
Overview
Hardens spline integration tests so they wait for a new trajectory to be accepted and actually running, not just a momentary zero in
spline_travel_time.Adds
sendTrajectoryAndConfirmStart, which starts an asyncconfirmTrajectoryStartedloop on RTDEoutput_double_register_1before sending points. That thread checks that travel time resets (drops or goes to zero), then that it increases again after upload—sending TRAJECTORY_NOOP from the background thread while the main thread streams the spline. Tests that usedsendTrajectory+waitForTrajectoryStartednowASSERT_TRUE(sendTrajectoryAndConfirmStart(...))and setg_trajectory_runningfrom the helper’s result.sendTrajectorynow passesRobotReceiveTimeout::off()onTRAJECTORY_START.waitForTrajectoryStartedremains in the file but is unused; its start check was tightened tostd::abs(spline_travel_time) < 0.01.Reviewed by Cursor Bugbot for commit 12d787e. Bugbot is set up for automated code reviews on this repo. Configure here.