ReviewOS

also looking at this

stacks/rpx

test: stop leaking mocked module state across test files

#2274
Merged glennmichael123 wants to merge fix/2270-2271-test-state-leaks into main
8 files +77 -67
10 days ago

Closes #2270. Also fixes the second half of #2271 — see the note at the bottom, the first half turned out to be a false positive.

Why

bun test shares one module registry across every file in the process, and spyOn does not auto-restore. A stub left installed therefore applies to every file that runs afterwards, and which files those are depends on bun's file order, which differs per platform. That is exactly the shape of #2267, where a leaked spawn stub hung the gateway CLI tests on Linux only.

Proof

A probe that runs the six offending files and then checks whether the shared exports are still real:

BEFORE  LEAKED: ["Hosts.addHosts","Hosts.checkHosts","Hosts.removeHosts",
                 "Https.cleanupCertificates","Start.startServer"]
AFTER   LEAKED: []

Five shared exports were being replaced by no-op stubs for the remainder of every run. Worth stressing that these fail silently — a leaked addHosts no-op means a later test asserting hosts-file behaviour passes without exercising anything, unlike #2267's leak which at least hung loudly.

Per file

fileproblemfix
hosts.test.ts3 spies on ../src/hosts in beforeEach, no restore anywhereafterEach(() => mock.restore())
start-lb-passthrough.test.tsStart.startServer stub, no restore — the file's own comment notes others leak this export onto itsame
integration.test.ts2 spies only ever asserted with toBeDefined(), never invokedassert the real exports; drop the spies
cleanup.test.tsmock.module('../src/hosts') / ('../src/https') replaced each namespace with a single export, permanently — mock.restore() does not undo a module mockconverted to restorable spies, the technique start.test.ts already uses
start-single-proxy-production-certs.test.tsrestores sat at the end of the it, so a throwing assertion left the stubs installedmoved into afterEach
start-on-demand.test.tssame conditional patternfile-level afterEach — the second describe needs the real redirect server
vite-plugin.test.ts3 dead mock.modules; nothing in the file imports the modules they mockeddeleted (module mocks are permanent, so restoring is not an option)
site-supervisor.test.tsthe supervisor in 'stopAll signals every running site' was the only one of eight never registered with supervisors, so the afterEach could not reach its 30s reaperone line, registered before the first await that can reject

Idiom throughout is afterEach(() => { mock.restore() }), already the convention in start.test.ts / process-manager.test.ts / https.test.ts. Unlike a trailing .mockRestore() it also fires when an assertion throws, and being global it launders inbound leaks too.

Left alone deliberately: sni-memory.test.ts (already correct), header-modification.test.ts's beforeAll/afterAll console spy (a blanket afterEach there would drop the spy after the first test), and cleanup.test.ts's node:process mock (Proxy-delegated, does not reach later files).

Two corrections to the filed issues

  • #2271's :48080 listener leak is a false positive. I probed it directly — a later file in the same process binds :48080 fine, both before and after. I had added a Start.cleanup() to drain it and removed that again rather than ship a fix for a bug that does not exist, especially one that globally drains servers other files may own. The other half of #2271, the untracked SiteSupervisor, is real and fixed here.
  • The vite-plugin.test.ts mocks are not demonstrably breaking anything. #2270 predicted plain bun test at the repo root would fail; it passes on main (549/0). The deletion still stands — the code is dead and holds permanent global state — but it is hygiene, not a live fix.

Verification

  • bun run test: 542 pass / 0 fail, plus 7/0 for vite-plugin
  • plain bun test at repo root (one process, all 49 files): 549 pass / 0 fail
  • tsc --noEmit: 0 errors
  • pickier: 0 errors (1 pre-existing warning in dataplane/README.md, untouched here)

🤖 Generated with Claude Code

8 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.