mirror of
https://github.com/claude-code-best/claude-code.git
synced 2026-06-19 06:45:50 +00:00
test: remove brittle review follow-up assumptions
CodeRabbit's second pass found two valid brittleness issues and one suggested callback-reference assertion that would not match production behavior. This keeps the production behavior unchanged: timers still schedule the summarizer closure, tests now assert timer-handle identity, and UDS connection errors use native Error.cause instead of shadowing it. Constraint: Do not manufacture behavior just to satisfy a review hint; assertions must match the real AgentSummary scheduling contract. Rejected: Assert a fresh scheduled callback function | scheduleNext intentionally passes the same runSummary closure each time. Rejected: Store a custom cause field on UdsPeerConnectionError | native Error.cause is available under ESNext/Bun. Confidence: high Scope-risk: narrow Directive: Timer tests should assert returned handle identity for ownership, not incidental numeric values. Tested: bun test src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/udsMessaging.test.ts Tested: bunx tsc --noEmit --pretty false Tested: bun run lint Tested: bun run test:all Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage Tested: bun run build Tested: bun run build:vite Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
This commit is contained in:
@@ -34,6 +34,7 @@ describe('startAgentSummarization', () => {
|
|||||||
let loggedErrors: Error[]
|
let loggedErrors: Error[]
|
||||||
let clearedHandles: unknown[]
|
let clearedHandles: unknown[]
|
||||||
let scheduledCount: number
|
let scheduledCount: number
|
||||||
|
let lastTimerHandle: unknown
|
||||||
|
|
||||||
function startTestSummarization(
|
function startTestSummarization(
|
||||||
dependencies: AgentSummaryDependencies = {},
|
dependencies: AgentSummaryDependencies = {},
|
||||||
@@ -84,7 +85,8 @@ describe('startAgentSummarization', () => {
|
|||||||
}
|
}
|
||||||
scheduledCount += 1
|
scheduledCount += 1
|
||||||
scheduled = callback as () => void | Promise<void>
|
scheduled = callback as () => void | Promise<void>
|
||||||
return scheduledCount as unknown as ReturnType<typeof setTimeout>
|
lastTimerHandle = { id: scheduledCount }
|
||||||
|
return lastTimerHandle as ReturnType<typeof setTimeout>
|
||||||
}) as unknown as typeof setTimeout,
|
}) as unknown as typeof setTimeout,
|
||||||
updateAgentSummary: (taskId: string, summary: string) => {
|
updateAgentSummary: (taskId: string, summary: string) => {
|
||||||
updateCalls.push({ taskId, summary })
|
updateCalls.push({ taskId, summary })
|
||||||
@@ -104,6 +106,7 @@ describe('startAgentSummarization', () => {
|
|||||||
loggedErrors = []
|
loggedErrors = []
|
||||||
clearedHandles = []
|
clearedHandles = []
|
||||||
scheduledCount = 0
|
scheduledCount = 0
|
||||||
|
lastTimerHandle = undefined
|
||||||
})
|
})
|
||||||
|
|
||||||
test('summarizes bounded transcript once and skips unchanged fingerprints', async () => {
|
test('summarizes bounded transcript once and skips unchanged fingerprints', async () => {
|
||||||
@@ -179,6 +182,7 @@ describe('startAgentSummarization', () => {
|
|||||||
|
|
||||||
expect(typeof scheduled).toBe('function')
|
expect(typeof scheduled).toBe('function')
|
||||||
const initialScheduledCount = scheduledCount
|
const initialScheduledCount = scheduledCount
|
||||||
|
const initialTimerHandle = lastTimerHandle
|
||||||
await scheduled!()
|
await scheduled!()
|
||||||
|
|
||||||
expect(forkCalls).toEqual([])
|
expect(forkCalls).toEqual([])
|
||||||
@@ -187,9 +191,10 @@ describe('startAgentSummarization', () => {
|
|||||||
'[AgentSummary] Skipping summary — poor mode active',
|
'[AgentSummary] Skipping summary — poor mode active',
|
||||||
)
|
)
|
||||||
expect(scheduledCount).toBe(initialScheduledCount + 1)
|
expect(scheduledCount).toBe(initialScheduledCount + 1)
|
||||||
|
expect(lastTimerHandle).not.toBe(initialTimerHandle)
|
||||||
})
|
})
|
||||||
|
|
||||||
test('logs summary errors and keeps the next timer owned by the summarizer', async () => {
|
test('logs summary errors and schedules the next timer', async () => {
|
||||||
const error = new Error('fork failed')
|
const error = new Error('fork failed')
|
||||||
handle = startTestSummarization({
|
handle = startTestSummarization({
|
||||||
runForkedAgent: async () => {
|
runForkedAgent: async () => {
|
||||||
@@ -199,21 +204,24 @@ describe('startAgentSummarization', () => {
|
|||||||
|
|
||||||
expect(typeof scheduled).toBe('function')
|
expect(typeof scheduled).toBe('function')
|
||||||
const initialScheduledCount = scheduledCount
|
const initialScheduledCount = scheduledCount
|
||||||
|
const initialTimerHandle = lastTimerHandle
|
||||||
await scheduled!()
|
await scheduled!()
|
||||||
|
|
||||||
expect(loggedErrors).toEqual([error])
|
expect(loggedErrors).toEqual([error])
|
||||||
expect(updateCalls).toEqual([])
|
expect(updateCalls).toEqual([])
|
||||||
expect(scheduledCount).toBe(initialScheduledCount + 1)
|
expect(scheduledCount).toBe(initialScheduledCount + 1)
|
||||||
|
expect(lastTimerHandle).not.toBe(initialTimerHandle)
|
||||||
})
|
})
|
||||||
|
|
||||||
test('stop clears the pending summary timer', () => {
|
test('stop clears the pending summary timer', () => {
|
||||||
handle = startTestSummarization()
|
handle = startTestSummarization()
|
||||||
|
const pendingHandle = lastTimerHandle
|
||||||
|
|
||||||
handle.stop()
|
handle.stop()
|
||||||
|
|
||||||
expect(debugLogs).toContain(
|
expect(debugLogs).toContain(
|
||||||
'[AgentSummary] Stopping summarization for task-1',
|
'[AgentSummary] Stopping summarization for task-1',
|
||||||
)
|
)
|
||||||
expect(clearedHandles).toEqual([1])
|
expect(clearedHandles).toEqual([pendingHandle])
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -38,13 +38,14 @@ export type PeerSession = {
|
|||||||
|
|
||||||
export class UdsPeerConnectionError extends Error {
|
export class UdsPeerConnectionError extends Error {
|
||||||
readonly socketPath: string
|
readonly socketPath: string
|
||||||
readonly cause: unknown
|
|
||||||
|
|
||||||
constructor(socketPath: string, cause: unknown) {
|
constructor(socketPath: string, cause: unknown) {
|
||||||
super(`Failed to connect to peer at ${socketPath}: ${errorMessage(cause)}`)
|
super(
|
||||||
|
`Failed to connect to peer at ${socketPath}: ${errorMessage(cause)}`,
|
||||||
|
{ cause },
|
||||||
|
)
|
||||||
this.name = 'UdsPeerConnectionError'
|
this.name = 'UdsPeerConnectionError'
|
||||||
this.socketPath = socketPath
|
this.socketPath = socketPath
|
||||||
this.cause = cause
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user