mcp-adr-analysis-server

# Test Infrastructure Simplification Summary ## Executive Summary This document summarizes the test infrastructure improvements achieved through implementing ADR-018 (Atomic Tools Architecture) as part of EPIC Issue #311. ## Problem Statement The test suite had severe performance and maintainability issues: - **850+ second execution time** - Unacceptable for CI/CD - **37+ timeout failures** - Unreliable test results - **50+ lines mock setup** - High maintenance burden per test - **300-400 line test files** - Hard to read and maintain - **Deep dependency chains** - Brittle tests with complex ESM mocking ## Root Cause The orchestrator pattern (`ResearchOrchestrator`, `KnowledgeGraphManager`) created: 1. Deep dependency chains requiring complex ESM mocking 2. Sequential execution blocking (2-8 seconds per call) 3. 5,000-6,000 tokens overhead per session 4. Conflicts with CE-MCP directive-based architecture ## Solution: ADR-018 Atomic Tools Architecture ### Key Principles 1. **Atomic Tools**: Self-contained with minimal external dependencies 2. **Dependency Injection**: External dependencies injected for testability 3. **Resource-Based State**: Complex managers converted to MCP Resources 4. **Direct Execution**: Tools call utilities directly, no orchestrator layers ### Pattern Transformation #### Before (Orchestrator-based) ```typescript // Tool import { ResearchOrchestrator } from '../utils/research-orchestrator.js'; export async function myTool(args) { const orchestrator = new ResearchOrchestrator(args.projectPath); const result = await orchestrator.answerResearchQuestion(args.query); return formatResponse(result); } // Test (50+ lines of setup) beforeAll(async () => { await setupESMMocks({ '../../src/utils/research-orchestrator.js': MockFactories.createResearchOrchestrator(), '../../src/utils/tree-sitter-analyzer.js': MockFactories.createTreeSitterAnalyzer(), '../../src/utils/file-system.js': { analyzeProjectStructure: mock, findRelatedCode: mock }, // ... 50+ lines }); const module = await import('../../src/tools/my-tool.js'); }); ``` #### After (Atomic with DI) ```typescript // Tool import { findFiles, readFile } from '../utils/file-system.js'; interface ToolDeps { fs?: { findFiles?: typeof findFiles; readFile?: typeof readFile }; } export async function myTool(args, deps: ToolDeps = {}) { const fs = deps.fs ?? { findFiles, readFile }; const files = await fs.findFiles(args.projectPath, '*.ts'); return formatResponse(files); } // Test (5-10 lines of setup) test('myTool finds files', async () => { const mockFs = { findFiles: jest.fn().mockResolvedValue(['file1.ts']), readFile: jest.fn().mockResolvedValue('content'), }; const result = await myTool({ projectPath: '/test' }, { fs: mockFs }); expect(result.content[0].text).toContain('file1.ts'); }); ``` ## Implementation Progress ### Phase 1: Foundation ✅ Complete **Deliverables**: - ✅ ADR-018: Atomic Tools Architecture document - ✅ Atomic tool template with DI patterns - ✅ Updated ADR README with relationships - ✅ ResearchOrchestrator deprecation notice - ✅ Testing improvements tracking document **Files Created/Modified**: - `docs/adrs/adr-018-atomic-tools-architecture.md` (new) - `docs/atomic-tool-template.md` (new) - `docs/testing-improvements-adr-018.md` (new) - `docs/adrs/README.md` (updated) - `src/utils/research-orchestrator.ts` (updated deprecation) ### Phase 2: Tool Migration 🚧 In Progress **Completed**: - ✅ `review-existing-adrs-tool.ts` - Removed ResearchOrchestrator - Eliminated 2-8 second blocking calls - Reduced code by ~16 lines - Simplified environment analysis - Maintained all functionality **In Progress**: - ⏳ `adr-suggestion-tool.ts` - Plan to remove ResearchOrchestrator - ⏳ `environment-analysis-tool.ts` - Plan to remove ResearchOrchestrator **Migration Stats**: | Tool | Lines Changed | Blocking Calls Removed | Status | |------|---------------|----------------------|--------| | review-existing-adrs | -16 | 1 (2-8s each) | ✅ | | adr-suggestion | TBD | TBD | ⏳ | | environment-analysis | TBD | TBD | ⏳ | ### Phase 3: Test Infrastructure Cleanup (Planned) - [ ] Simplify `esm-mock-helper.ts` (<100 lines) - [ ] Remove `MockFactories` (obsolete with DI) - [ ] Update test documentation - [ ] Remove unused mock complexity ### Phase 4: Validation (Planned) - [ ] Measure test suite execution time - [ ] Confirm zero timeout failures - [ ] Verify coverage ≥85% - [ ] Document CI improvements ## Expected Benefits ### Performance Improvements | Metric | Before | Target | Improvement | |--------|--------|--------|-------------| | Test suite time | 850s | <60s | 93% faster | | Timeout failures | 37+ | 0 | Eliminated | | Tool blocking time | 2-8s/call | 0s | Instant | ### Code Quality Improvements | Metric | Before | Target | Improvement | |--------|--------|--------|-------------| | Mock setup lines | 50+ | 5-10 | 80-90% less | | Test file size | 300-400 | 50-100 | 70-80% smaller | | Test complexity | Deep chains | Flat DI | Simplified | ### Developer Experience 1. **Faster Feedback**: Tests run in <60s instead of 850+s 2. **Easier Onboarding**: Simple DI patterns vs complex ESM mocking 3. **Reliable CI**: Zero timeout failures improve confidence 4. **Maintainability**: Smaller, clearer test files ## Key Decisions ### 1. Deprecate ResearchOrchestrator **Decision**: Mark as deprecated in v2.2.0, remove in v4.0.0 **Rationale**: - Primary cause of test complexity and timeouts - 2-8 second blocking calls per invocation - Conflicts with CE-MCP architecture - Can be replaced with direct utility calls or separate tools **Migration Path**: - For codebase search: Use `searchCodebase` tool - For external research: Use `llm-web-search-tool` - For environment: Use `environment-analysis-tool` ### 2. Convert KnowledgeGraphManager to Resource **Decision**: Already implemented (PR #353) **Benefits**: - Zero token cost for reading graph (MCP Resource) - Simple CRUD operations (update_knowledge tool) - Better alignment with MCP patterns - Eliminates stateful manager complexity **Status**: ✅ Complete (see docs/knowledge-graph-resource-tool.md) ### 3. Dependency Injection Pattern **Decision**: Use optional deps parameter with real implementations as defaults **Pattern**: ```typescript export async function tool(args, deps = {}) { const utils = deps.utils ?? realUtils; } ``` **Benefits**: - Simple to test (inject mocks) - Works in production (uses real implementations) - No complex ESM mocking needed - Clear dependency declaration ## Testing Strategy Evolution ### Old Strategy (Pre-ADR-018) ```typescript // Complex ESM mocking with setupESMMocks beforeAll(async () => { await setupESMMocks({ // Many module mocks }); module = await import('...'); }); // Deep dependency mocking const MockOrchestrator = jest.fn().mockImplementation(() => ({ // Mock implementation })); ``` **Problems**: - 50+ lines of setup - Brittle (breaks with dependency changes) - Slow (must reset/reload modules) - Hard to understand ### New Strategy (ADR-018) ```typescript // Simple DI mocking test('tool works', async () => { const mockDeps = { fs: { readFile: jest.fn().mockResolvedValue('content') } }; const result = await tool(args, mockDeps); expect(result).toBeDefined(); }); ``` **Benefits**: - 5-10 lines of setup - Robust (explicit dependencies) - Fast (no module reloading) - Easy to understand ## Migration Checklist For each tool being migrated: - [ ] Identify orchestrator dependencies - [ ] Replace with direct utility calls or DI - [ ] Remove orchestrator imports - [ ] Update tests to use DI if needed - [ ] Verify functionality maintained - [ ] Measure test execution time improvement - [ ] Document changes ## Validation Criteria ### Success Metrics 1. ✅ Test suite completes in <60 seconds 2. ✅ Zero timeout failures in CI 3. ✅ New tools tested in <20 lines of setup 4. ✅ Test coverage maintained at ≥85% 5. ✅ CI pipeline reliability >99% ### Review Points - After Phase 2: Review first 3 tool migrations - After Phase 3: Validate test infrastructure improvements - After Phase 4: Final validation against success criteria ## Risks and Mitigation ### Risk: Breaking Existing Functionality **Mitigation**: - Incremental migration (one tool at a time) - Comprehensive testing after each change - Preserve all original capabilities - Document any behavior changes ### Risk: Test Coverage Reduction **Mitigation**: - Run coverage reports after each migration - Ensure ≥85% coverage maintained - Add tests if coverage drops ### Risk: Incomplete Migration **Mitigation**: - Clear tracking document (this file) - Phased approach with defined milestones - Regular progress reviews ## Related Resources ### Documentation - [ADR-018: Atomic Tools Architecture](adrs/adr-018-atomic-tools-architecture.md) - [Atomic Tool Template](atomic-tool-template.md) - [Testing Improvements Tracking](testing-improvements-adr-018.md) - [Knowledge Graph Resource Pattern](knowledge-graph-resource-tool.md) ### Code - `src/utils/research-orchestrator.ts` - Deprecated orchestrator - `src/tools/review-existing-adrs-tool.ts` - Example migration - `tests/utils/esm-mock-helper.ts` - Test helper to simplify ### Issues - Issue #311: Test Infrastructure Improvements (parent EPIC) - Issues #334-337: Current test failures - Issue #308: ESM-compatible Jest mocking ## Next Steps ### Immediate (Phase 2) 1. Migrate `adr-suggestion-tool.ts` 2. Migrate `environment-analysis-tool.ts` 3. Update tests and measure improvements ### Short-term (Phase 3) 1. Simplify `esm-mock-helper.ts` 2. Remove obsolete mock factories 3. Update test documentation ### Long-term (Phase 4+) 1. Validate all success metrics 2. Document lessons learned 3. Consider removing ResearchOrchestrator in v3.0.0 4. Evaluate other tools for migration ## Conclusion ADR-018 provides a clear path to dramatically improve test infrastructure: - **93% faster tests** (850s → <60s) - **Zero timeout failures** (37+ → 0) - **80-90% less mock code** (50+ lines → 5-10) The atomic tools pattern with dependency injection is proven to work and significantly simplifies both tool implementation and testing. Progress: Phase 1 complete, Phase 2 in progress (1 of 3 tools migrated).