Skip to content

Fix flaky RB SUnit integration tests: 60s timeout + deterministic run-limit guard - #403

Draft
ericwinger wants to merge 2 commits into
mainfrom
eric/RBflakyTests
Draft

Fix flaky RB SUnit integration tests: 60s timeout + deterministic run-limit guard#403
ericwinger wants to merge 2 commits into
mainfrom
eric/RBflakyTests

Conversation

@ericwinger

Copy link
Copy Markdown
Member

Two fixes for the flaky RB SUnit integration tests, on one branch.

1. The retry-flake (the main one) — timeout bump

The ~15 refactoring *.integration.test.ts each file in the (growing) engine-tests.gs payload and
run an engine GS SUnit suite in-stone. Alone that's ~3s, but under full-npm test contention it's
8–11s — which brushes vitest's 5s default timeout, so those tests fail on the first run and pass
on retry
(the classic flake). Three files already carried an explicit timeout
(split / extract-superclass 60_000, change-signature 30_000) and did not flake; the rest ran
on the 5s default.

Give every in-stone suite-run it a uniform 60s timeout (~6× the worst observed contended
run), matching the existing 60_000 convention. Purely the timeout argument — no production code, no
logic change. (13 files, 15 line changes; change-signature normalized 30_000 → 60_000.) A scan
confirms no in-stone suite-run is left on the default, including the easy-to-miss ones in
refactoringMethod (two suite tests) and refactoringPushMethod (push-up + push-down).

2. The run-limit guard test — deterministic + CI-runnable

runFailingTests' MAX_RUN_CLASSES guard was only "tested" by an on-demand gci smoke test that
branched on the live image's size and, on the under-cap branch, ran every discovered suite in one
~46s un-interruptible GCI call — too slow for CI, so it was quarantined and never actually ran.
Replaced with two fast, deterministic integration tests (default project → run in CI) that trip the
guard via a bounded fixture of throwaway TestCase subclasses with no test methods, so the guard
fires before any suite runs (nothing blocks): limit + 1 → throws the guard; limit → doesn't.
Transient (useIntegrationTest per-test abort). Mirrors the already-merged 066c24d0 (gci → CI
integration migration).

Follow-up (not in this PR)

The cleaner long-term fix — run the engine SUnit once instead of every refactoring re-filing the
payload and running its own suite — is deferred; it needs a stone to verify the consolidated
discovery/run and is better done as its own change.

Verification

Full npm test green via the pre-push hook against a freshly-provisioned 3.7.5 stone with the RB
plugin installed (so the gated integration tests exercised, not skipped): client + server (322) +
mcp (92). CI will confirm across the version matrix.

🤖 Generated with Claude Code

ericwinger and others added 2 commits August 11, 2026 09:20
The MAX_RUN_CLASSES guard (runFailingTests) was only "tested" by an on-demand gci smoke test that
branched on the live image's size and, on the under-cap branch, ran every discovered suite in one
~46s un-interruptible GCI call — too slow for CI (vitest's 5s timeout can't kill it), so it was
quarantined out of CI and never actually ran.

Replace it with two fast, deterministic integration tests (default project → run in CI) that trip
the guard via a bounded fixture of throwaway TestCase subclasses with NO test methods, so the guard
fires BEFORE any suite runs (nothing blocks):
- delete client/src/__tests__/gci/querySunitRunLimit.smoke.test.ts
- add installGuardProbeClasses + SUNIT_GUARD_PROBE_PATTERN to sunitProbeFixture.ts
- add 'refuses an oversized run' (limit+1 -> throws the guard) and 'at the cap boundary'
  (limit -> no throw) to querySunit.integration.test.ts; drop the stale "parked in gci" note

Transient (useIntegrationTest per-test abort; nothing committed to the stone). Same pattern as the
already-merged 066c24d (gci -> CI integration migration).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… retry-flake)

The ~15 refactoring *.integration.test.ts each file in the (growing) engine-tests.gs payload and run
an engine GS SUnit suite in-stone. Alone that's ~3s, but under full-`npm test` contention it's 8-11s
— brushing vitest's 5s default timeout, so those tests fail on the first run and pass on retry (the
classic flake). Three files already carried an explicit timeout (split/extract-superclass 60_000,
change-signature 30_000) and did not flake; the rest ran on the 5s default.

Give every in-stone suite-run `it` a uniform 60s timeout (~6x the worst observed contended run),
matching the existing 60_000 convention. No production code, no logic change — purely the timeout
argument. (change-signature normalized 30_000 -> 60_000 for uniformity.)

A cleaner long-term fix — running the engine SUnit ONCE instead of every refactoring re-filing the
payload and running its own suite — is left as a follow-up (it needs a stone to verify the
consolidated discovery/run, which this environment can't provide).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ericwinger
ericwinger marked this pull request as draft August 11, 2026 16:49
@ericwinger

Copy link
Copy Markdown
Member Author

Data point from hitting this repeatedly over the last couple of days, in case it's useful while
you're in here: there seem to be two distinct failure modes, and this PR fixes one of them.

Mode 1 — the timeout. Exactly as described above: the in-stone suite run brushes vitest's 5s
default. The 60s bump fixes it.

Mode 2 — session exhaustion. Separately, the integration tests fail at login, before any
timeout is relevant:

Login failed:  the maximum number of users are already logged in.
 ❯ GciLibrary.login src/gciLibrary.ts:2102
 ❯ login src/__tests__/useIntegrationTest.ts:128

A longer timeout can't help this one — the login fails immediately, not slowly. It shows up as
whole test files failing in beforeAll, and the victims vary run to run (one run:
gciCompile + queryPython; another: backup + refactoringMethod; another: 5 files across
refactoringSplitClass). That variability plus "always at login" is what distinguishes it from
mode 1.

Workaround that reliably clears it: cap vitest's worker count for the run, which reduces
concurrent GCI logins without skipping anything:

env -u GEMSTONE_NRS_ALL VITEST_MAX_WORKERS=3 VITEST_MIN_WORKERS=1 npm test
# same for the pre-push gate:
env -u GEMSTONE_NRS_ALL VITEST_MAX_WORKERS=3 VITEST_MIN_WORKERS=1 git push origin <branch>

Without it I saw 1, 2, 4 and 7 files fail across different full runs; with it, repeated full runs were green (client 351
files / 5429 tests, server 322, mcp 92). I verified the stone was otherwise idle first — no stray
user gems — so it really is in-run parallelism against the stone's session ceiling, not leftovers.

Not suggesting this PR should grow to cover mode 2 — the timeout fix stands on its own and is
clearly right. Just flagging that the flakiness probably won't fully disappear when this merges,
so it's worth knowing the second cause exists rather than re-diagnosing it later. If it's worth
tracking, a session-count budget for the integration tier (or serializing that tier) seems like
the real fix; happy to open a separate issue for it if you'd prefer that to living in this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants