Skip to content

Commit 6455b61

Browse files
cameroncookecameroncooke
committed
WIP
fix: replace handler testing violations with logic function calls using dependency injection Replace 7 handler testing violations across 3 test files to follow the architectural principle: "NEVER test handlers directly - always test logic functions with dependency injection" VIOLATIONS FIXED: - src/mcp/tools/utilities/__tests__/clean_proj.test.ts: 2 violations * Replaced cleanProj.handler() calls with clean_projLogic() calls using mock executor * Changed parameter validation tests to successful execution tests - src/mcp/tools/ui-testing/__tests__/touch.test.ts: 3 violations * Replaced touchPlugin.handler() calls with touchLogic() calls using mock executor and axeHelpers * Changed parameter validation tests to business logic tests (axe dependency error, touch down/up) - src/mcp/tools/ui-testing/__tests__/type_text.test.ts: 2 violations * Replaced typeTextPlugin.handler() calls with type_textLogic() calls using mock executor and axeHelpers * Changed parameter validation tests to business logic tests (axe dependency error, successful typing) ARCHITECTURE COMPLIANCE: - All tests now use dependency injection pattern with createMockExecutor() - UI testing tools also use createMockAxeHelpers() for axe dependency simulation - Tests verify business logic rather than MCP SDK handler validation - Maintains comprehensive test coverage with literal response validation All affected tests pass (49/49) with proper dependency injection patterns. WIP
1 parent da23dcf commit 6455b61

162 files changed

Lines changed: 7362 additions & 7006 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.vscode/launch.json‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,33 @@
1818
"sourceMapPathOverrides": {
1919
"/*": "${workspaceFolder}/src/*"
2020
}
21+
},
22+
{
23+
"type": "node",
24+
"request": "launch",
25+
"name": "Launch MCP Server Dev",
26+
"program": "${workspaceFolder}/build/index.js",
27+
"cwd": "${workspaceFolder}",
28+
"runtimeArgs": [
29+
"--inspect=9999"
30+
],
31+
"env": {
32+
"XCODEBUILDMCP_DEBUG": "true",
33+
"XCODEBUILDMCP_DYNAMIC_TOOLS": "true",
34+
"INCREMENTAL_BUILDS_ENABLED": "false",
35+
"XCODEBUILDMCP_IOS_TEMPLATE_PATH": "/Volumes/Developer/XcodeBuildMCP-iOS-Template",
36+
"XCODEBUILDMCP_MACOS_TEMPLATE_PATH": "/Volumes/Developer/XcodeBuildMCP-macOS-Template"
37+
},
38+
"sourceMaps": true,
39+
"outFiles": [
40+
"${workspaceFolder}/build/**/*.js"
41+
],
42+
"skipFiles": [
43+
"<node_internals>/**"
44+
],
45+
"sourceMapPathOverrides": {
46+
"/*": "${workspaceFolder}/src/*"
47+
}
2148
}
2249
]
2350
}

‎README.md‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -311,11 +311,11 @@ https://github.com/user-attachments/assets/e3c08d75-8be6-4857-b4d0-9350b26ef086
311311

312312
Contributions are welcome! Here's how you can help improve XcodeBuildMCP.
313313

314-
See our [CONTRIBUTING](docs/CONTRIBUTING.md) document for detailed contribution guidelines, including:
315-
- Development setup instructions
316-
- Mandatory testing principles
317-
- Code quality standards
318-
- Pre-commit checklist
314+
See our documentation for development:
315+
- [CONTRIBUTING](docs/CONTRIBUTING.md) - Contribution guidelines and development setup
316+
- [CODE_QUALITY](docs/CODE_QUALITY.md) - Code quality standards, linting, and architectural rules
317+
- [TESTING](docs/TESTING.md) - Testing principles and patterns
318+
- [ARCHITECTURE](docs/ARCHITECTURE.md) - System architecture and design principles
319319

320320
## Licence
321321

‎docs/CODE_QUALITY.md‎

Lines changed: 303 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,303 @@
1+
# XcodeBuildMCP Code Quality Guide
2+
3+
This guide consolidates all code quality, linting, and architectural compliance information for the XcodeBuildMCP project.
4+
5+
## Table of Contents
6+
7+
1. [Overview](#overview)
8+
2. [ESLint Configuration](#eslint-configuration)
9+
3. [Architectural Rules](#architectural-rules)
10+
4. [Development Scripts](#development-scripts)
11+
5. [Code Pattern Violations](#code-pattern-violations)
12+
6. [Type Safety Migration](#type-safety-migration)
13+
7. [Best Practices](#best-practices)
14+
15+
## Overview
16+
17+
XcodeBuildMCP enforces code quality through multiple layers:
18+
19+
1. **ESLint**: Handles general code quality, TypeScript rules, and stylistic consistency
20+
2. **TypeScript**: Enforces type safety with strict mode
21+
3. **Pattern Checker**: Enforces XcodeBuildMCP-specific architectural rules
22+
4. **Migration Scripts**: Track progress on type safety improvements
23+
24+
## ESLint Configuration
25+
26+
### Current Configuration
27+
28+
The project uses a comprehensive ESLint setup that covers:
29+
30+
- TypeScript type safety rules
31+
- Code style consistency
32+
- Import ordering
33+
- Unused variable detection
34+
- Testing best practices
35+
36+
### ESLint Rules
37+
38+
For detailed ESLint rules and rationale, see [ESLINT_RULES.md](./ESLINT_RULES.md).
39+
40+
### Running ESLint
41+
42+
```bash
43+
# Check for linting issues
44+
npm run lint
45+
46+
# Auto-fix linting issues
47+
npm run lint:fix
48+
```
49+
50+
## Architectural Rules
51+
52+
XcodeBuildMCP enforces several architectural patterns that cannot be expressed through ESLint:
53+
54+
### 1. Dependency Injection Pattern
55+
56+
**Rule**: All tools must use dependency injection for external interactions.
57+
58+
✅ **Allowed**:
59+
- `createMockExecutor()` for command execution mocking
60+
- `createMockFileSystemExecutor()` for file system mocking
61+
- Logic functions accepting `executor?: CommandExecutor` parameter
62+
63+
❌ **Forbidden**:
64+
- Direct use of `vi.mock()`, `vi.fn()`, or any Vitest mocking
65+
- Direct calls to `execSync`, `spawn`, or `exec` in production code
66+
- Testing handler functions directly
67+
68+
### 2. Handler Signature Compliance
69+
70+
**Rule**: MCP handlers must have exact signatures as required by the SDK.
71+
72+
✅ **Tool Handler Signature**:
73+
```typescript
74+
async handler(args: Record<string, unknown>): Promise<ToolResponse>
75+
```
76+
77+
✅ **Resource Handler Signature**:
78+
```typescript
79+
async handler(uri: URL): Promise<{ contents: Array<{ text: string }> }>
80+
```
81+
82+
❌ **Forbidden**:
83+
- Multiple parameters in handlers
84+
- Optional parameters
85+
- Dependency injection parameters in handlers
86+
87+
### 3. Testing Architecture
88+
89+
**Rule**: Tests must only call logic functions, never handlers directly.
90+
91+
✅ **Correct Pattern**:
92+
```typescript
93+
const result = await myToolLogic(params, mockExecutor);
94+
```
95+
96+
❌ **Forbidden Pattern**:
97+
```typescript
98+
const result = await myTool.handler(params);
99+
```
100+
101+
### 4. Server Type Safety
102+
103+
**Rule**: MCP server instances must use proper SDK types, not generic casts.
104+
105+
✅ **Correct Pattern**:
106+
```typescript
107+
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
108+
const server = (globalThis as { mcpServer?: McpServer }).mcpServer;
109+
server.server.createMessage({...});
110+
```
111+
112+
❌ **Forbidden Pattern**:
113+
```typescript
114+
const server = (globalThis as { mcpServer?: Record<string, unknown> }).mcpServer;
115+
const serverInstance = (server.server ?? server) as Record<string, unknown> & {...};
116+
```
117+
118+
## Development Scripts
119+
120+
### Core Scripts
121+
122+
```bash
123+
# Build the project
124+
npm run build
125+
126+
# Run type checking
127+
npm run typecheck
128+
129+
# Run tests
130+
npm run test
131+
132+
# Check code patterns (architectural compliance)
133+
node scripts/check-code-patterns.js
134+
135+
# Check type safety migration progress
136+
npm run check-migration
137+
```
138+
139+
### Pattern Checker Usage
140+
141+
The pattern checker enforces XcodeBuildMCP-specific architectural rules:
142+
143+
```bash
144+
# Check all patterns
145+
node scripts/check-code-patterns.js
146+
147+
# Check specific pattern type
148+
node scripts/check-code-patterns.js --pattern=vitest
149+
node scripts/check-code-patterns.js --pattern=execsync
150+
node scripts/check-code-patterns.js --pattern=handler
151+
node scripts/check-code-patterns.js --pattern=handler-testing
152+
node scripts/check-code-patterns.js --pattern=server-typing
153+
154+
# Get help
155+
node scripts/check-code-patterns.js --help
156+
```
157+
158+
### Tool Summary Scripts
159+
160+
```bash
161+
# Show tool and resource summary
162+
npm run tools
163+
164+
# List all tools
165+
npm run tools:list
166+
167+
# List both tools and resources
168+
npm run tools:all
169+
```
170+
171+
## Code Pattern Violations
172+
173+
The pattern checker identifies the following violations:
174+
175+
### 1. Vitest Mocking Violations
176+
177+
**What**: Any use of Vitest mocking functions
178+
**Why**: Breaks dependency injection architecture
179+
**Fix**: Use `createMockExecutor()` instead
180+
181+
### 2. ExecSync Violations
182+
183+
**What**: Direct use of Node.js child_process functions in production code
184+
**Why**: Bypasses CommandExecutor dependency injection
185+
**Fix**: Accept `CommandExecutor` parameter and use it
186+
187+
### 3. Handler Signature Violations
188+
189+
**What**: Handlers with incorrect parameter signatures
190+
**Why**: MCP SDK requires exact signatures
191+
**Fix**: Move dependencies inside handler body
192+
193+
### 4. Handler Testing Violations
194+
195+
**What**: Tests calling `.handler()` directly
196+
**Why**: Violates dependency injection principle
197+
**Fix**: Test logic functions instead
198+
199+
### 5. Improper Server Typing Violations
200+
201+
**What**: Casting MCP server instances to `Record<string, unknown>` or using custom interfaces instead of SDK types
202+
**Why**: Breaks type safety and prevents proper API usage
203+
**Fix**: Import `McpServer` from SDK and use proper typing instead of generic casts
204+
205+
## Type Safety Migration
206+
207+
The project is migrating to improved type safety using the `createTypedTool` factory:
208+
209+
### Check Migration Status
210+
211+
```bash
212+
# Show summary
213+
npm run check-migration
214+
215+
# Show detailed analysis
216+
npm run check-migration:verbose
217+
218+
# Show only unmigrated tools
219+
npm run check-migration:unfixed
220+
```
221+
222+
### Migration Benefits
223+
224+
1. **Compile-time type safety** for tool parameters
225+
2. **Automatic Zod schema validation**
226+
3. **Better IDE support** and autocomplete
227+
4. **Consistent error handling**
228+
229+
## Best Practices
230+
231+
### 1. Before Committing
232+
233+
Always run these checks before committing:
234+
235+
```bash
236+
npm run build # Ensure code compiles
237+
npm run typecheck # Check TypeScript types
238+
npm run lint # Check linting rules
239+
npm run test # Run tests
240+
node scripts/check-code-patterns.js # Check architectural compliance
241+
```
242+
243+
### 2. Adding New Tools
244+
245+
1. Use dependency injection pattern
246+
2. Follow handler signature requirements
247+
3. Create comprehensive tests (test logic, not handlers)
248+
4. Use `createTypedTool` factory for type safety
249+
5. Document parameter schemas clearly
250+
251+
### 3. Writing Tests
252+
253+
1. Import the logic function, not the default export
254+
2. Use `createMockExecutor()` for mocking
255+
3. Test three dimensions: validation, command generation, output processing
256+
4. Never test handlers directly
257+
258+
### 4. Code Organization
259+
260+
1. Keep tools in appropriate workflow directories
261+
2. Share common tools via `-shared` directories
262+
3. Re-export shared tools, don't duplicate
263+
4. Follow naming conventions for tools
264+
265+
## Automated Enforcement
266+
267+
The project uses multiple layers of automated enforcement:
268+
269+
1. **Pre-commit**: ESLint and TypeScript checks (if configured)
270+
2. **CI Pipeline**: All checks run on every PR
271+
3. **PR Blocking**: Checks must pass before merge
272+
4. **Code Review**: Automated and manual review processes
273+
274+
## Troubleshooting
275+
276+
### ESLint False Positives
277+
278+
If ESLint reports false positives in test files, check that:
279+
1. Test files are properly configured in `.eslintrc.json`
280+
2. Test-specific rules are applied correctly
281+
3. File patterns match your test file locations
282+
283+
### Pattern Checker Issues
284+
285+
If the pattern checker reports unexpected violations:
286+
1. Check if it's a legitimate architectural violation
287+
2. Verify the file is in the correct directory
288+
3. Ensure you're using the latest pattern definitions
289+
290+
### Type Safety Migration
291+
292+
If migration tooling reports incorrect status:
293+
1. Ensure the tool exports follow standard patterns
294+
2. Check that schema definitions are properly typed
295+
3. Verify the handler uses the schema correctly
296+
297+
## Future Improvements
298+
299+
1. **Automated Fixes**: Add auto-fix capability to pattern checker
300+
2. **IDE Integration**: Create VS Code extension for real-time checking
301+
3. **Performance Metrics**: Add build and test performance tracking
302+
4. **Complexity Analysis**: Add code complexity metrics
303+
5. **Documentation Linting**: Add documentation quality checks

0 commit comments

Comments
 (0)