mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-08-24 10:14:19 -05:00
fix(test): eliminate mock.module() leaks and platform-specific test failures
## Problem
The full test suite (bun test --max-concurrency=1) had 10 failing tests on
Windows. Investigation revealed 4 distinct root causes, all stemming from
bun's mock.module() not being fully reversible by mock.restore(). When a
test file replaces a shared module via mock.module(), stale bindings persist
in already-imported modules even after mock.restore() is called. This is a
known bun limitation.
The CI (Ubuntu) only showed 1 consistent failure (the attribution test),
but the Windows-local failures exposed real bugs that could surface in CI
under different test ordering.
## Changes
### src/utils/hookChains.integration.test.ts (root polluter)
This file was the biggest source of test pollution with 9 mock.module()
calls replacing shared modules (analytics, growthbook, policyLimits,
teammateMailbox, teammate, AgentTool, replBridge, etc.) with partial
surfaces. For example, the teammateMailbox mock only exported writeToMailbox
but the real module has 20+ exports including isIdleNotification,
createIdleNotification, readMailbox, etc. When mock.restore() didn't fully
undo these mocks, downstream tests got undefined for missing exports.
Fix: Import real modules via cache-busted dynamic imports before setting up
mocks, then spread the real module surface into each mock.module() call.
This way even if the mock leaks, downstream tests see the full module
surface with only the intended overrides. All 9 mock.module calls now
spread their real module counterparts.
Also fixed: the test was failing in isolation with SyntaxError because
attachments.ts transitively imports isIdleNotification from
teammateMailbox.js, which was missing from the partial mock.
### src/utils/settings/changeDetector.test.ts (Windows path normalization)
4 tests failed because getSourceForPath() normalizes paths using
path.normalize() which converts forward slashes to backslashes on Windows.
The test hardcoded Unix-style paths (/tmp/openclaude/user/settings.json)
but path.normalize produces \tmp\openclaude\user\settings.json on
Windows. The path comparison always failed, so handleChange() returned
early without triggering any callbacks or debounce timers.
Fix: Import normalize from 'path' and apply it to all test path constants
(pathsBySource, getManagedSettingsDropInDir). This matches what the
production code does.
### src/utils/exportFormats.test.ts (Windows path separator)
resolveExportFilepath() uses path.join() which produces backslash-separated
paths on Windows. The test expected forward-slash paths.
Fix: Import join from 'path' and use it in the expected value so the
assertion is platform-agnostic.
### src/utils/file.test.ts (growthbook mock leak)
importFileModuleWithKillswitchEnabled() mocked growthbook.js with only
getFeatureValue_CACHED_MAY_BE_STALE: () => killswitchEnabled. When
killswitchEnabled was false, this poisoned isAgentSwarmsEnabled() for all
downstream tests because agentSwarmsEnabled.ts has a static import of
getFeatureValue_CACHED_MAY_BE_STALE that captured the mock binding.
Fix: Import the real growthbook module and spread it into the mock, so
all exports remain available even if the mock leaks.
### src/utils/plugins/officialMarketplaceStartupCheck.test.ts (same pattern)
Same growthbook mock leak pattern. Top-level mock.module with only
getFeatureValue_CACHED_MAY_BE_STALE: () => true.
Fix: Import real growthbook module and spread into mock.
### src/tools/AgentTool/AgentTool.teammateModel.test.ts (transitive mock binding)
4 tests failed with 'Agent Teams is not yet available on your plan' because
isAgentSwarmsEnabled() returned false. The function checks
getFeatureValue_CACHED_MAY_BE_STALE('tengu_amber_flint', true) from
growthbook.js, but the static import binding in agentSwarmsEnabled.ts was
captured from a leaked mock that returned false.
Cache-busting the AgentTool.js import doesn't help because
agentSwarmsEnabled.ts is a transitive dependency that keeps its
already-loaded (mocked) growthbook binding.
Fix: Add mock.module for agentSwarmsEnabled.js in importAgentToolWithSpawnMock()
to pin isAgentSwarmsEnabled to true, matching the test's intent (it sets
CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1).
## Verification
- bun run smoke: passes
- bun test --max-concurrency=1: 3019 pass, 0 fail (verified twice)
- No skipped tests (test.skip/it.skip/describe.skip), no test.todo,
no flaky markers, no test exclusions in config
## Known remaining risks
6 test files still have partial mock.module() calls on providers.js
(withRetry, officialRegistry, domainCheck, conversationRecovery, fastMode)
that don't spread the real module. These don't cause failures under current
test ordering but are latent risks if bun changes file execution order.
This commit is contained in:
@@ -93,6 +93,14 @@ async function importAgentToolWithSpawnMock(): Promise<{
|
||||
...originalSpawnMultiAgentModule!,
|
||||
spawnTeammate,
|
||||
}))
|
||||
// Pin isAgentSwarmsEnabled to true — a prior test's mock.module on
|
||||
// growthbook.js may have left a stale binding in agentSwarmsEnabled.ts
|
||||
// that returns false for the killswitch check. Cache-busting AgentTool.js
|
||||
// doesn't help because agentSwarmsEnabled.ts is a transitive dep that
|
||||
// keeps its already-loaded (mocked) growthbook import.
|
||||
mock.module('../../utils/agentSwarmsEnabled.js', () => ({
|
||||
isAgentSwarmsEnabled: () => true,
|
||||
}))
|
||||
|
||||
const { AgentTool } = await import(
|
||||
`./AgentTool.js?teammateModel=${Date.now()}-${Math.random()}`
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { join } from 'path'
|
||||
import { describe, expect, test } from 'bun:test'
|
||||
|
||||
import {
|
||||
@@ -233,7 +234,7 @@ describe('parseExportArgs', () => {
|
||||
|
||||
describe('resolveExportFilepath', () => {
|
||||
test('resolves relative export filenames under the current working directory', () => {
|
||||
expect(resolveExportFilepath('/work/project', 'transcript.md')).toBe('/work/project/transcript.md')
|
||||
expect(resolveExportFilepath('/work/project', 'transcript.md')).toBe(join('/work/project', 'transcript.md'))
|
||||
})
|
||||
|
||||
test('preserves absolute export filenames', () => {
|
||||
|
||||
@@ -7,7 +7,11 @@ import {
|
||||
async function importFileModuleWithKillswitchEnabled(
|
||||
killswitchEnabled: boolean,
|
||||
) {
|
||||
const realGrowthbook = await import(
|
||||
`../services/analytics/growthbook.js?real=${Date.now()}-${Math.random()}`
|
||||
)
|
||||
mock.module('../services/analytics/growthbook.js', () => ({
|
||||
...realGrowthbook,
|
||||
getFeatureValue_CACHED_MAY_BE_STALE: () => killswitchEnabled,
|
||||
}))
|
||||
|
||||
|
||||
@@ -33,6 +33,25 @@ async function createConfigFile(config: unknown): Promise<string> {
|
||||
return filePath
|
||||
}
|
||||
|
||||
// Cache-busted imports of the real modules so mock.module can spread them.
|
||||
// This prevents partial mocks from breaking transitive imports in other test
|
||||
// files that run after this one (mock.module is process-global and
|
||||
// mock.restore does not fully undo module mocks in Bun).
|
||||
async function importRealModules() {
|
||||
const ts = Date.now()
|
||||
const rand = Math.random()
|
||||
return {
|
||||
analytics: await import(`../services/analytics/index.js?real=${ts}-${rand}`) as typeof import('../services/analytics/index.js'),
|
||||
events: await import(`./telemetry/events.js?real=${ts}-${rand}`) as typeof import('./telemetry/events.js'),
|
||||
policyLimits: await import(`../services/policyLimits/index.js?real=${ts}-${rand}`) as typeof import('../services/policyLimits/index.js'),
|
||||
teamHelpers: await import(`./swarm/teamHelpers.js?real=${ts}-${rand}`) as typeof import('./swarm/teamHelpers.js'),
|
||||
teammateMailbox: await import(`./teammateMailbox.js?real=${ts}-${rand}`) as typeof import('./teammateMailbox.js'),
|
||||
teammate: await import(`./teammate.js?real=${ts}-${rand}`) as typeof import('./teammate.js'),
|
||||
replBridge: await import(`../bridge/replBridgeHandle.js?real=${ts}-${rand}`) as typeof import('../bridge/replBridgeHandle.js'),
|
||||
agentTool: await import(`../tools/AgentTool/AgentTool.js?real=${ts}-${rand}`) as typeof import('../tools/AgentTool/AgentTool.js'),
|
||||
}
|
||||
}
|
||||
|
||||
async function importHookChainsHarness(
|
||||
options: ImportHarnessOptions = {},
|
||||
): Promise<{
|
||||
@@ -55,47 +74,51 @@ async function importHookChainsHarness(
|
||||
},
|
||||
}))
|
||||
|
||||
const real = await importRealModules()
|
||||
|
||||
mock.module('../services/analytics/index.js', () => ({
|
||||
...real.analytics,
|
||||
logEvent: () => {},
|
||||
}))
|
||||
|
||||
mock.module('./telemetry/events.js', () => ({
|
||||
...real.events,
|
||||
logOTelEvent: async () => {},
|
||||
}))
|
||||
|
||||
mock.module('../services/policyLimits/index.js', () => ({
|
||||
...real.policyLimits,
|
||||
isPolicyAllowed: () => allowRemoteSessions,
|
||||
}))
|
||||
|
||||
mock.module('./swarm/teamHelpers.js', () => ({
|
||||
...real.teamHelpers,
|
||||
readTeamFileAsync: async () => options.teamFile ?? null,
|
||||
}))
|
||||
|
||||
mock.module('./teammateMailbox.js', () => ({
|
||||
...real.teammateMailbox,
|
||||
writeToMailbox: writeToMailboxSpy,
|
||||
}))
|
||||
|
||||
mock.module('./teammate.js', () => ({
|
||||
...real.teammate,
|
||||
getAgentName: () => senderName,
|
||||
getTeamName: () => teamName,
|
||||
getTeammateColor: () => 'blue',
|
||||
// Keep parity with the real module's surface so later tests that
|
||||
// run after this file (mock.module is process-global and mock.restore
|
||||
// does not undo module mocks in Bun) do not see undefined members.
|
||||
isTeammate: () => false,
|
||||
isPlanModeRequired: () => false,
|
||||
getAgentId: () => undefined,
|
||||
getParentSessionId: () => undefined,
|
||||
}))
|
||||
|
||||
mock.module('../bridge/replBridgeHandle.js', () => ({
|
||||
...real.replBridge,
|
||||
getReplBridgeHandle: () => replBridgeHandle,
|
||||
}))
|
||||
|
||||
// Integration mock target requested in the task: fallback action can route
|
||||
// through this mocked tool launcher from runtime callback wiring.
|
||||
mock.module('../tools/AgentTool/AgentTool.js', () => ({
|
||||
...real.agentTool,
|
||||
AgentTool: {
|
||||
...real.agentTool.AgentTool,
|
||||
call: agentToolCallSpy,
|
||||
},
|
||||
}))
|
||||
|
||||
@@ -38,7 +38,11 @@ const addMarketplaceSource = mock(async () => ({
|
||||
|
||||
await acquireSharedMutationLock('utils/plugins/officialMarketplaceStartupCheck.test.ts')
|
||||
|
||||
const realGrowthbook = await import(
|
||||
`../../services/analytics/growthbook.js?real=${Date.now()}-${Math.random()}`
|
||||
)
|
||||
mock.module('../../services/analytics/growthbook.js', () => ({
|
||||
...realGrowthbook,
|
||||
getFeatureValue_CACHED_MAY_BE_STALE: () => true,
|
||||
}))
|
||||
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test'
|
||||
import { normalize } from 'path'
|
||||
import {
|
||||
acquireSharedMutationLock,
|
||||
releaseSharedMutationLock,
|
||||
@@ -12,11 +13,11 @@ type SettingsChangeDetectorModule = typeof import('./changeDetector.js') & {
|
||||
}
|
||||
|
||||
const pathsBySource: Record<SettingSource, string | null> = {
|
||||
userSettings: '/tmp/openclaude/user/settings.json',
|
||||
projectSettings: '/tmp/openclaude/project/.claude/settings.json',
|
||||
localSettings: '/tmp/openclaude/project/.claude/settings.local.json',
|
||||
userSettings: normalize('/tmp/openclaude/user/settings.json'),
|
||||
projectSettings: normalize('/tmp/openclaude/project/.claude/settings.json'),
|
||||
localSettings: normalize('/tmp/openclaude/project/.claude/settings.local.json'),
|
||||
flagSettings: null,
|
||||
policySettings: '/tmp/openclaude/managed/managed-settings.json',
|
||||
policySettings: normalize('/tmp/openclaude/managed/managed-settings.json'),
|
||||
}
|
||||
|
||||
let resetSettingsCache = mock(() => {})
|
||||
@@ -43,7 +44,7 @@ async function importFreshModule(): Promise<SettingsChangeDetectorModule> {
|
||||
consumeInternalWrite,
|
||||
executeConfigChangeHooks,
|
||||
getManagedSettingsDropInDir: () =>
|
||||
'/tmp/openclaude/managed/managed-settings.d',
|
||||
normalize('/tmp/openclaude/managed/managed-settings.d'),
|
||||
getSettingsFilePathForSource: (source: SettingSource) =>
|
||||
pathsBySource[source],
|
||||
hasBlockingResult: (results: { blocked: boolean }[]) =>
|
||||
|
||||
Reference in New Issue
Block a user