test(server): prove the H1 precondition instead of assuming it
Some checks failed
ios / package-tests (APIClient) (push) Has been cancelled
ios / package-tests (HostRegistry) (push) Has been cancelled
ios / package-tests (SessionCore) (push) Has been cancelled
ios / package-tests (WireProtocol) (push) Has been cancelled
ios / testsupport-tests (push) Has been cancelled
ios / app-tests (push) Has been cancelled
ios / ipad-tests (push) Has been cancelled
ios / integration-tests (push) Has been cancelled
ios / ui-test (push) Has been cancelled
ios / ios17-floor-tests (push) Has been cancelled
relay-tripwire / cross-tenant-tripwire (push) Has been cancelled
Some checks failed
ios / package-tests (APIClient) (push) Has been cancelled
ios / package-tests (HostRegistry) (push) Has been cancelled
ios / package-tests (SessionCore) (push) Has been cancelled
ios / package-tests (WireProtocol) (push) Has been cancelled
ios / testsupport-tests (push) Has been cancelled
ios / app-tests (push) Has been cancelled
ios / ipad-tests (push) Has been cancelled
ios / integration-tests (push) Has been cancelled
ios / ui-test (push) Has been cancelled
ios / ios17-floor-tests (push) Has been cancelled
relay-tripwire / cross-tenant-tripwire (push) Has been cancelled
`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.
This commit is contained in:
@@ -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<boolean> {
|
||||
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<void> => 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<string, unknown>
|
||||
sid = att['sessionId'] as string
|
||||
const pre: string[] = []
|
||||
ws1.on('message', (raw) => {
|
||||
try {
|
||||
const m = JSON.parse(raw.toString('utf8')) as Record<string, unknown>
|
||||
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)
|
||||
|
||||
Reference in New Issue
Block a user