Conversation
test_non_retryable_exhaustion_arms_cooldown captured `before` ahead of three _try_activate_fallback() calls, then asserted the armed cooldown was <= before + 5.0 + 1.0. The cooldown is set relative to the *final* call, and the activation work (agent init, resolve_provider_client) can take >1s on a loaded CI worker — so `before`-anchored upper bound overshot by ~0.5s and failed reliably (slice 8/8, observed twice: 230.21 vs 229.73 bound, 905.80 vs 905.31 bound). Anchor the upper bound to `after = time.monotonic()` captured once the cooldown is armed. cooldown is final_call_time + 5.0 and after >= final_call_time, so cooldown <= after + 5.0 + 1.0 holds regardless of activation latency. Still proves the short 5s window vs the 60s rate-limit one (well under the +50s discriminator in the sibling test). Test added in #53909.
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-assignment |
1 |
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`
✅ Fixed issues (2):
| Rule | Count |
|---|---|
unresolved-attribute |
2 |
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`
run_agent.py:3014: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`
Unchanged: 6087 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean test fix. Anchoring the upper bound to after (captured once the cooldown has actually been armed) rather than before eliminates the wall-clock jitter that caused CI flakes. The rationale in the comment is clear. Note: this PR also includes an infographic PNG, which is fine.
Reviewed by Hermes Agent
|
Superseded by 88b3d86 ( |
Summary
test_non_retryable_exhaustion_arms_cooldownis a freshly-merged flaky test (added in #53909) that fails reliably on loaded CI workers. It capturedbefore = time.monotonic()ahead of three_try_activate_fallback()calls, then asserted the armed cooldown was<= before + 5.0 + 1.0. The cooldown is set relative to the final call, and the activation work (agent init,resolve_provider_client) takes >1s under load — so thebefore-anchored bound overshoots and fails.Anchoring the upper bound to
after(captured once the cooldown is armed) makes the assertion immune to activation latency without weakening what it verifies.Changes
tests/run_agent/test_24996_fallback_exhaustion_cooldown.py: captureafter = time.monotonic()after the activation block; assertcooldown <= after + _FALLBACK_EXHAUSTED_COOLDOWN_S + 1.0.Root cause
cooldown == final_call_time + 5.0, andafter >= final_call_time, socooldown <= after + 5.0 + 1.0holds regardless of how slow the three activation calls are. The oldbefore-anchored bound did not — any activation latency >1s leaked straight into the slack.Validation
assert 230.21 <= 229.73FAIL (observed twice; also905.80 <= 905.31)after, no driftpytest(file)Still proves the short 5s window vs the 60s rate-limit one — far below the
+50sdiscriminator the siblingtest_rate_limit_exhaustion_keeps_60s_cooldownrelies on. Test-only, +7/-1, no logic change.Infographic