fix(tests): kill the Bun mock.module cross-file pollution class (Levels 1 + 3 of the reliability audit)#1856
Merged
Merged
Conversation
Bun's mock.module() writes to a process-scoped module registry. All test files in the same 'bun test' run share it, so a top-level mock that replaces a real module with a partial object (dropping any export the factory didn't include) leaks the polluted table into every other test file that imports the same specifier. The corruption surfaces later as 'SyntaxError: Export named X not found in module ...' on files whose source clearly exports X, and depends on non-deterministic file-load ordering (macOS APFS: benign, Linux ext4: 50% CI failure rate on providerTemplates). For every partial-mock callsite whose target is an internal module that ANY other test could import, add '...realModule' to the factory so the real module's exports pass through. Concretely: - oauth-provider-flow.hooks.test.ts (the primary offender causing today's providerTemplates flake) - Mcp.test.tsx (connections.hooks) - Cockpit.test.tsx (audit.hooks, connections.hooks, cockpit.data) - RecentActivity.test.tsx (audit.hooks) Left alone: total-replacement mocks in toggleSidePanel.test.ts and provider-resolution.test.ts. Their targets pull in wxt/storage + graphql codegen output at module load, so the spread pattern would eagerly load environment-coupled code and break the test. No other test imports those specifiers, so cross-file pollution is not a risk. Level 3's per-file worker isolation covers the general class regardless (see below). Follows the 2026-07-17 test reliability audit.
Motivation. Bun's mock.module() registry is process-scoped: a top-level mock in one test file corrupts imports in every other test file that shares the same 'bun test' invocation. Level 1 patches the current offenders but a future contributor can silently reintroduce the class. Per-file isolation kills it regardless of what any author writes. The scripts/run-bun-test.ts wrapper is rewritten to: - Walk the target dir(s) for *.test.ts / *.test.tsx files - Spawn one 'bun test <file>' subprocess per file (stable ordering) - Direct each child's junit output to a temp file - Merge per-file XMLs into the single output CI expects, aggregating the top-level tests/failures/skipped counters - Preserve the existing single-directory calling convention so CI matrix commands work unchanged Cost: ~50 files x ~200ms Bun startup = ~10s added to a suite. Kills the class of failure regardless of future author mistakes. Extends the wrapper to apps/claw-app and apps/claw-server (previously plain 'bun test'); apps/app already used the wrapper. Other packages with no current mock.module usage stay on plain 'bun test' (they can opt in when they add mocks). Follows the 2026-07-17 test reliability audit.
Contributor
✅ Tests passed — 1963/1967
|
Contributor
Greptile SummaryThis PR eliminates a class of non-deterministic CI failures caused by Bun's process-scoped
Confidence Score: 4/5Safe to merge. The core per-file isolation and partial-mock spread are both mechanically correct and well-tested across all three suites. The two minor gaps in the XML merger (self-closing tag regex, missing aggregate The root-cause fix (per-process isolation) is structural and sound. The partial-mock changes are targeted and accompanied by clear inline rationale for every intentional total-replacement. The XML merging logic uses a depth-tracking scanner specifically to avoid the malformed-output bug a naïve regex would produce. The two remaining edge cases — the open-tag regex matching self-closing
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[bun run run-bun-test.ts ./apps/X] --> B{arg is directory?}
B -- yes --> C[walk: collect *.test.ts / *.test.tsx]
B -- no --> D{isTestFile?}
D -- yes --> E[use path directly]
D -- no --> F[warn + skip]
C --> G[sort paths for determinism]
E --> G
G --> H{BROWSEROS_JUNIT_PATH set?}
H -- yes --> I[mkdtemp: perFileJunitDir]
H -- no --> J
I --> J[for each file: spawnSync bun test file]
J --> K{junitDir?}
K -- yes --> L[bun test --reporter=junit --reporter-outfile=N.xml file]
K -- no --> M[bun test file]
L --> N[collect exit codes]
M --> N
N --> O{all files done?}
O -- yes --> P{junitDir?}
P -- yes --> Q[mergeJunitXml: depth-aware testsuite extractor]
Q --> R[write merged XML to BROWSEROS_JUNIT_PATH]
R --> S[rmSync perFileJunitDir]
P -- no --> T{any failures?}
S --> T
T -- yes --> U[process.exit 1]
T -- no --> V[process.exit 0]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[bun run run-bun-test.ts ./apps/X] --> B{arg is directory?}
B -- yes --> C[walk: collect *.test.ts / *.test.tsx]
B -- no --> D{isTestFile?}
D -- yes --> E[use path directly]
D -- no --> F[warn + skip]
C --> G[sort paths for determinism]
E --> G
G --> H{BROWSEROS_JUNIT_PATH set?}
H -- yes --> I[mkdtemp: perFileJunitDir]
H -- no --> J
I --> J[for each file: spawnSync bun test file]
J --> K{junitDir?}
K -- yes --> L[bun test --reporter=junit --reporter-outfile=N.xml file]
K -- no --> M[bun test file]
L --> N[collect exit codes]
M --> N
N --> O{all files done?}
O -- yes --> P{junitDir?}
P -- yes --> Q[mergeJunitXml: depth-aware testsuite extractor]
Q --> R[write merged XML to BROWSEROS_JUNIT_PATH]
R --> S[rmSync perFileJunitDir]
P -- no --> T{any failures?}
S --> T
T -- yes --> U[process.exit 1]
T -- no --> V[process.exit 0]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/scripts/run-bun-test.ts:198
**Self-closing `<testsuite/>` not excluded from open-tag regex**
The pattern `/<testsuite\b[^>]*>/g` matches `<testsuite ... />` because `/>` satisfies `[^>]*>`. A self-closing element would increment `depth` but never hit the close-tag handler, leaving `startOfCurrentTop` set indefinitely and causing the scanner to absorb all subsequent XML content into the current "suite". Bun's JUnit reporter doesn't currently emit self-closing `<testsuite>` elements (even empty suites get an open/close pair), but if that ever changes, the merged output would silently become malformed. Adding a negative lookahead `(?!\/)` before the final `>` excludes self-closing forms.
### Issue 2 of 2
packages/browseros-agent/scripts/run-bun-test.ts:154-156
The merged `<testsuites>` wrapper omits the `time` attribute. Many CI consumers (Jenkins, CircleCI, GitHub Actions test-report displays) read the wrapper `time` to show aggregate suite duration. The individual per-`<testsuite>` `time` attrs are preserved verbatim, so per-file timing is correct; only the top-level aggregate is missing. Adding a `totalTime` accumulator and emitting it in the wrapper would give CI a complete picture without touching the nested XML.
```suggestion
let totalTests = 0
let totalFailures = 0
let totalSkipped = 0
let totalTime = 0
```
Reviews (1): Last reviewed commit: "fix(tests): one bun test process per fil..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile catch: /<testsuite\b[^>]*>/g would match <testsuite ... /> because /> satisfies [^>]*>. A self-closing element would then increment depth without a matching close, leaving startOfCurrentTop set and quietly absorbing subsequent XML into a bogus 'suite'. Bun does not emit self-closing today; the guard is cheap regression insurance for the day it might.
Contributor
Author
|
Note on failing Scope evidence:
Fixing the |
ai@6.0.209 has an ESM export bug where bun cannot resolve the 'tool' named export from ai/dist/index.mjs (SyntaxError at module load). Breaks Tests / server-api on every PR because the caret in 'ai': '^6.0.208' floats to the latest patch. Pin exact so we can't resolve to the broken build. - apps/server/package.json: '^6.0.208' -> '6.0.208' - apps/app/package.json: '^6.0.208' -> '6.0.208' - apps/claw-app/package.json: '^6.0.197' -> '6.0.208' (was on an older range, aligning while we're here) Verified locally after clean reinstall: apps/server test:api = 155 pass, test:agent = 301 pass, both green. When ai ships a fixed 6.0.210+, bump the pin then; the friction is intentional (same reasoning as the ~/.bunfig.toml minimum-release-age gate).
The actual root cause of Tests / server-api failing was NOT the ai package version (the pin from e9ffae4 did not help — CI kept failing on 6.0.208 too). The failure was the same mock.module cross-file pollution class this branch already fixes for apps/app and apps/claw-app: chat-service.test.ts had mock.module('ai', () => ({ createAgentUIStreamResponse: ... })) which replaces the entire 'ai' npm package with an object that drops 'tool', 'UIMessage', and every other export. Every other test file in the same apps/server bun-test run that imports 'tool' from 'ai' then blows up at module load with the SyntaxError CI kept reporting. macOS APFS file ordering happens to load the mock-owning file last, so local runs pass; Linux ext4 ordering hits it first, so CI fails. Spread the real module so unrelated tests keep working. Same pattern applied to providerTemplates / audit.hooks / connections.hooks earlier in this branch. apps/server still uses its bespoke test runner (not the per-file wrapper from Level 3), so it remains vulnerable to future misuse of mock.module(). Migrating it is out of scope for this PR (would rewrite tests/__helpers__/run-test-group.ts); tracked as follow-up. Two other apps/server test files use unsafe mocks on modules imported by other tests (browseros-dir, mcp-manager). Not currently blocking but latent. Leaving for a targeted follow-up rather than expanding this PR.
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
1 participant
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.
What was breaking
50% of recent PR runs of the Tests workflow have been failing, and
Tests / agentwas among the failing suites in 83% of those failures. Signature is always the same:The source file clearly exports both. The retry loop from PR #1851 partially masks it but does not remove the root cause. Full write-up in the audit at plans/browseros-ai/BrowserOS/audits/2026-07-17-1415-claude-code-test-reliability-audit.md.
Root cause
Bun's
mock.module()writes to a process-scoped module registry. All test files in the samebun testrun share it. A top-levelmock.module('X', () => ({ getY: ... }))replaces the entire X module with a stub that has onlygetYand drops every other real export. Any OTHER test file in the same run that later imports one of the dropped exports blows up at module-load time with theSyntaxErrorabove.Which test file loads first is
readdirorder. Stable on macOS APFS (walks alphabetically = benign). Non-deterministic on Linux ext4. That's why local runs pass and CI flakes.Two-part fix
Two commits, applied together as Levels 1 and 3 of the audit's proposed plan.
Commit 1: spread real modules in partial mocks (Level 1)
For every partial
mock.module()callsite that targets an INTERNAL module some other test could import, add...realModule. Concretely:oauth-provider-flow.hooks.test.ts(the file that caused today'sproviderTemplatesflake, plus 5 other mocks)Mcp.test.tsx(connections.hooks)Cockpit.test.tsx(audit.hooks,connections.hooks,cockpit.data)RecentActivity.test.tsx(audit.hooks)Left as total replacement (documented inline):
toggleSidePanel.test.tsandprovider-resolution.test.ts. Their targets pull inwxt/storage+ generated graphql code at module load, so spreading would eagerly load environment-coupled code and break the test. No other test imports those specifiers, so cross-file pollution isn't a risk. Level 3 covers the general class regardless.Commit 2: one bun test process per file (Level 3)
Rewrite
scripts/run-bun-test.tsto walk the target dir(s), spawn onebun test <file>subprocess per test file, and merge per-file JUnit XML into the single output CI expects. This kills the entire class of failure regardless of any future author mistakes.<testsuite>extractor when merging junit (Bun nests suites fordescribegroups; a lazy regex would close on the inner tag and produce malformed XML).apps/claw-appandapps/claw-serverat the wrapper as well;apps/appalready used it. Other packages with no currentmock.moduleusage keep plainbun test; they can opt in later.Cost: ~50 files x ~200ms Bun startup ≈ 10 s added to each suite. Acceptable for zero-flake CI.
Deliberately NOT in this PR
apps/serverhas a bespoke test runner (tests/__helpers__/run-test-group.ts) and 8 mock.module callers that could benefit from the same treatment. Deferred to keep this PR scoped to the specific class that was killing PRs today.Test plan
apps/appsuite: 311 tests, 0 fail across 50 files via the new wrapperapps/claw-appsuite: 26 files, 0 failapps/claw-serversuite: 400 tests, 0 fail across 55 filesNote on commit split
The two commits went out under distinct messages but git shows Level 3 as an empty follow-up commit; the wrapper changes accidentally got staged into Commit 1. The commit messages read correctly and the diff is coherent. Not amending per the repo's git safety rules.
Follows