Add WebSocket hub lifecycle integration tests#39
Conversation
📝 WalkthroughWalkthroughThis PR adds a new test file, market/ws/server_test.go, containing unit tests for the ws.Hub type. It includes helper functions for logger/client setup and polling, and five test functions covering registration lifecycle, broadcast fanout, slow-client eviction, unknown-client unregistration safety, and concurrent register/unregister operations. ChangesHub Test Suite
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
market/ws/server_test.go (3)
142-152: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
time.SleepwithwaitUntilfor consistency and reliability.Line 148 uses
time.Sleep(30 * time.Millisecond)to wait for the unregister to be processed. This is inherently flaky. Since the expected state is simplyclientCount(h) == 0,waitUntilis a direct fit.♻️ Proposed refactor
- h.unregister <- ghost - time.Sleep(30 * time.Millisecond) - if clientCount(h) != 0 { - t.Fatalf("ghost unregister mutated client set: count=%d", clientCount(h)) - } + h.unregister <- ghost + waitUntil(t, func() bool { return clientCount(h) == 0 }, 200*time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@market/ws/server_test.go` around lines 142 - 152, The test TestHub_UnregisterUnknownClientIsSafe uses a fixed sleep to wait for unregister processing, which is flaky and inconsistent with the rest of the suite. Replace the time.Sleep call with waitUntil in this test, using the expected clientCount(h) == 0 condition after sending on h.unregister, so the check waits deterministically for Hub state to settle.
154-174: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
time.SleepwithwaitUntilfor the final hub-empty assertion.Line 170 uses
time.Sleep(80 * time.Millisecond)to wait for all unregister operations to drain. This is the same pattern already abstracted bywaitUntiland is susceptible to flakiness under load.♻️ Proposed refactor
wg.Wait() - time.Sleep(80 * time.Millisecond) - if n := clientCount(h); n != 0 { - t.Fatalf("expected empty hub after churn, got %d", n) - } + waitUntil(t, func() bool { return clientCount(h) == 0 }, 300*time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@market/ws/server_test.go` around lines 154 - 174, The final hub-empty check in TestHub_ConcurrentRegisterUnregister is relying on a fixed time.Sleep, which can be flaky under load. Replace the sleep-based wait with the existing waitUntil helper so the test waits for clientCount(h) to reach zero before asserting, using TestHub_ConcurrentRegisterUnregister and clientCount as the key symbols to update.
105-140: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
time.SleepwithwaitUntilpolling to reduce flakiness.Line 118 uses a fixed
time.Sleep(50 * time.Millisecond)to wait for broadcast processing. On slow CI runners this may not be enough, and on fast machines it wastes time. The rest of the file already useswaitUntilfor the same pattern. Consider polling for the expected state (slow removed, fast present) instead.♻️ Proposed refactor
- h.broadcast <- []byte("pressure") - time.Sleep(50 * time.Millisecond) + h.broadcast <- []byte("pressure") + waitUntil(t, func() bool { + h.mu.RLock() + _, slowPresent := h.clients[slow] + _, fastPresent := h.clients[fast] + h.mu.RUnlock() + return !slowPresent && fastPresent + }, 300*time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@market/ws/server_test.go` around lines 105 - 140, The broadcast eviction test uses a fixed sleep to wait for Hub cleanup, which is flaky and unnecessary. In TestHub_BroadcastEvictsSlowClient, replace the time.Sleep-based pause after h.broadcast with waitUntil polling for the expected Hub state, using h.clients and the slow/fast client presence checks already in the test. Keep the assertions on slow client removal, fast client retention, and closed send channel, but only run them after the polling condition is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@market/ws/server_test.go`:
- Around line 132-139: The slow client closure assertion in the broadcast test
is checking `slow.send` before draining its buffered prefill message, so the
receive can still վերադարձ `ok=true` even after close. Update the test around
`slow.send` to first consume the buffered item (the prefilled message) and then
perform the closed-channel check, using the existing `slow.send` setup and
broadcast flow in the same test to verify the channel is actually closed.
---
Nitpick comments:
In `@market/ws/server_test.go`:
- Around line 142-152: The test TestHub_UnregisterUnknownClientIsSafe uses a
fixed sleep to wait for unregister processing, which is flaky and inconsistent
with the rest of the suite. Replace the time.Sleep call with waitUntil in this
test, using the expected clientCount(h) == 0 condition after sending on
h.unregister, so the check waits deterministically for Hub state to settle.
- Around line 154-174: The final hub-empty check in
TestHub_ConcurrentRegisterUnregister is relying on a fixed time.Sleep, which can
be flaky under load. Replace the sleep-based wait with the existing waitUntil
helper so the test waits for clientCount(h) to reach zero before asserting,
using TestHub_ConcurrentRegisterUnregister and clientCount as the key symbols to
update.
- Around line 105-140: The broadcast eviction test uses a fixed sleep to wait
for Hub cleanup, which is flaky and unnecessary. In
TestHub_BroadcastEvictsSlowClient, replace the time.Sleep-based pause after
h.broadcast with waitUntil polling for the expected Hub state, using h.clients
and the slow/fast client presence checks already in the test. Keep the
assertions on slow client removal, fast client retention, and closed send
channel, but only run them after the polling condition is satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| select { | ||
| case _, ok := <-slow.send: | ||
| if ok { | ||
| t.Fatal("slow client send channel should be closed") | ||
| } | ||
| default: | ||
| t.Fatal("expected closed slow send channel") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Slow client send channel check will fail due to buffered "prefill" message.
The slow client's buffer (size 1) is pre-filled with "prefill" at line 110. When broadcast closes slow.send, the buffered item remains readable. A receive on a closed channel with pending items returns the item with ok=true, so the check at line 133-135 will hit t.Fatal("slow client send channel should be closed") instead of confirming closure.
🐛 Proposed fix: drain the channel before checking closure
select {
- case _, ok := <-slow.send:
- if ok {
- t.Fatal("slow client send channel should be closed")
- }
- default:
- t.Fatal("expected closed slow send channel")
+ case v, ok := <-slow.send:
+ if ok && string(v) != "prefill" {
+ t.Fatal("unexpected buffered message in slow send channel")
+ }
+ if !ok {
+ break
+ }
+ // drain remaining buffered item, next read should confirm closure
+ if _, ok := <-slow.send; ok {
+ t.Fatal("slow client send channel should be closed after draining")
+ }
+ default:
+ t.Fatal("expected closed slow send channel")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| select { | |
| case _, ok := <-slow.send: | |
| if ok { | |
| t.Fatal("slow client send channel should be closed") | |
| } | |
| default: | |
| t.Fatal("expected closed slow send channel") | |
| } | |
| select { | |
| case v, ok := <-slow.send: | |
| if ok && string(v) != "prefill" { | |
| t.Fatal("unexpected buffered message in slow send channel") | |
| } | |
| if !ok { | |
| break | |
| } | |
| // drain remaining buffered item, next read should confirm closure | |
| if _, ok := <-slow.send; ok { | |
| t.Fatal("slow client send channel should be closed after draining") | |
| } | |
| default: | |
| t.Fatal("expected closed slow send channel") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@market/ws/server_test.go` around lines 132 - 139, The slow client closure
assertion in the broadcast test is checking `slow.send` before draining its
buffered prefill message, so the receive can still վերադարձ `ok=true` even after
close. Update the test around `slow.send` to first consume the buffered item
(the prefilled message) and then perform the closed-channel check, using the
existing `slow.send` setup and broadcast flow in the same test to verify the
channel is actually closed.
1 participant
Bounty delivery — TentOfTrials #1 ($45)
Issue: #1
Deliverable:
market/ws/server_test.go(WebSocket hub lifecycle tests)Win-EV: $45 · 11 comments · harness rank #7
What ships
Go hub lifecycle integration tests covering:
register/unregister— client count transitions + send channel close on disconnectbroadcastfan-out — all connected clients receive payloadsendbuffer triggers cleanup during broadcast (RLock path)Fire
python runtime/external_exec_kernel.py tick --fire # or target this deliverable: python runtime/github_pr_stage.py bounty TentOfTrials_1 --fireσ: tests drafted from live
market/ws/server.go· payout requires maintainer merge.Summary by CodeRabbit