fix(tests): make integration tests pass on macOS#1091
Conversation
|
Warning Review limit reached
More reviews will be available in 8 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTests updated to wait deterministically: server-context tests spin until ChangesIntegration test synchronization and message waiting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/integration_tests/test_servercontext.py`:
- Around line 94-95: The unbounded polling `while not ctx.connections: await
asyncio.sleep(0)` can hang tests; update both occurrences (around the blocks at
lines referencing ctx.connections) to use a bounded wait with a timeout (e.g.,
record a deadline or use asyncio.wait_for) and fail fast when the timeout
elapses (raise an AssertionError or call pytest.fail with a clear message
indicating server registration did not occur within the timeout). Ensure you
apply the same change to both occurrences so the test will time out
deterministically rather than hanging indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb1d217b-1e55-49e2-9293-d6cea5a313da
📒 Files selected for processing (1)
tests/integration_tests/test_servercontext.py
|
Addressed CodeRabbit's actionable comment in 5c2831e — both The remaining lint failures are pre-existing on
Happy to address either in a follow-up if you'd like them bundled. |
On macOS with KqueueSelector, `asyncio.open_connection` returns before the server-side accept callback registers the connection in `ctx.connections`. Three tests in test_servercontext.py raced this scheduling difference and failed deterministically on macOS while passing on Linux CI. - test_unexpected_exception: yield via exhaust_callbacks so the server task emits the "Exception in protocol" log before caplog is checked. - test_drain_connections / test_connection_broken_external: poll ctx.connections until populated before depending on it. exhaust_callbacks cannot be used inside @fast_forward — the outer ticker keeps rescheduling itself and the inner exhaust never exits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`matchmaker_info` is broadcast by the periodic `report_dirties` ticker. On macOS the first broadcast after `game_matchmaking` could be the startup one (queue dirty but no searcher → empty boundary), making the [[300, 700]] assertion fail. Wait for a matchmaker_info where the tmm2v2 queue actually carries a non-empty boundary before asserting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address CodeRabbit feedback: replace `while not ctx.connections` spin loops with a helper that polls up to a fixed iteration count and raises AssertionError on exhaustion, so tests fail fast instead of hanging if the server never registers the connection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5c2831e to
23bf026
Compare
Summary
On macOS with
KqueueSelector,asyncio.open_connection(...)returns before the server-side accept callback has registered the new connection inctx.connections. Three tests intests/integration_tests/test_servercontext.pyrace this scheduling difference — they pass on Linux CI but fail deterministically on macOS.test_unexpected_exception— addedawait exhaust_callbacks()so the server task emits the "Exception in protocol" log beforecaplog.textis asserted.test_drain_connections— pollctx.connectionsuntil populated before callingdrain_connections. Cannot useexhaust_callbacksinside@fast_forward: the outer ticker keeps rescheduling itself and the inner exhaust never exits.test_connection_broken_external— same poll, otherwisenext(iter(ctx.connections.values()))raisedStopIteration.No production code is changed. Full integration suite (
tests/integration_tests) goes from 3 fail / 323 pass to 326 pass on macOS (Darwin 24.6.0, Python 3.13). Linux behavior is unchanged — the added waits are no-ops when the server task has already run.Test plan
pipenv run py.test tests/integration_tests --mysql_database=fafpasses on macOS🤖 Generated with Claude Code
Summary by CodeRabbit