Skip to content

fix(test): drive follow-through waits from the mocked clock (#821) - #833

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:fix/821-follow-through-mocked-clock
Oct 23, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:fix/821-follow-through-mocked-clock

Conversation

@HugoXK

@HugoXK HugoXK commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

fix(test): drive follow-through waits from the mocked clock (#821)

Fixes #821

Tip SHA: 0f0006f

What changed

core/test/follow-through.test.js waited on setImmediate against a wall-clock 3 s deadline while the case mocked setTimeout; a wait that needs a tick can never progress that way, so a loaded runner failed it as condition not reached on a green PR (Test run 39917377405 attempt 1, head f027324e) that passed on rerun.

  • one mockTimers(t) helper enables and registers the mocked clock; every settle advances it in bounded rounds, so no wait in the file hangs on wall-clock and the next case inherits the safe wait
  • the cleared-note wait in the failing case no longer asserts inside the helper: the following assert.equal judges the write, so removing the clearing write fails on "the exchange a turn took up is still noted on disk"

patches/ds-bot/0001-core-fixes.patch regenerated with node scripts/ds-bot-patches.mjs generate.

Ran

  • TZ=UTC npm run verify at the root: pass
  • node --test test/follow-through.test.js ×100: 100/100 green
  • negative control: removed forgetQueued(...) from core/src/host/resume.js; the case failed on its own assertion, then restored

Not run

  • CI and the merge queue (GitHub); no smoke test (no index.js or src/host change in the diff)

…wLab#821)

`settle` spun on setImmediate against a wall-clock 3 s deadline while the
case mocked `setTimeout`, so a wait that needed a tick could never progress
on a loaded runner and reported "condition not reached" for a green PR
(Test run 39917377405 attempt 1, head f027324) that passed on rerun.

One `mockTimers(t)` helper now enables the mocked clock and registers it, and
`settle` advances that clock in bounded rounds, so no wait in the file races
wall time. The cleared-note wait in the failing case no longer asserts inside
the helper: the following assertion judges the write, so removing it fails on
"the exchange a turn took up is still noted on disk".

Regenerated patches/ds-bot/0001-core-fixes.patch.
@Birfy Birfy added agent-pr Pull request opened by an agent area:automation Scheduled tasks and automation labels Oct 23, 2026
@Birfy

Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at 0f0006f
Written-by: Reviewer

The waits in this file now run on the clock the case controls, not on wall time: settle becomes waitFor (bounded rounds, advancing the mocked clock each round), and the cleared-note wait no longer asserts inside the helper, so the case at :387 is judged by the assert.equal at :411. Test-only change plus its patches/ds-bot/ mirror; no product path is touched.

What I checked:

  • The failure this case exists for is still caught: removing forgetQueued(event.data.source.exchangeId) from src/host/resume.js:145 — so a turn taking the exchange up never clears its note — fails the case on its own assertion (the exchange a turn took up is still noted on disk, 1 !== 0), not on condition not reached.
  • No wait here depends on wall-clock any more, and 100 consecutive runs of the file are green (below).
  • An exhausted wait is bounded and cheap: 20000 setImmediate rounds measure ~87 ms here, so a genuine failure does not burn a multi-second budget on the runner.

Non-blocking:

  1. mockedTimers?.tick(1) inside waitFor is not pinned by any case: deleting that line leaves the file 17/17 green, because the cases already call t.mock.timers.tick(3000) themselves. The rule the comment states ("a wait must advance the clock") can therefore be dropped in a later edit without a red test; the first case that needs a shorter advance would then fail as condition not reached.
  2. notes() treats a read or parse error as "no notes", so waitFor(() => notedExchange().length === 0) can be satisfied by a torn in-place write rather than by the clearing write. The assert.equal after second.stop() keeps the case honest (stop() returns the write chain, src/host/resume.js:313), so this is a sharp edge, not a defect.

Not run: node scripts/ds-bot-patches.mjs check locally — this sandbox has no git binary and the script shells out to it. The tip's ubuntu-latest job runs that same check (.github/workflows/test.yml:102) and is green, so the regenerated mirror is covered by CI. No smoke test: no index.js or src/host change.

Ran at this tip (0f0006f), in a copy of it:

$ node --test test/follow-through.test.js
$ for i in $(seq 1 100); do timeout 120 node --test test/follow-through.test.js >/tmp/ft833-$i.log 2>&1 || echo "fail $i"; done
100 runs, 0 failures (17 pass each)
$ node --test --test-name-pattern="forgotten everywhere" test/follow-through.test.js
$ node .github/scripts/changelog.mjs check
10 fragment(s) in changelog.d/ are well formed

@github-actions github-actions Bot added the pass label Oct 23, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 23, 2026
Merged via the queue into ThinkFlowLab:main with commit 9d26947 Oct 23, 2026
11 of 12 checks passed
@HugoXK

HugoXK commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Written-by: Test

PASS at 0f0006f40e80aebcd5154079db08a35604b40665 (base 6e4b637f69cfa6a967094e1d2be06f8ef3ed3f05). This is a post-merge record: the pull request merged at 2026-10-23T09:23:02Z while this verification was running, so it is evidence for that merged tip, not a request to merge.

Where: fresh checkout of my own at /home/xuke/.dsh/bot/workspace/v833 — git init, git fetch --depth=100 origin refs/pull/833/head:pr833 main, git checkout pr833 (HEAD 0f0006f4), node_modules symlinked from the shared flybot-dev core, npm_config_cache inside my workspace, TMPDIR=/tmp.

Ran: TZ=UTC npm run verify → exit 0: core 1290/1290, plugins/social 361/361, plugins/tree 120/120, plugins/timeline 125/125, .github/scripts/*.test.mjs 288/288, interop 37/37, live-e2e 16/16; core lint 43 warnings / 0 errors; arch-docs: OK (2 changed file(s)); 10 patches apply cleanly in order and core/ equals ds-bot/ + patches/ds-bot/ — so the regenerated 0001-core-fixes.patch still reproduces core/; build-client --check clean.

Ran: 30 consecutive runs of node --import ./core/test/harness-resolve.mjs --test core/test/follow-through.test.js → 0 failures, 17/17 in every run. That is the flake this fixes: the waits now advance the mocked clock instead of racing a wall-clock deadline with setImmediate.

Ran, knife: removed the clearing write (forgetQueued(event.data.source.exchangeId) beside taken?.add(...) in core/src/host/resume.js) in a copy → 14/17 pass, 3 red: a group turn cut off by a restart is picked up once, and the group is told, a message a shutdown cancelled is sent again once; one still waiting, taken up or withdrawn is not, and a message sent again is forgotten everywhere once a turn takes it up failing at the exchange a turn took up is still noted on disk — the assertion the case exists for. Restored → 17/17. The pull request body says one red; I see three, with the asserted one among them, which is expected since two earlier cases rely on the same clearing write.

Not run: CI and the merge queue (GitHub); no deliberate attempt to reproduce the original timeout by slowing the runner (the mocked-clock change is covered here by 30 repeated runs plus the knife, not by recreating the loaded CI machine); no packaged-app run, and no behaviour outside the test file was touched.

Written-by: Test

@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:automation Scheduled tasks and automation pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

follow-through.test.js:374 races a 3 s wall-clock deadline under mocked timers, so it fails on a green PR and costs a rerun

2 participants