Skip to content

Commit 1a578d9

Browse files
Eibon7claude
andcommitted
docs(patterns): Add Jest integration test lessons - Issue #618
Added comprehensive lessons from test fixing session (2025-10-20): ### New Pattern: Jest Integration Tests & Module Loading **5 Mistake-Fix pairs documented:** 1. Router mounting order (duplicate endpoints) 2. Module-level calls without defensive checks (flags.isEnabled) 3. Rate limiters in test environment (express-rate-limit errors) 4. Global mocks interfering with integration tests 5. External dependencies not available in test environment **Impact documented:** - Before: 0/24 tests passing (100% failure) - After: 21/24 tests passing (87.5% success) - Discovered critical production bug **Prevention checklist added** with 7 items Related: Issue #618 - Test suite stabilization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 6e3e1a6 commit 1a578d9

File tree

1 file changed

+158
-3
lines changed

1 file changed

+158
-3
lines changed

docs/patterns/coderabbit-lessons.md

Lines changed: 158 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
**Purpose:** Document recurring patterns from CodeRabbit reviews to prevent repetition and improve code quality.
44

5-
**Last Updated:** 2025-10-16
5+
**Last Updated:** 2025-10-20
66

77
**Usage:** Read this file BEFORE implementing any feature (FASE 0 or FASE 2 of task workflow).
88

@@ -379,6 +379,161 @@ Antes de escribir código, verificar:
379379

380380
---
381381

382+
### 9. Jest Integration Tests & Module Loading
383+
384+
**Pattern:** Tests failing with "is not a function" errors, duplicate endpoints intercepting routes, rate limiters breaking tests
385+
386+
**Issue:** Test Fixing Session (2025-10-20) - From 0% to 87.5% passing
387+
388+
**❌ Mistake 1: Router Mounting Order**
389+
```javascript
390+
// Wrong: Generic routes registered before specific ones
391+
app.use('/api', dashboardRoutes); // Has /roast/preview
392+
app.use('/api/roast', roastRoutes); // Has /preview (intercepted!)
393+
```
394+
395+
**✅ Fix:**
396+
```javascript
397+
// Correct: Most specific routes first, or remove duplicates
398+
// Option 1: Remove duplicate from dashboard
399+
// Option 2: Register specific routes first
400+
app.use('/api/roast', roastRoutes); // Register first
401+
app.use('/api', dashboardRoutes); // Generic last
402+
```
403+
404+
**❌ Mistake 2: Module-level calls without defensive checks**
405+
```javascript
406+
// src/routes/roast.js
407+
const { flags } = require('../config/flags');
408+
409+
// Called at module load time - breaks in Jest
410+
if (flags.isEnabled('ENABLE_REAL_OPENAI')) {
411+
roastGenerator = new RoastGeneratorEnhanced();
412+
}
413+
```
414+
415+
**✅ Fix:**
416+
```javascript
417+
// Add defensive helper function
418+
const isFlagEnabled = (flagName) => {
419+
try {
420+
return flags && typeof flags.isEnabled === 'function' && flags.isEnabled(flagName);
421+
} catch (error) {
422+
logger.warn(`⚠️ Error checking flag ${flagName}:`, error.message);
423+
return false;
424+
}
425+
};
426+
427+
if (isFlagEnabled('ENABLE_REAL_OPENAI')) {
428+
roastGenerator = new RoastGeneratorEnhanced();
429+
}
430+
```
431+
432+
**❌ Mistake 3: Rate limiters in test environment**
433+
```javascript
434+
// express-rate-limit throws errors with trust proxy in Jest
435+
const roastRateLimit = createRoastRateLimiter();
436+
router.post('/preview', roastRateLimit, handler);
437+
// Error: ERR_ERL_PERMISSIVE_TRUST_PROXY
438+
```
439+
440+
**✅ Fix:**
441+
```javascript
442+
function createRoastRateLimiter(options = {}) {
443+
// Issue #618: Disable rate limiting in test environment
444+
if (process.env.NODE_ENV === 'test') {
445+
return (req, res, next) => next();
446+
}
447+
448+
// Normal rate limiter setup for production
449+
return (req, res, next) => { /* ... */ };
450+
}
451+
```
452+
453+
**❌ Mistake 4: Global mocks interfering with integration tests**
454+
```javascript
455+
// tests/setupEnvOnly.js
456+
jest.mock('../src/config/flags', () => ({
457+
flags: { isEnabled: jest.fn() }
458+
}));
459+
// This breaks ALL tests, including integration tests that need real behavior
460+
```
461+
462+
**✅ Fix:**
463+
```javascript
464+
// Remove global mocks from setupEnvOnly.js
465+
// Let each unit test define its own mocks:
466+
// tests/unit/service.test.js
467+
jest.mock('../../src/config/flags', () => ({
468+
flags: { isEnabled: jest.fn() }
469+
}));
470+
```
471+
472+
**❌ Mistake 5: External dependencies not available in test**
473+
```javascript
474+
// src/services/perspectiveService.js
475+
this.client = google.commentanalyzer({ // Breaks in Jest
476+
version: 'v1alpha1',
477+
auth: this.apiKey
478+
});
479+
```
480+
481+
**✅ Fix:**
482+
```javascript
483+
try {
484+
// Check if available before using
485+
if (!google || typeof google.commentanalyzer !== 'function') {
486+
logger.warn('Google Perspective API client not available (likely test environment)');
487+
this.enabled = false;
488+
return;
489+
}
490+
491+
this.client = google.commentanalyzer({
492+
version: 'v1alpha1',
493+
auth: this.apiKey
494+
});
495+
} catch (error) {
496+
logger.warn('⚠️ Failed to initialize Perspective API:', error.message);
497+
this.enabled = false;
498+
}
499+
```
500+
501+
**Impact:**
502+
- **Before:** 0/24 tests passing (100% failure)
503+
- **After:** 21/24 tests passing (87.5% success)
504+
- **Discovered:** Critical production bug (duplicate endpoint serving wrong responses)
505+
506+
**Files affected:**
507+
- src/routes/dashboard.js (duplicate endpoint removed)
508+
- src/routes/roast.js (defensive flag checks)
509+
- src/services/perspectiveService.js (defensive initialization)
510+
- src/middleware/roastRateLimiter.js (test environment no-op)
511+
- tests/setupEnvOnly.js (global mock removed)
512+
- tests/integration/roast.test.js (rewritten for production quality)
513+
514+
**Prevention checklist:**
515+
- [ ] Check router mounting order (specific before generic)
516+
- [ ] Add defensive checks for module-level calls
517+
- [ ] Disable rate limiters in test environment
518+
- [ ] Avoid global mocks in setup files
519+
- [ ] Check external dependencies availability before use
520+
- [ ] Test integration tests actually test production code paths
521+
- [ ] Verify no duplicate endpoints across route files
522+
523+
**Occurrences:**
524+
- Router order: 1 (dashboard intercepting roast)
525+
- Module loading: 3 (flags, perspectiveService, roastEngine)
526+
- Rate limiter: Multiple (all routes with rate limiting)
527+
- Global mocks: 1 (flags mock in setupEnvOnly)
528+
529+
**Last occurrence:** 2025-10-20 (Issue #618)
530+
531+
**Related patterns:**
532+
- Testing Patterns (#2) - Write production-quality tests
533+
- Integration Workflow - Check for duplicates before adding endpoints
534+
535+
---
536+
382537
## 📚 Related Documentation
383538

384539
- [Quality Standards](../QUALITY-STANDARDS.md) - Non-negotiable requirements for merge
@@ -390,5 +545,5 @@ Antes de escribir código, verificar:
390545

391546
**Maintained by:** Orchestrator
392547
**Review Frequency:** Weekly or after significant reviews
393-
**Last Reviewed:** 2025-10-16
394-
**Version:** 1.2.0
548+
**Last Reviewed:** 2025-10-20
549+
**Version:** 1.3.0

0 commit comments

Comments
 (0)