refactor(3/3): extract shared test infrastructure to reduce duplication - #5
Closed
terryso wants to merge 61 commits into
Closed
refactor(3/3): extract shared test infrastructure to reduce duplication#5terryso wants to merge 61 commits into
terryso wants to merge 61 commits into
Conversation
…ication and cyclomatic complexity: SandboxChecker.matchesAllowedPathList(), OpenAIClient.buildRequestBody(), and Agent.closedQueryResult().
…eateTraceRecorder) from duplicated code in Agent.swift's two largest methods, reducing promptImpl from 678→625 lines and stream from 1120→1092 lines, with all 6209 tests passing.
…essionMessages) that eliminated 7 duplicated inline blocks across promptImpl and stream, reducing promptImpl from 641→610 lines and stream from 1108→1078 lines with all 6212 tests passing.
…createSubAgentSpawner) that eliminated duplicated skill resolution logic across executeSkill/executeSkillStream and SubAgentSpawner construction across promptImpl/stream, with all 5682 tests passing.
… executeToolWithLifecycle) from ToolExecutor.swift's executeSingleTool method, eliminating 5× duplicated error-return patterns, 2× ToolStartedEvent emission blocks, and 2× full tool execution lifecycle blocks, reducing executeSingleTool from ~250→99 lines with all 6211 tests passing.
…rver) from MCPClientManager.swift, eliminating 3× duplicated 6-case switch dispatch blocks across connectAll/reconnect/setServers and 2× duplicated 4-line server removal blocks in setServers, reducing the file from 622→610 lines with all 5684+12 tests passing.
…sultFromExecute, executionErrorResult) from ToolBuilder.swift, eliminating duplicated input decoding pipeline across CodableTool/StructuredCodableTool (2×26 identical lines), duplicated result mapping across RawInputTool/StructuredCodableTool (2×13 identical lines), and duplicated error-return catch blocks across 3 tool structs (3×6 identical lines), reducing the file from 430→409 lines with all 5682+12 tests passing.
…ppendFallbackEntry) from Compact.swift, eliminating 3× duplicated LLM response text extraction blocks and 3× duplicated fallback SessionMemoryEntry construction blocks, reducing the file from 485→477 lines with all 5682+12 tests passing.
…oryStore.swift, eliminating 2× duplicated 26-line filter/sort/limit blocks across InMemoryStore.query and FileBasedMemoryStore.query, reducing the file from 391→377 lines with all 5682+12 tests passing.
…ponse, stripCodeFences) into new LLMResponseHelpers.swift, eliminating 3× duplicated extractTextFromResponse implementations and 3× duplicated stripCodeFences implementations across LLMSkillEvolver.swift, PromptEvolverEngine.swift, and LLMExperienceExtractor.swift, reducing those 3 files by 93 lines combined with all 5682+12 tests passing.
…hropicClient.swift (eliminating 2×11-line duplicated URLError catch blocks) and validatePathSafeIdentifier() into EnvUtils.swift (eliminating 3×10-line duplicated path traversal validation methods across SessionStore, FactStore, and MemoryStore), net -18 lines with all 5682+12 tests passing.
…SkillsDir, atomicWriteJSON) into EnvUtils.swift, eliminating 2× duplicated skills directory resolution blocks (~15 lines each) and 2× duplicated atomic JSON file write patterns (~40 lines each) from SkillUsageStore and SkillCuratorStore, net -36 lines with all 5684+12 tests passing.
…ryDir()/defaultMemoryDir shared helpers into EnvUtils.swift, eliminating 9× duplicated 5-line dateFormatter initializations across store files and Logger, plus 2× duplicated ~18-line resolveMemoryDir methods from MemoryStore and FactStore, net -71 lines with all 5682+12 tests passing.
…wift (unifying 3 duplicated JSON parsing implementations from Agent.swift, OpenAIClient.swift, and Streaming.swift) and replaced 4 inline ISO8601DateFormatter constructions in RunTracker, RunRecoveryService, and ExperienceTypes with makeISO8601DateFormatter() calls, net -26 lines with all 6212+12 tests passing.
…lidateLLMHTTPResponse, resolveBaseURL) into LLMClient.swift, unifying duplicated URLError handling, HTTP response validation, and URL resolution between AnthropicClient and OpenAIClient, net -33 lines with all 6212+12 tests passing.
…ted, defaultHomeDir) eliminating 5× duplicated stop+sessionEnd hook blocks, 2× duplicated LLMRequestStartedEvent emissions, and 2× duplicated home directory resolution blocks, net -12 lines with all 5682+12 tests passing.
…BudgetExceededInStream) eliminating 2× duplicated error classification blocks, 3× duplicated budget check patterns, and 2× duplicated ~35-line budget exceeded handling blocks, net -3 lines with all 5682+12 tests passing.
…LMResponseAsObject, parseLLMResponseAsArray) into LLMResponseHelpers.swift, eliminating the identical trim→stripCodeFences→data→JSONSerialization pipeline duplicated across LLMSkillEvolver, PromptEvolverEngine, and LLMExperienceExtractor, with all 6212+12 tests passing.
…keSDKJSONDecoder) into EnvUtils.swift, eliminating 3× duplicated djb2 hash implementations + 1 inline hash + 3× duplicated JSON encoder/decoder initialization blocks + 3× inline decoder constructions across 6 files, net -28 lines with all 5682+12 tests passing.
…ed ~12-line skill reconstructions in review tools, and replaced inline JSONEncoder/JSONDecoder in MemoryBundleExport/ImportService with shared makeSDKJSONEncoder/makeSDKJSONDecoder factories, net -28 lines with all 5682+12 tests passing.
…tInterrupted and emitAgentFailed in Agent) eliminating 7× duplicated Logger+SDKError denial blocks and 8× duplicated if-let-eventBus+AgentTerminalEvent emission patterns, net -6 lines with all 5682+12 tests passing.
…fy) into EnvUtils.swift, eliminating the last remaining #if os(Linux) home directory resolution pattern from SessionStore and unifying 2 duplicated JSON serialization functions from ConfigTool and ReadMcpResourceTool, net -25 lines with all 5682+12 tests passing.
…xt) centralizing 4× duplicated LLMCostEvent constructions and 2× duplicated 24-field ToolContext constructions across promptImpl and stream, with all 5682+12 tests passing.
…orResponse, requireNonEmptyInput) centralizing 4× duplicated createDirectory+error blocks across store files and 14× duplicated error response patterns + 9× duplicated non-empty guard patterns across review tools, net -41 lines with all 5682+12 tests passing.
…ft, centralizing 4× duplicated ~28-line cost-tracking pipelines (TokenUsage parsing → estimateCost → recordUsage → emitLLMCostEvent → recordCostBreakdown) across promptImpl fallback path, promptImpl main loop, stream messageStart, and stream messageDelta, reducing Agent.swift from 3619→3586 lines with all 5682+12 tests passing.
…efaultSubAgentSpawner.swift and refactored simple spawn to delegate to enhanced spawn, reducing the file from 197→169 lines (-28 lines) by eliminating ~45 lines of duplicated tool filtering, Agent creation, and result processing logic across 2 spawn methods, with all 5682+12 tests passing.
…Value, anyToMCPValue, mcpValueToAny) into MCPTypes.swift, eliminating 8 duplicated private methods across InProcessMCPServer, MCPClientManager, and AgentMCPServer, net -54 lines with all 5682+12 tests passing.
…ver, createMCPSession) and moved ToolExecutionError to MCPTypes.swift, eliminating duplicated tool registration loop (~45 lines), session creation (2×5 lines), and error struct (2×8 lines) across InProcessMCPServer and AgentMCPServer, net -28 lines with all 5682+12 tests passing.
… EnvUtils.swift and 2 default directory constants (defaultTracesDir, defaultApiRunsDir), eliminating duplicated YAML escaping from CuratorRunReport and SkillWriter and inline home directory resolution from TraceRecorder and RunPersistenceService, net -6 lines with all 5682+12 tests passing.
… helper infrastructure from complexity reduction refactoring - project-context.md: added rules 48-56 (shared utility patterns, test infrastructure), updated file counts in rule 35 - architecture.md: added LLMResponseHelpers.swift and AgentEventSSEMapping.swift to Utils/ listing, added shared test infrastructure files to Tests/ listing
…JSONResponse error patterns with reviewErrorResponse() helper, replacing manual Skill reconstruction with skill.withBaseDir(), and replacing manual empty-name guard with requireNonEmptyInput(), reducing the file from 125→105 lines (-20 lines) with all 5682+12 tests passing.
…t, eliminating ~35 lines of duplicated pause/resume/timeout logic between setupPauseHandler (stream) and setupPromptPauseHandler (prompt), reducing Agent.swift from 3586→3581 lines with all 5682+12 tests passing.
…rs in SandboxChecker.swift (eliminating 6 duplicated backslash/quote-stripping blocks across extractSubshellCommand and extractCommandBasename) and simplified PersistedSSEEvent.init(from:) in APITypes.swift (replacing 34-line verbose switch with 15-line nil-first-then-assign pattern), net -22 lines with all 5682+12 tests passing.
…Data, SystemData in SDKMessage.swift and OutputFormat in AgentTypes.swift) that compared all stored properties identically to what Swift synthesizes, net -35 lines with all 5682+12 tests passing.
…g 15 duplicated "store not available" guard-else-return patterns across 12 tool files into single-line try calls, net -5 lines with all 5682+12 tests passing.
…ate) into TestHarness.swift, eliminating 3× duplicated makeTempDir, 3× duplicated cleanup, and 2× duplicated date function definitions across SkillUsageTrackerE2ETests, SkillCuratorE2ETests, and SessionSearchE2ETests, net -28 lines with all 6212+12 tests passing.
…erties in AgentEventTypesE2ETests.swift, eliminating 21× duplicated JSONEncoder+iso8601 and 17× duplicated JSONDecoder+iso8601 construction blocks, net -23 lines with all 5682+12 tests passing.
…umentation test files, eliminating 548 lines of duplicated helper methods (projectRoot, examplesDir, fileContent, packageSwiftContent) that were identically copied across all files, with all 5682+12 tests passing.
…es in AgentEventTypesTests.swift, eliminating 37× duplicated JSONEncoder+iso8601 and 64× duplicated JSONDecoder+iso8601 construction blocks, net -85 lines with all 5682+12 tests passing.
…es in SkillEvolutionTypesTests.swift, centralizing 4 iso8601-configured and 10 raw JSONEncoder/JSONDecoder sites into 2 shared properties, while correctly preserving 4 SkillSignal test sites that require the default .deferredToDate date strategy, net -5 lines with all 5682+12 tests passing.
…n MemoryFactTests and TokenStreamingEventTests (centralizing 3 iso8601 encoder+decoder sites), and replaced 2 raw ISO8601DateFormatter constructions in FactStoreTests with makeISO8601DateFormatter(), with all 5682+12 tests passing.
…, eliminating 276 lines of duplicated setUp/tearDown tempDir boilerplate (13× ~21 lines each) into a single 31-line shared base class, net -245 lines with all 5682+12 tests passing.
…Helpers.swift and replaced 436 duplicated guard-let-XCTFail-return blocks across 15 documentation test files with single-line try requireFileContent() calls, saving 1308 lines with all 5682+12 tests passing.
…Tests, FileCacheTests, MemoryReviewHookTests) to inherit from TempDirTestCase instead of XCTestCase, eliminating 125 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…, FrozenSnapshotTests, SkillLoaderTests) to inherit from TempDirTestCase instead of XCTestCase, eliminating 41 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…wift to inherit from TempDirTestCase, eliminating 152 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…ests 2 classes, SessionManagementWiringATDDTests 3 classes, SessionSearchEngineTests 1 class) to inherit from TempDirTestCase instead of XCTestCase, eliminating 64 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…, CombinedSessionOptionsWiringTests, StreamSessionWiringTests) in SessionManagementWiringATDDTests.swift to inherit from TempDirTestCase instead of XCTestCase, eliminating 18 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…grated 2 remaining test files (TraceRecorderTests, ProjectDocumentDiscoveryTests) to inherit from TempDirTestCase instead of XCTestCase, eliminating 21 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…makeMockURLSession) into MockURLProtocolHelpers.swift, eliminating 10 duplicated readBodyFromStream method definitions across 8 files and 17 duplicated URLSessionConfiguration.ephemeral patterns across 15 files, net -158 lines with all 5682+12 tests passing.
…eTempGitRepo, cleanupTempDir) and makeTestToolContext() shared test helpers, eliminating 2×48-line createTemplateGitRepo method definitions and 6×6-line makeContextWithoutStore definitions across 7 test files, net -28 lines with all 5682+12 tests passing.
…Id parameters and removed 14 private makeContext/makeTestToolContext method definitions across 14 test files, net -105 lines with all 5682+12 tests passing.
…s (teamStore, taskStore, planStore, cronStore, todoStore, worktreeStore, mailboxStore, senderName, mcpConnections) and removed 8 private makeContext/makeTestToolContext method definitions across 8 test files, net -64 lines with all 5682+12 tests passing.
…n 5 test files (APITypesTests, TaskTypesExtendedTests, TokenUsageTests, TaskTypesTests, PromptEvolutionTypesTests), centralizing 67 JSONEncoder/JSONDecoder construction sites into 10 shared static properties with all 6209 tests passing.
…swift, eliminating 4 duplicated private makeSkill() method definitions across ExecuteSkillTests, ExecuteSkillStreamTests, SkillToolTests, and SkillRegistryTests, net -54 lines with all 17028+12 tests passing.
… into GitTestHelpers.swift, eliminating 3 duplicated seedSkill method definitions (17 lines each) and 2 duplicated date(daysAgo:) method definitions (3 lines each) across SkillUsageTrackerTests, SkillCuratorTests, and IntelligentCuratorTests, with all 5682+12 tests passing.
…s.swift, replacing 12 identical 9-line callTool method definitions across 12 test files with thin 3-line delegations, net -26 lines with all 5682+12 tests passing.
Owner
Author
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.
Summary
22 iterations of shared test infrastructure extraction, eliminating duplicated test helpers across 60+ test files.
New shared test infrastructure files
TempDirTestCase.swift(NEW)GitTestHelpers.swift(NEW)MockURLProtocolHelpers.swift(NEW)DocumentationTestHelpers.swift(NEW)Key changes
TempDirTestCase migration (iterations 40-47):
var tempDir + setUp + tearDowntoTempDirTestCasebase classTest encoder/decoder centralization (iterations 35, 37-39, 58):
testEncoder/testDecoderstatic properties replacing 200+ inline JSONEncoder/JSONDecoder constructionsDocumentationTestHelpers (iterations 36, 41):
requireFileContent()replacing 436 guard-let-XCTFail-return blocksTool test context (iterations 50-51, 55):
makeTestToolContext()with optional parameters (cwd, toolUseId, stores, mcpConnections) replacing 28 private factory methods across 28 test filesURL mocking (iteration 49):
readRequestBodyFromStream+makeMockURLSessionreplacing 10 duplicated method definitions and 17 inline URLSessionConfiguration patternsDependencies
Verification
🤖 Generated with Claude Code