Skip to content

Test hygiene: no 15 s sleep in the sync test, bounded polls instead of fixed sleeps, test doubles out of the package - #336

Merged
renmengye merged 3 commits into
mainfrom
chore/test-hygiene
Sep 8, 2026
Merged

renmengye merged 3 commits into
mainfrom
chore/test-hygiene

Conversation

@renmengye

Copy link
Copy Markdown
Member

Digest #332, grouping 3 (test hygiene), the mechanical part. No behaviour change in the kernel.

  • The sync test spent 15 real seconds because cmd_sync polled with a hard time.sleep(15); the interval is now the module constant SYNC_POLL_S and the test shrinks it. 15.01 s → 0.31 s.
  • Fixed sleeps around real subprocesses become bounded polls on the condition they were waiting for (tests/helpers.py::wait_until): the sweep tests poll for the git process being live the way the sweep script itself looks for it; the harness and local-compute tests poll for the child to be gone; the syscall-CLI and watcher tests poll for their markers.
  • The two test doubles that lived in the package, FakeHarness and RecordingDispatcher, move to tests/fakes.py; nothing under src/ or scripts/ used them.

Left for later, deliberately: consolidating the per-file FakePopen / FakeMeasurer / _git helpers (a larger refactor with more room for semantic drift).

🤖 Generated with Claude Code

renmengye and others added 2 commits September 7, 2026 22:44
…f fixed sleeps, test doubles out of src

- syscall_cli.cmd_sync polls on a module constant SYNC_POLL_S (still 15 s);
  test_sync_cli_waits_for_the_done_marker shrinks it instead of paying it
  (15.01 s -> 0.31 s). CLI behaviour unchanged.
- tests/helpers.py: wait_until(predicate, timeout, interval). Fixed sleeps
  and hand-rolled deadline loops around real subprocesses now poll the
  condition they wait for: test_sweep_git_locks (git visible in the process
  table as the sweep sees it), test_harness and test_local_compute (killed
  child gone), test_syscall_cli (request marker seen), test_watcher (done
  marker written).
- tests/fakes.py: FakeHarness and RecordingDispatcher moved out of
  src/outerloop/harness.py and src/outerloop/tick.py; nothing in src or
  scripts used them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 1 — reviewed head 02e4627a — reviewer summarizer:hermes/gpt-5.6-terra over coverage+credentials+deployment+general+lifecycle+prose.

terra
Advisory findings from outerloop — the code owner decides. Reply to disagree; the outerloop:no-review label opts this PR out.

Verdict: 1 blocking, 2 advisory.

3 findings attached to the lines below.

Verdict: blocking — the new live-process readiness wait times out in two serial sweep tests, causing them to fail before exercising the sweep. Also, the sync test lacks coverage proving the poll interval is configurable. Rejected findings: none; the six reports of the two serial test failures were merged because they make the same claim about the same readiness assertion.

Comment thread tests/test_sweep_git_locks.py
Comment thread tests/test_syscall.py
Comment thread tests/helpers.py Outdated
…sweep does; the sync test times itself

- _live no longer depends on pgrep plus ps agreeing: it checks the child is
  alive and reads its cmdline the way the sweep does (/proc on Linux, ps on
  a Mac), so the test's readiness and the sweep's detection never diverge.
- the sync test asserts it returns well under one default interval, proving
  the shrunk SYNC_POLL_S is actually used and not just that a marker arrived.
- wait_until's docstring says a timeout fails the test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@renmengye renmengye added the outerloop:review re-request the advisory review label Sep 8, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 1 — reviewed head 37c2f898 — reviewer hermes/gpt-5.6-terra.

terra
Advisory findings from outerloop — the code owner decides. Reply to disagree; the outerloop:no-review label opts this PR out.

Verdict: no defects found.

@renmengye
renmengye merged commit dba0f7d into main Sep 8, 2026
5 checks passed
@renmengye
renmengye deleted the chore/test-hygiene branch September 8, 2026 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

outerloop:review re-request the advisory review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant