Update test coverage plan with lessons learned and progress
Documentation updates after completing quick wins 1-2: Progress: - Tests: 1000 → 1045 (+45, +4.5%) - Coverage: 63% → ~65% (+2%) - Quick wins completed: 2/3 - Hours spent: ~4 hours - TEST-015 (CardBack): 25 tests, ~95% coverage - TEST-020 (Socket factories): 20 tests, 100% coverage Key Lessons Learned: 1. Phaser mocking pattern - mock classes inside vi.mock() factory 2. Disable ESLint explicit-any for complex Phaser mocks 3. Test docstrings are essential for maintainability 4. Always verify actual dimensions/constants before asserting 5. Include integration tests for full object lifecycle 6. Factory function testing strategy (structure, uniqueness, variations) 7. Avoid testing browser internals (crypto, etc) 8. Pre-commit hooks catch everything - working perfectly 9. Quick wins build momentum - start simple 10. Coverage updates automatically after tests added Updated PROJECT_PLAN_TEST_COVERAGE.json: - Mark TEST-015 and TEST-020 as completed - Add progress tracking metadata - Update current coverage estimate
This commit is contained in:
parent
56de143397
commit
d03dc1ddd2
@ -1,15 +1,24 @@
|
|||||||
{
|
{
|
||||||
"meta": {
|
"meta": {
|
||||||
"version": "1.0.0",
|
"version": "1.0.1",
|
||||||
"created": "2026-02-02",
|
"created": "2026-02-02",
|
||||||
"lastUpdated": "2026-02-02",
|
"lastUpdated": "2026-02-02",
|
||||||
|
"started": "2026-02-02",
|
||||||
"planType": "testing",
|
"planType": "testing",
|
||||||
"description": "Test coverage improvement plan - filling critical gaps in game engine, WebSocket, and gameplay code",
|
"description": "Test coverage improvement plan - filling critical gaps in game engine, WebSocket, and gameplay code",
|
||||||
"totalEstimatedHours": 120,
|
"totalEstimatedHours": 120,
|
||||||
"totalTasks": 35,
|
"totalTasks": 35,
|
||||||
"completedTasks": 0,
|
"completedTasks": 2,
|
||||||
"currentCoverage": "63%",
|
"currentCoverage": "~65%",
|
||||||
"targetCoverage": "85%"
|
"targetCoverage": "85%",
|
||||||
|
"progress": {
|
||||||
|
"testsAdded": 45,
|
||||||
|
"totalTests": 1045,
|
||||||
|
"quickWinsCompleted": 2,
|
||||||
|
"quickWinsRemaining": 1,
|
||||||
|
"hoursSpent": 4,
|
||||||
|
"coverageGain": "+2%"
|
||||||
|
}
|
||||||
},
|
},
|
||||||
"categories": {
|
"categories": {
|
||||||
"critical": "Zero or critically low coverage (<30%) on production code",
|
"critical": "Zero or critically low coverage (<30%) on production code",
|
||||||
@ -390,8 +399,12 @@
|
|||||||
"description": "Test CardBack.ts: face-down card display, back texture, positioning. Currently 0% coverage but simple component.",
|
"description": "Test CardBack.ts: face-down card display, back texture, positioning. Currently 0% coverage but simple component.",
|
||||||
"category": "medium",
|
"category": "medium",
|
||||||
"priority": 15,
|
"priority": 15,
|
||||||
"completed": false,
|
"completed": true,
|
||||||
"tested": false,
|
"tested": true,
|
||||||
|
"completedDate": "2026-02-02",
|
||||||
|
"actualHours": 2,
|
||||||
|
"testsAdded": 25,
|
||||||
|
"coverageAfter": "~95%",
|
||||||
"dependencies": ["TEST-001"],
|
"dependencies": ["TEST-001"],
|
||||||
"files": [
|
"files": [
|
||||||
{
|
{
|
||||||
@ -526,12 +539,16 @@
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
"id": "TEST-020",
|
"id": "TEST-020",
|
||||||
"name": "Test socket message type guards",
|
"name": "Test socket message factory functions",
|
||||||
"description": "Test socket/types.ts: type guard functions, message validation. Currently 0% coverage but may be all type definitions.",
|
"description": "Test socket/types.ts: message factory functions (createJoinGameMessage, createActionMessage, etc). Lines 308-361 were factory functions, not type guards.",
|
||||||
"category": "low",
|
"category": "low",
|
||||||
"priority": 20,
|
"priority": 20,
|
||||||
"completed": false,
|
"completed": true,
|
||||||
"tested": false,
|
"tested": true,
|
||||||
|
"completedDate": "2026-02-02",
|
||||||
|
"actualHours": 2,
|
||||||
|
"testsAdded": 20,
|
||||||
|
"coverageAfter": "100%",
|
||||||
"dependencies": [],
|
"dependencies": [],
|
||||||
"files": [
|
"files": [
|
||||||
{
|
{
|
||||||
|
|||||||
@ -1,15 +1,208 @@
|
|||||||
# Test Coverage Improvement Plan
|
# Test Coverage Improvement Plan
|
||||||
|
|
||||||
**Status:** Draft
|
**Status:** In Progress
|
||||||
**Created:** 2026-02-02
|
**Created:** 2026-02-02
|
||||||
|
**Started:** 2026-02-02
|
||||||
**Target Completion:** 6 weeks
|
**Target Completion:** 6 weeks
|
||||||
**Current Coverage:** 63%
|
**Current Coverage:** 63% → **~65%** (after quick wins 1-2)
|
||||||
**Target Coverage:** 85%
|
**Target Coverage:** 85%
|
||||||
|
|
||||||
See [`PROJECT_PLAN_TEST_COVERAGE.json`](./PROJECT_PLAN_TEST_COVERAGE.json) for full structured plan.
|
See [`PROJECT_PLAN_TEST_COVERAGE.json`](./PROJECT_PLAN_TEST_COVERAGE.json) for full structured plan.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
## 📝 Lessons Learned (Session 2026-02-02)
|
||||||
|
|
||||||
|
### Quick Wins Completed
|
||||||
|
- ✅ **TEST-015:** CardBack tests (25 tests, ~2h)
|
||||||
|
- ✅ **TEST-020:** Socket message factories (20 tests, ~2h)
|
||||||
|
- **Total:** 45 new tests, 1000 → 1045 tests (+4.5%)
|
||||||
|
|
||||||
|
### Key Learnings for Future Testing
|
||||||
|
|
||||||
|
#### 1. **Phaser Mocking Pattern (from TEST-015)**
|
||||||
|
|
||||||
|
**Problem:** Phaser classes can't be imported directly in tests (WebGL/Canvas dependencies)
|
||||||
|
|
||||||
|
**Solution:** Mock Phaser classes inline within `vi.mock()` factory function
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
// ✅ CORRECT - Define mock classes inside vi.mock factory
|
||||||
|
vi.mock('phaser', () => {
|
||||||
|
class MockContainerFactory {
|
||||||
|
x: number
|
||||||
|
y: number
|
||||||
|
// ... mock implementation
|
||||||
|
}
|
||||||
|
|
||||||
|
return {
|
||||||
|
default: {
|
||||||
|
GameObjects: {
|
||||||
|
Container: MockContainerFactory,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
// ❌ WRONG - Referencing external classes causes hoisting issues
|
||||||
|
class MockContainer { /* ... */ }
|
||||||
|
vi.mock('phaser', () => ({
|
||||||
|
default: {
|
||||||
|
GameObjects: {
|
||||||
|
Container: MockContainer, // ReferenceError: Cannot access before initialization
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}))
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why:** `vi.mock()` is hoisted to the top of the file, so it runs before any class definitions. Mock classes must be defined within the factory function.
|
||||||
|
|
||||||
|
#### 2. **Disabling ESLint for Test Mocks**
|
||||||
|
|
||||||
|
When mocking complex Phaser objects, `any` types are acceptable in tests:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
/* eslint-disable @typescript-eslint/no-explicit-any */
|
||||||
|
// Disabling explicit-any for test mocks - Phaser types are complex and mocking requires any
|
||||||
|
```
|
||||||
|
|
||||||
|
**Rationale:** Mocking Phaser's deep type hierarchies is impractical. Focus on testing behavior, not perfect type accuracy in mocks.
|
||||||
|
|
||||||
|
#### 3. **Test Docstrings Are Essential**
|
||||||
|
|
||||||
|
Every test must include a docstring explaining "what" and "why":
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
it('creates a card back with sprite when texture exists', () => {
|
||||||
|
/**
|
||||||
|
* Test that CardBack uses sprite when texture is available.
|
||||||
|
*
|
||||||
|
* When the card back texture is loaded, CardBack should create
|
||||||
|
* a sprite instead of fallback graphics.
|
||||||
|
*/
|
||||||
|
// test implementation
|
||||||
|
})
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why:** These docstrings make tests self-documenting and help future developers understand intent.
|
||||||
|
|
||||||
|
#### 4. **Test Actual Dimensions, Not Assumptions**
|
||||||
|
|
||||||
|
**Mistake Made:** Initially used wrong card dimensions (assumed 120x167 for large, actual was 150x210)
|
||||||
|
|
||||||
|
**Lesson:** Always check actual constants/values in codebase before writing assertions. Don't assume dimensions or magic numbers.
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
// ✅ Verified actual CARD_SIZES from types/phaser.ts
|
||||||
|
expect(dimensions.width).toBe(150) // large: 150x210
|
||||||
|
expect(dimensions.height).toBe(210)
|
||||||
|
|
||||||
|
// ❌ Assumed without checking
|
||||||
|
expect(dimensions.width).toBe(120) // Wrong!
|
||||||
|
```
|
||||||
|
|
||||||
|
#### 5. **Integration Tests Validate Full Lifecycle**
|
||||||
|
|
||||||
|
Include at least one integration test that exercises the full object lifecycle:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
it('can create, resize, and destroy a card back', () => {
|
||||||
|
/**
|
||||||
|
* Test full lifecycle of a CardBack.
|
||||||
|
*
|
||||||
|
* This integration test verifies that a CardBack can be
|
||||||
|
* created, resized multiple times, and destroyed without errors.
|
||||||
|
*/
|
||||||
|
const cardBack = new CardBack(mockScene, 100, 200, 'small')
|
||||||
|
cardBack.setCardSize('medium')
|
||||||
|
cardBack.setCardSize('large')
|
||||||
|
expect(() => cardBack.destroy()).not.toThrow()
|
||||||
|
})
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why:** Unit tests verify individual methods, integration tests verify they work together correctly.
|
||||||
|
|
||||||
|
#### 6. **Factory Function Testing Strategy**
|
||||||
|
|
||||||
|
When testing message/object factories (TEST-020):
|
||||||
|
|
||||||
|
1. **Test structure first** - Verify all required fields present
|
||||||
|
2. **Test uniqueness** - IDs/tokens should be unique on each call
|
||||||
|
3. **Test variations** - Test with/without optional parameters
|
||||||
|
4. **Test integration** - Verify factories work together (same game, different messages)
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
// Structure
|
||||||
|
expect(message.type).toBe('join_game')
|
||||||
|
expect(message.message_id).toBeTruthy()
|
||||||
|
|
||||||
|
// Uniqueness
|
||||||
|
const msg1 = createJoinGameMessage('game-1')
|
||||||
|
const msg2 = createJoinGameMessage('game-1')
|
||||||
|
expect(msg1.message_id).not.toBe(msg2.message_id)
|
||||||
|
|
||||||
|
// Variations
|
||||||
|
const withLastEvent = createJoinGameMessage('game-1', 'event-123')
|
||||||
|
expect(withLastEvent.last_event_id).toBe('event-123')
|
||||||
|
|
||||||
|
const withoutLastEvent = createJoinGameMessage('game-1')
|
||||||
|
expect(withoutLastEvent.last_event_id).toBeUndefined()
|
||||||
|
```
|
||||||
|
|
||||||
|
#### 7. **Avoid Testing Browser Internals**
|
||||||
|
|
||||||
|
**Mistake Made:** Attempted to mock `global.crypto` which is read-only
|
||||||
|
|
||||||
|
**Lesson:** Don't test browser APIs directly. Test your wrapper functions instead. If a function uses `crypto.randomUUID()`, test that it returns valid UUIDs, not that it calls the browser API.
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
// ✅ Test the wrapper's behavior
|
||||||
|
it('generates a unique message ID', () => {
|
||||||
|
const id = generateMessageId()
|
||||||
|
expect(id).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i)
|
||||||
|
})
|
||||||
|
|
||||||
|
// ❌ Don't test browser internals
|
||||||
|
it('uses crypto.randomUUID when available', () => {
|
||||||
|
global.crypto = { randomUUID: vi.fn() } // TypeError: Cannot set property
|
||||||
|
})
|
||||||
|
```
|
||||||
|
|
||||||
|
#### 8. **Pre-commit Hooks Catch Everything**
|
||||||
|
|
||||||
|
Since fixing pre-existing lint errors and enabling pre-commit hooks (PR #2):
|
||||||
|
- All TypeScript errors caught immediately
|
||||||
|
- All lint errors caught immediately
|
||||||
|
- All test failures caught immediately
|
||||||
|
- **No bypassing with --no-verify**
|
||||||
|
|
||||||
|
**Impact:** Zero bugs made it into commits during this session. Pre-commit hooks are working perfectly.
|
||||||
|
|
||||||
|
#### 9. **Quick Wins Build Momentum**
|
||||||
|
|
||||||
|
Starting with simple, high-value tests (CardBack, socket factories) built confidence and validated patterns:
|
||||||
|
|
||||||
|
- CardBack: Simple game object, validated Phaser mocking
|
||||||
|
- Socket factories: Straightforward logic, 100% coverage quickly
|
||||||
|
- Both: Provided immediate value (45 tests) in 4 hours
|
||||||
|
|
||||||
|
**Strategy:** When starting a new test area, pick the simplest component first to validate your approach.
|
||||||
|
|
||||||
|
#### 10. **Coverage Numbers Update Automatically**
|
||||||
|
|
||||||
|
After adding tests, re-run coverage to see actual improvement:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
npm run test -- --coverage
|
||||||
|
```
|
||||||
|
|
||||||
|
**Observed Gains:**
|
||||||
|
- CardBack: 0% → ~95% coverage (238 lines)
|
||||||
|
- Socket factories: 0% → 100% coverage (54 lines)
|
||||||
|
- Overall: 63% → ~65% (+2%)
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
## Executive Summary
|
## Executive Summary
|
||||||
|
|
||||||
The Mantimon TCG frontend has **excellent test discipline** (1000 passing tests) but has **critical gaps** in the game engine layer:
|
The Mantimon TCG frontend has **excellent test discipline** (1000 passing tests) but has **critical gaps** in the game engine layer:
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user