Repository navigation
tests: test_connect_all_parallel_execution asserts a 0.2s wall-clock threshold with ~2ms of real margin #226
Description
tests/test_client_manager.py::TestParallelConnections::test_connect_all_parallel_execution proves that connect_all runs connections in parallel by mocking _connect_server with await asyncio.sleep(0.1), running three of them, and asserting:
assert elapsed < 0.2, f"Expected parallel execution, took {elapsed}s"Parallel is ~0.1 s and serial would be ~0.3 s, so the threshold sits midway — but the slack over parallel is only ~100 ms, and that has to absorb coverage instrumentation (--cov in CI), event-loop scheduling, and a shared runner. It does not, reliably. Two consecutive test (3.10) failures on PR #225, whose diff touches only exception handlers:
| run | elapsed | margin |
|---|---|---|
| 33718867432 | 0.22305679321289062 s | 23 ms over |
| 33727450531 | 0.2019734382629... s | 2 ms over |
Measured 10 consecutive local runs on both that PR's branch and main: 10/10 pass on each, so this is environment sensitivity, not a regression — the test is simply calibrated too tightly to survive a loaded runner.
Why it is worth fixing rather than re-running: a two-millisecond margin makes the test a coin flip under load, and it fails in a way that looks like the PR under review broke something. It cost two review cycles on #225 alone.
Suggested fix — assert the property directly instead of via wall clock. The test already records call_times; concurrency is provable from them without any threshold on total elapsed:
assert max(call_times) - min(call_times) < 0.05 # all three started togetherThat is what "parallel" actually means here, and it does not tighten as the machine slows down. If a wall-clock bound is still wanted as a backstop, < 0.25 keeps the serial case (~0.3 s) excluded with 2.5x the current margin.
Found while working #224; not caused by it.
Activity
ViperJuice commentedon Sep 5, 2026
Folded into #235, which covers the same class across the suite: non-hermetic tests, 65 real wall-clock sleeps, unreset module caches, and a test reaching the npm registry.
Keeping this open as the concrete instance — it has the measurements (two CI failures at 23 ms and 2 ms over a 0.2 s threshold, and 10/10 local passes on both branches) and a specific fix that asserts concurrency from the recorded
call_timesrather than from total elapsed. Whoever takes #235 should do this one first: it is the cheapest, and it is the only one of the group that has already cost review cycles on unrelated PRs.chore(plan): rev 2 of the slice C1 timing plan after panel review (see …
chore(plan): deterministic test timing, slice C1 of #235 (see #235, see
chore(plan): test cwd/filesystem isolation — slice A of #235 (see #261 …
test: deterministic timing — slice C1 of #235, fixes the #226 flake (#…
ViperJuice commentedon Sep 21, 2026
Closing evidence (this issue was auto-closed by the merge commit; recording the verification here).
Fixed by #268 (slice C1 of #235).
test_connect_all_parallel_executionno longer measures wall-clock time at all — each of the threemock_connectbodies passes aRendezvous(3)released only when the third arrives, so "parallel" is asserted as the property it actually is rather than inferred from a margin that shrinks as the runner slows.The fix suggested in this issue —
max(call_times) - min(call_times) < 0.05— was deliberately not adopted: it is still a wall-clock upper bound, so it has the same failure mode under load, just with a different constant.Verified:
--cov=pmcp, the instrumentation that widened the original margin past 0.2 s._connect_all_unlocked(each task awaited before the next is created) fails the test in ~4.3 s, inside its 5 s hang guard, withFailed to connect to …: only 1 of 3 parties were in flight together— the regression is still caught, and now names itself.The same PR removed the other ten
elapsed < Xupper bounds across the suite and addedtests/_timing.pyplus a CONTRIBUTING rule, so the class does not come back by hand.test: replace the wall-clock windows with property proofs (slice C2c of