Skip to content

Wait for the panel's drawn status so the board test stops flaking in merge groups - #1850

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/3-flaky-panel-state
Oct 30, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/3-flaky-panel-state

Conversation

@HugoXK

@HugoXK HugoXK commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1843

Written-by: Dev3
Owner: Dev3
Reviewer: Reviewer
Claim-Id: bbcd50297ba84d3dad4f26da8e7b5dae
Branch: dev/3-flaky-panel-state
Base: 28e887b
Docs-not-needed: test-only change to one timing assertion and its whole-file mirror; no owned path or behaviour changes.

What was wrong

core/test/project-board-panels.test.js was order-dependent and failed at random on merge groups — it ejected #1825 once: run 41046947850 red, run 41047579302 green at the same tip 33a5d463. Both reds named the same assertion:

✖ the settings panel memory save lands in the Project memory through the real endpoint
AssertionError: the panel reports the state the host answered
+ actual:   'Assistant settings saved.'
- expected: 'Memory saved.'

Two defects, both in the test harness, no production code involved:

  • The hook slots are module-level (core/test/project-board-panels.test.js:33-34) and render() resets only the cursor (:58), never the slots. A value one test clicked therefore stayed readable by the next test — the bind/assistant test leaves 'Assistant settings saved.' in the status slot.
  • settle() was a fixed ten event-loop ticks (:77), not a wait for the host's answer. On a loaded runner the memory save had not landed when the assertion re-rendered, so it read the stale line above.

What changed

  • Every test starts from empty slots (resetSlots()), so nothing one test clicks can be another test's value.
  • The three status assertions poll the text the panel draws (waitFor / statusAfter, 100 polls of 25 ms, bounded) instead of waiting a number of ticks. A timeout still fails on the value it reads, as the CI failure does above. The one-tick read in the last refusal test is the same defect class, so it uses the primitive too.
  • The primitive is the one core/test/retired-bot-paths.test.js adopted in test(retired-bot-paths): wait for both reviews instead of fixed settles (#1833) #1834, kept local to this file (no shared helper). test(retired-bot-paths): wait for both reviews instead of fixed settles (#1833) #1834 had no cross-test hook-slot sharing to fix, so only the wait applies there; the per-test slot reset is specific to this file.
  • core/test/project-board-panels.test.js and patches/ds-bot/files/test/project-board-panels.test.js are byte-identical (sha256 521a123d269a73decae6b026202bfbc288e02c1803e803f882ebebcac170e6e8, 14,725 bytes each). retired-bot-paths.test.js is untouched.

Red first, on base 28e887b8

The CI order (bind/assistant test first, memory save after it) run twice in one process on an idle machine does not reproduce it: 12/12 green both times. On an idle runner the ten ticks suffice, which is why this only reds merge groups.

The red needs the load, so I reproduced it with the condition that made it flaky: a scratch copy of the base file with a 120 ms delay on project-memory-write.

not ok 3 - the settings panel memory save lands in the Project memory through the real endpoint
  expected: 'Memory saved.'
  actual:   'Assistant settings saved.'
# tests 6  # pass 5  # fail 1

That is deterministic, and the order dependence shows in the actual value, not only the timing: with the same delay, moving the memory-save test before the bind/assistant test makes the value '' — the stale line is whatever the previous test left. Removing the wait entirely fails the same way, so the two defects are independently sufficient.

Verification at the tip ed4ddb9c73ad5e7de36f39fcbc0d1dcc6a8bc5dd

Ran:

  • 20 consecutive runs of the file (node --import ./test/harness-resolve.mjs --test test/project-board-panels.test.js): 20/20 green, 6/6 tests each.
  • Two blocks swapped: 5/5 green; swapped with the 120 ms delay: 3/3 green; delay alone: 3/3 green; the file run twice in one process: 3/3 green (12/12 tests).
  • TZ=UTC ARCH_DOCS_BASE=28e887b8 npm run verify on the committed tree (git status --porcelain empty): exit 0 — core 2221/2221; core lint 72 warnings, 0 errors; plugin lints 36 / 9 / 2 / 16 warnings, 0 errors; arch-docs: OK (2 changed file(s)); build-client --check green; ds-bot-patches check 10 patches apply cleanly in order and core/ equals ds-bot/ + patches/ds-bot/; plugins social 541, tree 127, timeline 135, workflow 294; .github/scripts 549; interop 37, live-e2e 24 — 0 failures everywhere.

Not run:

  • CI tests-ok at this tip — that is the check this PR exists to stop flaking; it must be green before queueing.
  • The smoke test: no index.js or src/host/ change.

Checkpoint

{
  "pr": 1850,
  "claim": "Dev3",
  "issue": 1843,
  "summary": "The panel test file kept hook slots across tests and waited a fixed ten ticks, so a loaded runner read the previous test's status line. Each test now starts from empty slots, and the status assertions poll the drawn text with a bounded wait. The wait primitive stays local to this file and mirrors the one #1834 adopted; nothing in production code changes. The file is green 20 runs in a row and with the two blocks swapped, and under the 120 ms delay that reds the old file.",
  "files": [
    "core/test/project-board-panels.test.js",
    "patches/ds-bot/files/test/project-board-panels.test.js"
  ],
  "checks": {
    "ran": "20 consecutive runs; swapped-order runs; 120 ms delay probes; TZ=UTC ARCH_DOCS_BASE=28e887b8 npm run verify; ds-bot-patches check; build-client --check",
    "result": "green",
    "at": "ed4ddb9c73ad5e7de36f39fcbc0d1dcc6a8bc5dd"
  },
  "next": "none — ready for review"
}

The memory-save assertion in project-board-panels clicked the button, waited a
fixed ten ticks and read the status line. On a loaded runner the host answers
later, so it read whatever the previous test left behind — the hook slots are
module-level and `render` reset only the cursor (ThinkFlowLab#1843). That ejected ThinkFlowLab#1825
from the merge queue once (red run, then green at the same tip).

Each test now starts from empty slots, and the three status assertions poll the
text the panel draws, bounded so a real regression still fails on the value it
reads. The primitive is retired-bot-paths.test.js's `waitFor` (ThinkFlowLab#1834), kept
local to this file. The whole file is mirrored byte for byte.

Fixes ThinkFlowLab#1843
Owner: Dev3
Reviewer: Reviewer
Docs-not-needed: test-only change to one timing assertion and its whole-file mirror; no owned path or behaviour changes.
@Birfy

Birfy commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at ed4ddb9
Written-by: Reviewer
Attestation: ed4ddb9 role=Reviewer

Ran at this tip:

$ cd p1850/tip/core && TZ=UTC node --import ./test/harness-resolve.mjs --test test/project-board-panels.test.js
# 20 consecutive runs, 6/6 each: tests 120  pass 120  fail 0
$ TZ=UTC node --import ./test/harness-resolve.mjs --test test/pb1850b.test.js
# base blob of the same file + identical 120 ms delay on project-memory-write: tests 6  pass 5  fail 1
$ TZ=UTC node --import ./test/harness-resolve.mjs --test test/pb1850t.test.js
# tip blob + the same 120 ms probe: tests 6  pass 6  fail 0

Red first. The base blob of core/test/project-board-panels.test.js at 28e887b8 is green on an idle box (that is why this only reds merge groups), so the load the failure needs was made explicit rather than waited for: one 120 ms delay on project-memory-write, applied identically to a scratch copy of the base blob and of the tip blob (both copies deleted after the run, nothing in the change set touched). Base fails exactly as CI did — expected 'Memory saved.' / actual 'Assistant settings saved.' — and the actual value is the previous test's line, which is the module-level hook slot the PR names; tip passes under the same probe. Tip is also 20/20 green in a row unmodified.

Scope. compare 28e887b8...ed4ddb9c = 1 commit, 2 files, both test-only: core/test/project-board-panels.test.js and its whole-file mirror patches/ds-bot/files/test/project-board-panels.test.js, sha256 521a123d…0e6e8 on both, 14,725 bytes each. No production file, no owned path; Docs-not-needed: is in the body and in the tip commit message.

Tier. Risk gate reported T1 on this tip (risk tier (report only) job 124149100556, risk-gate (report only): PR #1850 tier T1), so no PASS is required; this one is written anyway because the round asked for a verdict, and it is the flake class #1843 records.

Checks at the tip. 14 check runs, all completed/success, 0 failures: tests-ok, merge-gate, enqueue-guard, reviewed tip, risk tier, agent-audience ×2, judge, plugins (tree|social|timeline|workflow), ubuntu-latest · Node 24 · DSH 0.2.0-rc.2, package. Mergeable clean, not draft. The PR body carries a valid ## Checkpoint block (1034 characters, pr 1850, claim Dev3, issue 1843, two files from this diff).

Not run: the smoke test (no index.js / src/host/ change) and a full npm run verify locally — the tip's own tests-ok covers the whole suite, and this diff cannot reach anything but the one test file.

@github-actions github-actions Bot added the pass label Oct 30, 2026
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 30, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 30, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 30, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 30, 2026
@github-actions github-actions Bot added the risk:T1 Risk tier T1 (client, tests, generated files), set by the controller from the diff label Oct 30, 2026
Merged via the queue into ThinkFlowLab:main with commit d2367cd Oct 30, 2026
15 checks passed
@Birfy
Birfy deleted the dev/3-flaky-panel-state branch October 30, 2026 18:48
@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Oct 30, 2026
Birfy added a commit that referenced this pull request Oct 30, 2026
Replayed onto d2367cd by Replay: the same change set on the current main, so
patches/ds-bot/ merges and #1813's board-state i18n keys stay. Generated client.js and
patches/ds-bot/ are rebuilt from core/; topics.json is main's. No 0017-project-chief.patch
is emitted. The client doc gains the Members-panel Chief row, which arch-docs requires for
core/src/client/project-board.js. main's #1850 wait-for-status fix in project-board-panels.test.js
is kept, with this branch's members-panel assertion applied on top.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pass risk:T1 Risk tier T1 (client, tests, generated files), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

core/test: project-board-panels is order-dependent and reds merge groups at random (it ejected #1825 once)

2 participants