test(state): sweep async time.sleep → asyncio.sleep (followup to #73)#76
Conversation
Same hazard as PR #73: sync time.sleep blocks the event loop in async tests, which can flake adjacent tests and mask ordering bugs. - tests/test_memory_state.py: 7 sites - tests/test_state_postgres.py: 9 sites Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces blocking Changes
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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request replaces synchronous time.sleep calls with await asyncio.sleep in asynchronous tests for the memory and Postgres state adapters to improve test hygiene and mitigate flaky-test hazards. Feedback was provided to remove a redundant asyncio.sleep call in tests/test_state_postgres.py where the lock is manually expired, rendering the sleep unnecessary.
| # Third acquire after expiry: should succeed in single query | ||
| mock_pool.executed_queries.clear() | ||
| time.sleep(0.005) | ||
| await asyncio.sleep(0.005) |
There was a problem hiding this comment.
This asyncio.sleep call is redundant in this specific test case. The lock is acquired with a 30-second TTL at line 739, and then it is manually force-expired by directly modifying the mock pool's state at line 759. The 5ms sleep does not contribute to the expiration logic and can be safely removed to improve test efficiency.
There was a problem hiding this comment.
Addressed in 7883a73 — the sleep was mechanical leftover from the time.sleep sweep; force-expiry on the next line makes it redundant. Thanks.
…gres test The lock is force-expired on the next line by directly setting expires_at into the past — the 5ms sleep contributed nothing. Flagged by gemini on PR #76. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
tests/test_state_redis.py. Identical synctime.sleepcalls remained in the MemoryState and PostgresState test suites — sweeping them here.tests/test_memory_state.py, 9 intests/test_state_postgres.py. Each was inside a@pytest.mark.asyncio async def test_*method, sotime.sleep(X)→await asyncio.sleep(X)is a direct, semantics-preserving swap.import asyncioadded (alphabetized) to both files.import timekept — both files still usetime.time()for millisecond timestamps.Why it matters
Sync
time.sleepblocks the event loop in an async test, which can flake adjacent tests and mask ordering bugs. Same rationale as #73; no test-level behavior change is intended.Test plan
uv run ruff check src/ tests/ scripts/uv run ruff format --check src/ tests/ scripts/uv run pyrefly check(0 errors)uv run python scripts/audit_test_quality.py(0 hard failures; pre-existing duplicate-test warnings only)uv run python scripts/verify_test_fidelity.py(no regression; 40 pre-existing missing)uv run pytest tests/test_memory_state.py tests/test_state_postgres.py --tb=short -q— 105 passeduv run pytest tests/ --tb=short -q— 3545 passed, 2 skippedNotes
## Unreleased(created) for0.4.26.2.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation