fix(test): test get_update_result timeout behavior, not result-value identity

My previous attempt (patching check_for_updates) still lost the race:
the background update-check thread captures check_for_updates via
global lookup at call time, but on CI the thread was already past that
point (mid-git-fetch) by the time the test's patch took effect.  The
real fetch returned 4954 commits-behind and wrote that to
banner._update_result before the test's assertion ran.

Fix: test what we actually care about — that get_update_result respects
its timeout parameter — and drop the asserting-on-result-value that
races with legitimate background activity.  The get_update_result
function's job is to return after `timeout` seconds if the event isn't
set.  The value of `_update_result` is incidental to that test.

Validation: tests/hermes_cli/test_update_check.py now 9/9 pass under
CI-parity env, and the test no longer has a correctness dependency on
module-level state that other threads can write.
This commit is contained in:
Teknium 2026-04-19 19:04:57 -07:00 committed by Teknium
parent ad4680cf74
commit b2f8e231dd

View file

@ -114,30 +114,30 @@ def test_prefetch_non_blocking():
def test_get_update_result_timeout():
"""get_update_result() returns None when check hasn't completed within timeout.
"""get_update_result() waits up to ``timeout`` seconds and returns.
Race protection: a background update-check thread from an earlier
test, or from hermes_cli.main's own prefetch_update_check(), could
write to module-level ``_update_result`` during this test's
``wait(0.1)``. Observed on CI: a real git-fetch returned 4950
commits-behind mid-test, failing ``assert 4950 is None``. Patching
``check_for_updates`` for the duration of the test ensures any
in-flight thread writes ``None`` rather than a real fetch result.
The original assertion that the return value is ``None`` races on
CI: a background update-check thread (from hermes_cli.main's
prefetch_update_check() or an earlier test in the same xdist worker)
can finish a real ``git fetch`` mid-test and write a genuine commits-
behind count into module-level ``banner._update_result`` (observed:
4950, 4954). The behavior we actually care about here is that
``get_update_result`` respects its ``timeout`` blocking calls to
``Event.wait()`` should return after the timeout even when the event
is never set. Test that directly.
"""
import hermes_cli.banner as banner
with patch.object(banner, "check_for_updates", return_value=None):
# Fresh Event so we hit the timeout branch deterministically.
banner._update_result = None
banner._update_check_done = threading.Event()
# Fresh Event so we hit the timeout branch deterministically.
banner._update_check_done = threading.Event()
start = time.monotonic()
result = banner.get_update_result(timeout=0.1)
elapsed = time.monotonic() - start
start = time.monotonic()
banner.get_update_result(timeout=0.1)
elapsed = time.monotonic() - start
# Should have waited ~0.1s and returned None
assert result is None
assert elapsed < 0.5
# Waited at least the timeout, but returned well before a "real" wait
# would have (the default 5s a fully-blocking call would imply).
assert 0.05 < elapsed < 0.5
def test_invalidate_update_cache_clears_all_profiles(tmp_path):