tests: fix flaky test_conflict_resolution.cpp fixed-sleep race - #83
Merged
Conversation
…a poll on notifyCount Pre-existing, unrelated to any open PR (present since #17): each test called bridge.switchBackend() then slept a fixed 80ms hoping onBackendChanged() (which drains the offline queue on the new backend's thread pool) had finished by then, before asserting on the queue/notifyCount. Under contention -- a coverage-instrumented build, a busy CI runner, Valgrind's own overhead -- 80ms isn't always enough, so REQUIRE(queue.drain().empty()) intermittently sees a queue that hasn't drained yet. Confirmed reproducing on 3 independent CI jobs (Linux clang-debug, Linux Qt6-WebSockets, Valgrind memcheck) and, separately, in a local WSL coverage build. Adds waitForBackendChanged(), which polls the model's own notifyCount (via the existing read-only OrderQueryAction, already used by these tests) until it reaches 1 -- the real signal that onBackendChanged has run, and therefore that its queue drain has too -- instead of guessing a fixed delay. Bounded at 2 seconds, matching waitInt's own budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pre-existing flaky test, unrelated to any currently-open PR (present since #17). Every test in
test_conflict_resolution.cppcalledbridge.switchBackend()then slept a fixed80mshopingonBackendChanged()(which asynchronously drains the offline queue on the new backend's thread pool) had finished by then, before asserting on the queue/notifyCount. Under contention — a coverage-instrumented build, a busy CI runner, Valgrind's own overhead — 80ms isn't always enough.Confirmed reproducing on 3 independent CI jobs while investigating an unrelated PR's CI (Linux clang-debug, Linux Qt6-WebSockets, Valgrind memcheck), and separately in a local WSL coverage build.
Fix
Replaces all 5 fixed
sleep_for(80ms)sites withwaitForBackendChanged(), which polls the model's ownnotifyCount(via the existing read-onlyOrderQueryAction, already used by these tests) until it reaches the expected value — the real signal thatonBackendChangedhas run, and therefore that its queue drain has too — instead of guessing a fixed delay. Bounded at 2 seconds, matching the existingwaitInthelper's own budget, so a genuine regression still fails fast rather than hanging.Verification
[conflict]tests run 5x back-to-back, all green.🤖 Generated with Claude Code