test(chat-client): give the jsdom-backed suites a realistic timeout budget - #2843
Open
ashishrp-aws wants to merge 1 commit into
Open
Conversation
…udget The chat-client suites intermittently fail CI with "Timeout of 5000ms exceeded", on a different test each time and on whichever platform happens to be slowest. Reproduced and measured locally rather than guessed at. These tests drive the real MynahUI against jsdom, so creating a tab is several hundred milliseconds of synchronous DOM work: 1811ms and 1700ms for the two sendGenericCommand cases, 825ms and 818ms for the two openTab cases, on an idle machine. Mocha reports a synchronous test that overruns its budget as a timeout -- it compares elapsed time after the body returns, which is the `at processImmediate` frame in the CI stack traces. Under 3x CPU contention, emulating a loaded shared runner, every test slows by a consistent 4.6-7.0x. Three tests then exceed the 5s default and two more land at 70-98% of it, so which test is blamed is down to luck. That matches the observed failures exactly: `openTab > should create a new tab with welcome messages...` at 5559ms against its 5s default on Linux, and `sendGenericCommand > should create a new tab if none exits` at 8275ms against the 10s patch it already carried on Windows. The per-test `this.timeout(10000)` and `this.timeout(30000)` already in the file were earlier attempts at the same problem, applied only to whichever test had failed at the time. This replaces them with one suite-level budget per file, and extends the same to chat.test.ts, whose slowest case reaches 3.5s under load and is next in line. Verification, same 3x contention that reproduces the failure: before: 1 failing (openTab, 5559ms vs 5000ms) after: 0 failing across 3 consecutive runs, slowest test 8804-9193ms vs 30000ms Idle behaviour is unchanged: 213 passing, 1 pending, 0 failing before and after. No product code changes, and 30s still catches a genuine hang.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
chat-clientsuites intermittently fail CI withTimeout of 5000ms exceeded— on a different test each time, and on whichever platform happens to be slowest. Recent examples on the release PR (#2839, a version/changelog-only diff that cannot cause it):sendGenericCommand > should create a new tab if none exits(10000ms)openTab > should create a new tab with welcome messages…(5000ms)The tests being blamed are synchronous, which is what makes the message misleading — there is no missing
awaitor unresolved promise. Mocha compares elapsed time after a synchronous body returns and reports the overrun as a timeout; that is theat processImmediateframe in the CI stack traces.Root cause, measured
These tests drive the real MynahUI against jsdom, so creating a tab is several hundred milliseconds of synchronous DOM work. Idle, on a 14-core dev machine:
sendGenericCommand > should create a new tab if none exitssendGenericCommand > should create a new tab if current tab is loadingopenTab > should create a new tab with messages…openTab > should create a new tab with welcome messages…Chat > onGetSerializedChat > getSerializedChat requestId was propagated…Under 3× CPU contention, emulating a loaded shared runner, every test slows by a consistent 4.6–7.0×. Three then exceed the 5s default and two more land at 70–98% of it — so which test gets blamed is down to luck. That reproduces the observed failures exactly:
openTabat 5559ms against its 5s default, andsendGenericCommandat 8275ms against the 10s patch it already carried.This is not state corruption or a leaked stub. I checked both: the
global.setTimeoutstubs insendGenericCommandare restored by the existingafterEacheven when an assertion throws, and on Linux the blamedopenTabtest runs before any of them.Change
The per-test
this.timeout(10000)andthis.timeout(30000)already inmynahUi.test.tswere earlier attempts at this same problem, each applied only to whichever test had failed at the time — hence the inconsistent 5s/10s/30s budgets. This replaces them with one suite-level budget per file, and extends it tochat.test.ts, whose slowest case reaches 3.5s under load and is next in line.No product code changes. 30s still catches a genuine hang, and the two remaining
function ()test bodies are left as-is.Verification
Under the same 3× contention that reproduces the failure:
Idle behaviour unchanged: 213 passing, 1 pending, 0 failing before and after. Prettier clean.
Why a bigger budget rather than making the tests faster
The work is real and wanted — these suites exercise MynahUI's actual rendering path in jsdom. Making them cheap would mean either changing product code for test convenience or stubbing out the rendering the tests exist to cover. Mocha's timeout is a hang guard, not a performance assertion, and the 5s default is tuned for plain unit tests rather than jsdom UI rendering.
Two things I deliberately did not do, to keep this reviewable: removing
includeNodeLocations: truefromjsDomInjector(measured: only ~7% faster, so not worth the unrelated diff), and adding retries (masks failures rather than budgeting for real work).Note for the release
This unblocks #2839. Because GitHub tests a PR merged into its base, once this is on
maina re-run of #2839 picks it up — the release-please branch does not need regenerating.