From 822364d12bec34480d4d1aeeac20af62378247db Mon Sep 17 00:00:00 2001 From: Yaojia Wang Date: Thu, 30 Jul 2026 16:27:32 +0200 Subject: [PATCH] test(server): prove the H1 precondition instead of assuming it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `H1 shell state survives a server restart` failed in the full parallel suite and passed in isolation, which reads like a timing flake in the assertion. It was not. Polling for the marker instead of sleeping a fixed 900ms showed the shell replying promptly with: echo MARK-$WEBTERM_MARK MARK- An EMPTY variable. The post-restart shell was fine; the variable had never been set in the PRE-restart shell, because a fixed 500ms is not enough for the shell to become ready and run the assignment when 81 test files are competing for the machine. The symptom then appears at the far end of the test and reads as "the tmux session did not survive the restart", sending whoever debugs it after entirely the wrong bug. So the setup now proves itself: it echoes the variable back and waits for `SET-tmuxlives` before restarting, and fails there with "the variable was never set in the pre-restart shell" if it did not take. The final assertion likewise waits for its marker with a bounded timeout rather than guessing a duration. Neither wait can mask a genuine failure: if the tmux session really did not survive, the shell is fresh, the variable is empty, and the marker never appears no matter how long we wait. Added a `waitUntil` helper for this. It carries its own sleep because the `delay` the cases use is a local const inside a test body, not module scope. Verified: full `npx vitest run test/` -> 81 files, 2243 tests, 0 failures. NOT fixed, and not mine to guess at: `server.test.ts` has at least one more real-PTY case that flakes under the same parallel load (⑤ attach → attached → shell prompt output times out in `waitForMessage`, then its afterEach hook times out too). It passes in isolation. Same class of fixed-delay assumption, different case. --- test/integration/server.test.ts | 54 +++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/test/integration/server.test.ts b/test/integration/server.test.ts index 7029f04..43fa363 100644 --- a/test/integration/server.test.ts +++ b/test/integration/server.test.ts @@ -192,6 +192,30 @@ function collectMessages(ws: WebSocket, count: number, timeoutMs = 5_000): Promi }) } +/** + * Poll `predicate` until it is true, or give up after `timeoutMs`. + * + * Prefer this over a fixed `delay()` whenever the thing being waited for is a + * real event (a shell replying, a file appearing) rather than a deliberate + * pause. A fixed sleep encodes a guess about machine speed, and the guess is + * wrong exactly when the suite is busiest — which is why it shows up as a + * "flaky" test that only fails in the full parallel run. + * + * Returns whether the predicate became true, rather than throwing, so the caller + * can assert with its own message and dump the state it actually observed. + */ +async function waitUntil(predicate: () => boolean, timeoutMs: number, stepMs = 50): Promise { + const deadline = Date.now() + timeoutMs + // Its own sleep on purpose: the `delay` the cases use is a local const inside a + // test body, so a module-level helper cannot see it. + const sleep = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)) + while (Date.now() < deadline) { + if (predicate()) return true + await sleep(stepMs) + } + return predicate() +} + /** * Wait for the WebSocket to close, returning { code, reason }. */ @@ -828,9 +852,26 @@ describe('startServer — integration', { timeout: 60_000 }, () => { ws1.send(JSON.stringify({ type: 'attach', sessionId: null })) const att = (await waitForMessage(ws1, isAttached, PTY_WAIT_MS)) as Record sid = att['sessionId'] as string + const pre: string[] = [] + ws1.on('message', (raw) => { + try { + const m = JSON.parse(raw.toString('utf8')) as Record + if (m['type'] === 'output' && typeof m['data'] === 'string') pre.push(m['data']) + } catch { + /* ignore */ + } + }) await delay(500) ws1.send(JSON.stringify({ type: 'input', data: 'WEBTERM_MARK=tmuxlives\r' })) - await delay(500) + // PROVE the precondition instead of assuming it. Under the full parallel + // suite a fixed sleep is not enough for the shell to be ready and run the + // assignment before the restart, and when that happens the post-restart + // shell reports an EMPTY variable — which looks exactly like "the tmux + // session did not survive" and sends the reader hunting the wrong bug. + // Echo it back and wait for proof, so a setup failure fails HERE, saying so. + ws1.send(JSON.stringify({ type: 'input', data: 'echo SET-$WEBTERM_MARK\r' })) + const markSet = await waitUntil(() => pre.join('').includes('SET-tmuxlives'), 8_000) + expect(markSet, 'the variable was never set in the pre-restart shell').toBe(true) ws1.close() await waitForClose(ws1, 3_000).catch(() => undefined) @@ -856,8 +897,17 @@ describe('startServer — integration', { timeout: 60_000 }, () => { ws2.send(JSON.stringify({ type: 'attach', sessionId: sid })) await delay(400) ws2.send(JSON.stringify({ type: 'input', data: 'echo MARK-$WEBTERM_MARK\r' })) - await delay(900) + // POLL, don't sleep a fixed 900ms. The shell's reply is the only thing we + // are waiting for, and how long it takes depends on machine load: under the + // full 81-file parallel suite this reliably overran the old budget, so the + // assertion fired while `out` still held only the echoed input. Waiting for + // the condition cannot hide a genuine failure — if the tmux session did NOT + // survive, the shell is fresh, `$WEBTERM_MARK` is empty, and the marker never + // appears no matter how long we wait; the expect below then fails exactly as + // it did before, just with a bounded wait instead of a guessed one. + const markerSeen = await waitUntil(() => out.join('').includes('MARK-tmuxlives'), 8_000) // The variable set before the restart is still set → same shell survived. + expect(markerSeen, `shell reply after re-attach: ${JSON.stringify(out.join(''))}`).toBe(true) expect(out.join('')).toContain('MARK-tmuxlives') ws2.close() await waitForClose(ws2, 3_000).catch(() => undefined)