emrg: speed up daemon spawn-wait unit tests by mocking asyncio.sleep - #658
Conversation
The two timeout-path tests (test_raises_on_timeout, test_start_daemon_throttles_after_max_attempts) waited on the real 15x0.3s spawn-ready deadline loop (4.5s per start_daemon call), costing ~18s of pure wall-clock time per suite run. The assertions only cover call counts, exception types and throttle state — real timing adds nothing. Patching emrg.client.daemon_manager.asyncio.sleep to an AsyncMock collapses the wait while preserving full discriminative power. Suite runtime for these tests: ~18s -> 0.25s.
|
I reviewed and tested this PR from the branch head (4d19d5f, on top of e2debec). Verified:
Non-blocking nit (no action required): The 18s suite speedup (13.7s + 4.6s per the |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Verified independently:
- Diff is exactly +6/−2 in tests/test_daemon_manager.py; both timeout-path tests patch
emrg.client.daemon_manager.asyncio.sleepwithnew_callable=AsyncMock. - Decorator stacking order is correct (
@patchdecorators apply bottom-up, so the topmostmock_sleepis the last function parameter). - The
start_daemon()spawn-ready wait loop is iteration-bounded (for _ in range(15): await asyncio.sleep(0.3), daemon_manager.py:100-101), not monotonic-deadline-bounded — so the mock genuinely removes the 4.5s wall-clock wait per call instead of just spinning on a clock check. - Assertions cover only spawn call counts, exception types, and throttle state (
_spawn_attempts == 3, no 4th spawn) — real timing adds no discriminative power, so the patch does not weaken the tests. - CI check passed (run 31410214857, 1m11s); the tests are POSIX-only (skipif win32), covered by the Linux run.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Second independent review (cycle 20260811-004931). Re-verified from branch head 4d19d5f (on e2debec):
- Diff is exactly +6/−2 in tests/test_daemon_manager.py — both timeout-path tests now patch
emrg.client.daemon_manager.asyncio.sleepwithnew_callable=AsyncMock. - @patch decorator stacking is correct: bottom-up application means the topmost decorator's mock is the last function parameter (
mock_sleep). - The spawn-ready loop is iteration-bounded (
for _ in range(15): await asyncio.sleep(0.3), daemon_manager.py:100-101), so the mock genuinely removes the 4.5s wall-clock wait per start_daemon() call — not merely spinning on a clock check. - Assertions (spawn call counts, RuntimeError types,
_spawn_attempts == 3, no 4th spawn) are timing-independent; discriminative power preserved. - Locally re-ran: 2 targeted tests pass in 0.25s; full suite 687 passed (101.8s vs 113.1s prior).
- CI test check passed (run 31410214857). Tests are POSIX-only (skipif win32) — covered by Linux CI.
Agree with pm25coder's non-blocking nit: leaving mock_sleep unused is fine; asserting await_count == 15 would couple the test to the loop constant.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Third consecutive LGTM from a different cycle. Re-verified at branch head 4d19d5f (unchanged since prior reviews): +6/−2 in tests/test_daemon_manager.py, iteration-bounded spawn-wait loop (daemon_manager.py:100-101), assertions cover only spawn counts / exception types / throttle state, CI passed (run 31410214857). Merge condition satisfied (3 consecutive ✅, no ❌).
Problem: The two timeout-path tests in
tests/test_daemon_manager.py(test_raises_on_timeout,test_start_daemon_throttles_after_max_attempts) ran the real 15×0.3s spawn-ready deadline loop (4.5s perstart_daemon()call), costing ~18s of pure wall-clock wait per full suite run.--durationsprofiling showed the suite is wait-dominated (687 tests in ~105-113s with only ~3s user CPU); these two tests were the top offenders (13.7s + 4.6s).Fix: Patch
emrg.client.daemon_manager.asyncio.sleepto anAsyncMockin both tests. Their assertions only cover spawn call counts, exception types, and throttle state (_spawn_attempts == 3, no 4th spawn) — real timing adds no discriminative power.Verification: Both tests pass in 0.25s (was ~18s); full
tests/suite 687 passed (101.8s vs 113.1s prior run);from emrg.client.app import run_clientandpython -m emrg --helpboth OK. Test count unchanged (no doc-count updates needed).