mcp-adr-analysis-server

# Testing Improvements from ADR-018 This document tracks the testing improvements achieved through the implementation of ADR-018 (Atomic Tools Architecture). ## Overview ADR-018 introduced an atomic tools architecture with dependency injection to dramatically simplify test infrastructure and improve test execution speed. ## Problems Addressed ### Before ADR-018 | Problem | Impact | |---------|--------| | Deep orchestrator dependencies | 50+ lines of mock setup per test | | ResearchOrchestrator blocking calls | 2-8 seconds per call, 850+ second test suite | | Complex ESM mocking | Brittle tests with deep dependency chains | | Timeout failures | 37+ test timeout failures in CI | | Test file size | 300-400 lines per test file | ### Root Causes 1. **Orchestrator Pattern**: Tools depended on `ResearchOrchestrator` and `KnowledgeGraphManager` classes 2. **Sequential Execution**: Orchestrators execute sequentially, blocking test execution 3. **Token Overhead**: 5,000-6,000 tokens per session from orchestrator calls 4. **ESM Mocking Complexity**: `jest.unstable_mockModule()` required for deep dependency chains ## Solution: Atomic Tools with Dependency Injection ### Pattern ```typescript // OLD: Orchestrator-based 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); // ... complex logic } // NEW: Atomic with DI 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'); // ... direct logic } ``` ### Test Pattern ```typescript // OLD: Complex ESM mocking beforeAll(async () => { await setupESMMocks({ '../../src/utils/research-orchestrator.js': { ... }, '../../src/utils/tree-sitter-analyzer.js': { ... }, // ... 50+ lines of mocks }); }); // NEW: Simple DI 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).toBeDefined(); }); ``` ## Tools Migrated ### Phase 2: High-Priority Tools #### 1. review-existing-adrs-tool.ts ✅ **Changes**: - Removed `import { ResearchOrchestrator }` - Eliminated orchestrator instantiation (line 187) - Replaced 2-8 second blocking research call with static context - Simplified environment analysis section - Tool now recommends using `environment-analysis-tool` separately **Benefits**: - No more 2-8 second blocking calls - Reduced complexity by ~16 lines - Better separation of concerns (ADR compliance vs environment analysis) - Maintains all ADR compliance checking functionality **Test Impact**: - Test file already uses functional testing (no complex mocks) - Test execution should be faster (no orchestrator delays) - Tests remain simple and maintainable #### 2. adr-suggestion-tool.ts (Planned) **Current State**: - Uses `ResearchOrchestrator` for environment research - Has complex orchestrator-based logic **Migration Plan**: - Remove ResearchOrchestrator dependency - Use direct utility calls for ADR discovery - Simplify suggestion generation logic #### 3. environment-analysis-tool.ts (Planned) **Current State**: - Uses `ResearchOrchestrator` for environment queries - Complex orchestrator-based analysis **Migration Plan**: - Remove ResearchOrchestrator dependency - Use direct environment utility calls - Simplify analysis flow ## Test Infrastructure Improvements ### esm-mock-helper.ts Evolution **Current State**: - Provides `setupESMMocks()` for complex module mocking - Includes `MockFactories` for common orchestrator mocks - ~265 lines of helper code **Planned Simplification**: 1. Remove orchestrator mock factories (not needed with DI) 2. Simplify to basic ESM mocking utilities 3. Add DI helper patterns 4. Reduce to <100 lines ### Testing Documentation **Created**: - `docs/atomic-tool-template.md` - Full atomic tool and test template - `docs/adrs/adr-018-atomic-tools-architecture.md` - Architecture decision - This document - Testing improvements tracking **To Update**: - `tests/README.md` - Add atomic tool testing patterns - Tool-specific test documentation ## Metrics ### Target Metrics (from ADR-018) | Metric | Before | Target | Status | |--------|--------|--------|--------| | Test suite time | 850s | <60s | 🟡 In Progress | | Timeout failures | 37+ | 0 | 🟡 In Progress | | Mock setup lines | 50+ | 5-10 | ✅ Achieved in migrated tools | | Test file size | 300-400 | 50-100 | 🟡 Varies by tool | ### Current Progress | Tool | ResearchOrch Removed | DI Added | Tests Simplified | Status | |------|---------------------|----------|------------------|--------| | review-existing-adrs-tool | ✅ | N/A* | ✅ | Complete | | adr-suggestion-tool | ⏳ | ⏳ | ⏳ | Planned | | environment-analysis-tool | ⏳ | ⏳ | ⏳ | Planned | \* N/A: Tool doesn't need DI as it uses direct utility calls ## Testing Best Practices ### For New Tools 1. **Use Dependency Injection** ```typescript export async function myTool(args, deps = {}) { const utils = deps.utils ?? realUtils; // ... } ``` 2. **Simple Test Setup** ```typescript test('tool works', async () => { const mockDeps = { utils: { fn: jest.fn() } }; const result = await myTool(args, mockDeps); expect(result).toBeDefined(); }); ``` 3. **Avoid Orchestrators** - Use direct utility calls instead of ResearchOrchestrator - Break down complex operations into smaller functions - Keep tools focused and atomic ### For Existing Tools 1. **Identify Orchestrator Dependencies** ```bash grep -n "ResearchOrchestrator\|KnowledgeGraphManager" src/tools/*.ts ``` 2. **Replace with Direct Calls** - Import specific utilities needed - Remove orchestrator instantiation - Simplify logic flow 3. **Update Tests** - Remove complex ESM mocking - Add simple DI mocking if needed - Verify tests still pass ## Validation Checklist - [ ] All high-priority tools migrated (review, adr-suggestion, environment-analysis) - [ ] Test suite runs in <60 seconds - [ ] Zero timeout failures in CI - [ ] Test coverage maintained at ≥85% - [ ] esm-mock-helper.ts simplified to <100 lines - [ ] MockFactories removed (obsolete with DI) - [ ] Documentation updated with new patterns ## Next Steps ### Phase 2 (In Progress) - [ ] Migrate `adr-suggestion-tool.ts` - [ ] Migrate `environment-analysis-tool.ts` - [ ] Update tests for migrated tools ### Phase 3 (Planned) - [ ] Simplify `esm-mock-helper.ts` - [ ] Remove MockFactories - [ ] Update test documentation ### Phase 4 (Validation) - [ ] Measure test suite execution time - [ ] Confirm zero timeout failures - [ ] Verify test coverage - [ ] Document improvements ## References - **ADR-018**: [Atomic Tools Architecture](../adrs/adr-018-atomic-tools-architecture.md) - **Template**: [Atomic Tool Template](atomic-tool-template.md) - **ADR-005**: [Testing and Quality Assurance Strategy](../adrs/adr-005-testing-and-quality-assurance-strategy.md) - **Issue #311**: Test Infrastructure Improvements (parent EPIC) ## Success Metrics (To Be Updated) ### Test Execution Time - **Baseline**: 850+ seconds - **Current**: TBD (measure after migrations complete) - **Target**: <60 seconds ### Timeout Failures - **Baseline**: 37+ failures - **Current**: TBD (measure after migrations complete) - **Target**: 0 failures ### Code Quality - **Mock Setup Reduction**: 50+ lines → 5-10 lines ✅ (in migrated tools) - **Test File Size**: 300-400 lines → varies (depends on tool complexity) - **Test Maintainability**: Improved with simpler DI patterns ✅ ## Lessons Learned ### What Worked Well 1. **Removing Orchestrators**: Dramatically simplified tool logic 2. **Static Contexts**: Replaced blocking calls with static information 3. **Separation of Concerns**: Tools now have single, clear responsibilities 4. **Documentation First**: ADR-018 and template provided clear guidance ### Challenges 1. **Hook Dependencies**: Pre-commit/pre-push hooks require gitleaks (not available in all environments) 2. **Test Environment**: Some tests need actual file system for accurate validation 3. **Balancing Coverage**: Need to maintain high coverage while simplifying tests ### Recommendations 1. **Incremental Migration**: Migrate tools one at a time, validate each 2. **Preserve Functionality**: Ensure tools maintain all original capabilities 3. **Document Changes**: Clear migration notes for each tool 4. **Measure Impact**: Track test execution time improvements