517 lines
13 KiB
Markdown
517 lines
13 KiB
Markdown
# TDD Refactoring Workflow
|
||
|
||
**Purpose**: Enforce test-driven discipline during refactoring to ensure behavior preservation and quality
|
||
|
||
**When to Use**: During ALL refactoring work
|
||
|
||
**Origin**: Iteration 1 - Problem E1 (No TDD Enforcement)
|
||
|
||
---
|
||
|
||
## TDD Principle for Refactoring
|
||
|
||
**Red-Green-Refactor Cycle** (adapted for refactoring existing code):
|
||
|
||
1. **Green** (Baseline): Ensure existing tests pass
|
||
2. **Red** (Add Tests): Write tests for uncovered behavior (tests should pass immediately since code exists)
|
||
3. **Refactor**: Restructure code while maintaining green tests
|
||
4. **Green** (Verify): Confirm all tests still pass after refactoring
|
||
|
||
**Key Difference from New Development TDD**:
|
||
- **New Development**: Write failing test → Make it pass → Refactor
|
||
- **Refactoring**: Ensure passing tests → Add missing tests (passing) → Refactor → Keep tests passing
|
||
|
||
---
|
||
|
||
## Workflow Steps
|
||
|
||
### Phase 1: Baseline Green (Ensure Safety Net)
|
||
|
||
**Goal**: Verify existing tests provide safety net for refactoring
|
||
|
||
#### Step 1: Run Existing Tests
|
||
|
||
```bash
|
||
go test -v ./internal/query/... > tests-baseline.txt
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] All existing tests pass: YES / NO
|
||
- [ ] Test count: ___ tests
|
||
- [ ] Duration: ___s
|
||
- [ ] If any fail: FIX BEFORE PROCEEDING
|
||
|
||
#### Step 2: Check Coverage
|
||
|
||
```bash
|
||
go test -cover ./internal/query/...
|
||
go test -coverprofile=coverage.out ./internal/query/...
|
||
go tool cover -html=coverage.out -o coverage.html
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] Overall coverage: ___%
|
||
- [ ] Target function coverage: ___%
|
||
- [ ] Uncovered lines identified: YES / NO
|
||
- [ ] Coverage file: `coverage.html` (review in browser)
|
||
|
||
#### Step 3: Identify Coverage Gaps
|
||
|
||
**Review `coverage.html` and identify**:
|
||
- [ ] Uncovered branches: _______________
|
||
- [ ] Uncovered error paths: _______________
|
||
- [ ] Uncovered edge cases: _______________
|
||
- [ ] Missing edge case examples:
|
||
- Empty inputs: ___ (e.g., empty slice, nil, zero)
|
||
- Boundary conditions: ___ (e.g., single element, max value)
|
||
- Error conditions: ___ (e.g., invalid input, out of range)
|
||
|
||
**Decision Point**:
|
||
- If coverage ≥95% on target code: Proceed to Phase 2 (Refactor)
|
||
- If coverage <95%: Proceed to Phase 1b (Write Missing Tests)
|
||
|
||
---
|
||
|
||
### Phase 1b: Write Missing Tests (Red → Immediate Green)
|
||
|
||
**Goal**: Add tests for uncovered code paths BEFORE refactoring
|
||
|
||
#### For Each Coverage Gap:
|
||
|
||
**1. Write Characterization Test** (documents current behavior):
|
||
|
||
```go
|
||
func TestCalculateSequenceTimeSpan_<EdgeCase>(t *testing.T) {
|
||
// Setup: Create input that triggers uncovered path
|
||
// ...
|
||
|
||
// Execute: Call function
|
||
result := calculateSequenceTimeSpan(occurrences, entries, toolCalls)
|
||
|
||
// Verify: Document current behavior (even if it's wrong)
|
||
assert.Equal(t, <expected>, result, "current behavior")
|
||
}
|
||
```
|
||
|
||
**Test Naming Convention**:
|
||
- `Test<FunctionName>_<EdgeCase>` (e.g., `TestCalculateTimeSpan_EmptyOccurrences`)
|
||
- `Test<FunctionName>_<Scenario>` (e.g., `TestCalculateTimeSpan_SingleOccurrence`)
|
||
|
||
**2. Verify Test Passes** (should pass immediately since code exists):
|
||
|
||
```bash
|
||
go test -v -run Test<FunctionName>_<EdgeCase> ./...
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] Test written: `Test<FunctionName>_<EdgeCase>`
|
||
- [ ] Test passes immediately: YES / NO
|
||
- [ ] If NO: Bug in test or unexpected current behavior → Fix test
|
||
- [ ] Coverage increased: __% → ___%
|
||
|
||
**3. Commit Test**:
|
||
|
||
```bash
|
||
git add <test_file>
|
||
git commit -m "test: add <edge-case> test for <function>"
|
||
```
|
||
|
||
**Repeat for all coverage gaps until target coverage ≥95%**
|
||
|
||
#### Coverage Target
|
||
|
||
- [ ] **Overall coverage**: ≥85% (project minimum)
|
||
- [ ] **Target function coverage**: ≥95% (refactoring requirement)
|
||
- [ ] **New test coverage**: ≥100% (all new tests pass)
|
||
|
||
**Checkpoint**: Before proceeding to refactoring:
|
||
- [ ] All tests pass: PASS
|
||
- [ ] Target function coverage: ≥95%
|
||
- [ ] Coverage gaps documented if <95%: _______________
|
||
|
||
---
|
||
|
||
### Phase 2: Refactor (Maintain Green)
|
||
|
||
**Goal**: Restructure code while keeping all tests passing
|
||
|
||
#### For Each Refactoring Step:
|
||
|
||
**1. Plan Single Refactoring Transformation**:
|
||
|
||
- [ ] Transformation type: _______________ (Extract Method, Inline, Rename, etc.)
|
||
- [ ] Target code: _______________ (function, lines, scope)
|
||
- [ ] Expected outcome: _______________ (complexity reduction, clarity, etc.)
|
||
- [ ] Estimated time: ___ minutes
|
||
|
||
**2. Make Minimal Change**:
|
||
|
||
**Examples**:
|
||
- Extract Method: Move lines X-Y to new function `<name>`
|
||
- Simplify Conditional: Replace nested if with guard clause
|
||
- Rename: Change `<oldName>` to `<newName>`
|
||
|
||
**Checklist**:
|
||
- [ ] Single, focused change: YES / NO
|
||
- [ ] No behavioral changes: Only structural / organizational
|
||
- [ ] Files modified: _______________
|
||
- [ ] Lines changed: ~___
|
||
|
||
**3. Run Tests Immediately**:
|
||
|
||
```bash
|
||
go test -v ./internal/query/... | tee test-results-step-N.txt
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] All tests pass: PASS / FAIL
|
||
- [ ] Duration: ___s (should be quick, <10s)
|
||
- [ ] If FAIL: **ROLLBACK IMMEDIATELY**
|
||
|
||
**4. Verify Coverage Maintained**:
|
||
|
||
```bash
|
||
go test -cover ./internal/query/...
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] Coverage: Before __% → After ___%
|
||
- [ ] Change: +/- ___%
|
||
- [ ] If decreased >1%: Investigate (might need to update tests)
|
||
- [ ] If decreased >5%: **ROLLBACK**
|
||
|
||
**5. Verify Complexity**:
|
||
|
||
```bash
|
||
gocyclo -over 10 internal/query/<target-file>.go
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] Target function complexity: ___
|
||
- [ ] Change from previous: +/- ___
|
||
- [ ] If increased: Not a valid refactoring step → ROLLBACK
|
||
|
||
**6. Commit Incremental Progress**:
|
||
|
||
```bash
|
||
git add .
|
||
git commit -m "refactor(<file>): <pattern> - <what changed>"
|
||
```
|
||
|
||
**Example Commit Messages**:
|
||
- `refactor(sequences): extract collectTimestamps helper`
|
||
- `refactor(sequences): simplify min/max calculation`
|
||
- `refactor(sequences): rename ts to timestamp for clarity`
|
||
|
||
**Checklist**:
|
||
- [ ] Commit hash: _______________
|
||
- [ ] Message follows convention: YES / NO
|
||
- [ ] Commit is small, focused: YES / NO
|
||
|
||
**Repeat refactoring steps until refactoring complete or target achieved**
|
||
|
||
---
|
||
|
||
### Phase 3: Final Verification (Confirm Green)
|
||
|
||
**Goal**: Comprehensive verification that refactoring succeeded
|
||
|
||
#### 1. Run Full Test Suite
|
||
|
||
```bash
|
||
go test -v ./... | tee test-results-final.txt
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] All tests pass: PASS / FAIL
|
||
- [ ] Test count: ___ (should match baseline or increase)
|
||
- [ ] Duration: ___s
|
||
- [ ] No flaky tests: All consistent
|
||
|
||
#### 2. Verify Coverage Improved or Maintained
|
||
|
||
```bash
|
||
go test -cover ./internal/query/...
|
||
go test -coverprofile=coverage-final.out ./internal/query/...
|
||
go tool cover -func=coverage-final.out | grep total
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] Baseline coverage: ___%
|
||
- [ ] Final coverage: ___%
|
||
- [ ] Change: +___%
|
||
- [ ] Target met (≥85% overall, ≥95% refactored code): YES / NO
|
||
|
||
#### 3. Compare Baseline and Final Metrics
|
||
|
||
| Metric | Baseline | Final | Change | Target Met |
|
||
|--------|----------|-------|--------|------------|
|
||
| **Complexity** | ___ | ___ | ___% | YES / NO |
|
||
| **Coverage** | ___% | ___% | +___% | YES / NO |
|
||
| **Test count** | ___ | ___ | +___ | N/A |
|
||
| **Test duration** | ___s | ___s | ___s | N/A |
|
||
|
||
**Checklist**:
|
||
- [ ] All targets met: YES / NO
|
||
- [ ] If NO: Document gaps and plan next iteration
|
||
|
||
#### 4. Update Documentation
|
||
|
||
```bash
|
||
# Add/update GoDoc comments for refactored code
|
||
# Example:
|
||
// calculateSequenceTimeSpan calculates the time span in minutes between
|
||
// the first and last occurrence of a sequence pattern across turns.
|
||
// Returns 0 if no valid timestamps found.
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] GoDoc added/updated: YES / NO
|
||
- [ ] Public functions documented: ___ / ___ (100%)
|
||
- [ ] Parameter descriptions clear: YES / NO
|
||
- [ ] Return value documented: YES / NO
|
||
|
||
---
|
||
|
||
## TDD Metrics (Track Over Time)
|
||
|
||
**Refactoring Session**: ___ (e.g., calculateSequenceTimeSpan - 2025-10-19)
|
||
|
||
| Metric | Value |
|
||
|--------|-------|
|
||
| **Baseline coverage** | ___% |
|
||
| **Final coverage** | ___% |
|
||
| **Coverage improvement** | +___% |
|
||
| **Tests added** | ___ |
|
||
| **Test failures during refactoring** | ___ |
|
||
| **Rollbacks due to test failures** | ___ |
|
||
| **Time spent writing tests** | ___ min |
|
||
| **Time spent refactoring** | ___ min |
|
||
| **Test writing : Refactoring ratio** | ___:1 |
|
||
|
||
**TDD Discipline Score**: (Tests passing after each step) / (Total steps) × 100% = ___%
|
||
|
||
**Target**: 100% TDD discipline (tests pass after EVERY step)
|
||
|
||
---
|
||
|
||
## Common TDD Refactoring Patterns
|
||
|
||
### Pattern 1: Extract Method with Tests
|
||
|
||
**Scenario**: Function too complex, need to extract helper
|
||
|
||
**Steps**:
|
||
1. ✅ Ensure tests pass
|
||
2. ✅ Write test for behavior to be extracted (if not covered)
|
||
3. ✅ Extract method
|
||
4. ✅ Tests still pass
|
||
5. ✅ Write direct test for new extracted method
|
||
6. ✅ Tests pass
|
||
7. ✅ Commit
|
||
|
||
**Example**:
|
||
```go
|
||
// Before:
|
||
func calculate() {
|
||
// ... 20 lines of timestamp collection
|
||
// ... 15 lines of min/max finding
|
||
}
|
||
|
||
// After:
|
||
func calculate() {
|
||
timestamps := collectTimestamps()
|
||
return findMinMax(timestamps)
|
||
}
|
||
|
||
func collectTimestamps() []int64 { /* extracted */ }
|
||
func findMinMax([]int64) int { /* extracted */ }
|
||
```
|
||
|
||
**Tests**:
|
||
- Existing: `TestCalculate` (still passes)
|
||
- New: `TestCollectTimestamps` (covers extracted logic)
|
||
- New: `TestFindMinMax` (covers min/max logic)
|
||
|
||
---
|
||
|
||
### Pattern 2: Simplify Conditionals with Tests
|
||
|
||
**Scenario**: Nested conditionals hard to read, need to simplify
|
||
|
||
**Steps**:
|
||
1. ✅ Ensure tests pass (covering all branches)
|
||
2. ✅ If branches uncovered: Add tests for all paths
|
||
3. ✅ Simplify conditionals (guard clauses, early returns)
|
||
4. ✅ Tests still pass
|
||
5. ✅ Commit
|
||
|
||
**Example**:
|
||
```go
|
||
// Before: Nested conditionals
|
||
if len(timestamps) > 0 {
|
||
minTs := timestamps[0]
|
||
maxTs := timestamps[0]
|
||
for _, ts := range timestamps[1:] {
|
||
if ts < minTs {
|
||
minTs = ts
|
||
}
|
||
if ts > maxTs {
|
||
maxTs = ts
|
||
}
|
||
}
|
||
return int((maxTs - minTs) / 60)
|
||
} else {
|
||
return 0
|
||
}
|
||
|
||
// After: Guard clause
|
||
if len(timestamps) == 0 {
|
||
return 0
|
||
}
|
||
minTs := timestamps[0]
|
||
maxTs := timestamps[0]
|
||
for _, ts := range timestamps[1:] {
|
||
if ts < minTs {
|
||
minTs = ts
|
||
}
|
||
if ts > maxTs {
|
||
maxTs = ts
|
||
}
|
||
}
|
||
return int((maxTs - minTs) / 60)
|
||
```
|
||
|
||
**Tests**: No new tests needed (behavior unchanged), existing tests verify correctness
|
||
|
||
---
|
||
|
||
### Pattern 3: Remove Duplication with Tests
|
||
|
||
**Scenario**: Duplicated code blocks, need to extract to shared helper
|
||
|
||
**Steps**:
|
||
1. ✅ Ensure tests pass
|
||
2. ✅ Identify duplication: Lines X-Y in File A same as Lines M-N in File B
|
||
3. ✅ Extract to shared helper
|
||
4. ✅ Replace first occurrence with helper call
|
||
5. ✅ Tests pass
|
||
6. ✅ Replace second occurrence
|
||
7. ✅ Tests pass
|
||
8. ✅ Commit
|
||
|
||
**Example**:
|
||
```go
|
||
// Before: Duplication
|
||
// File A:
|
||
if startTs > 0 {
|
||
timestamps = append(timestamps, startTs)
|
||
}
|
||
|
||
// File B:
|
||
if endTs > 0 {
|
||
timestamps = append(timestamps, endTs)
|
||
}
|
||
|
||
// After: Shared helper
|
||
func appendIfValid(timestamps []int64, ts int64) []int64 {
|
||
if ts > 0 {
|
||
return append(timestamps, ts)
|
||
}
|
||
return timestamps
|
||
}
|
||
|
||
// File A: timestamps = appendIfValid(timestamps, startTs)
|
||
// File B: timestamps = appendIfValid(timestamps, endTs)
|
||
```
|
||
|
||
**Tests**:
|
||
- Existing tests for Files A and B (still pass)
|
||
- New: `TestAppendIfValid` (covers helper)
|
||
|
||
---
|
||
|
||
## TDD Anti-Patterns (Avoid These)
|
||
|
||
### ❌ Anti-Pattern 1: "Skip Tests, Code Seems Fine"
|
||
|
||
**Problem**: Refactor without running tests
|
||
**Risk**: Break behavior without noticing
|
||
**Fix**: ALWAYS run tests after each change
|
||
|
||
### ❌ Anti-Pattern 2: "Write Tests After Refactoring"
|
||
|
||
**Problem**: Tests written to match new code (not verify behavior)
|
||
**Risk**: Tests pass but behavior changed
|
||
**Fix**: Write tests BEFORE refactoring (characterization tests)
|
||
|
||
### ❌ Anti-Pattern 3: "Batch Multiple Changes Before Testing"
|
||
|
||
**Problem**: Make 3-4 changes, then run tests
|
||
**Risk**: If tests fail, hard to identify which change broke it
|
||
**Fix**: Test after EACH change
|
||
|
||
### ❌ Anti-Pattern 4: "Update Tests to Match New Code"
|
||
|
||
**Problem**: Tests fail after refactoring, so "fix" tests
|
||
**Risk**: Masking behavioral changes
|
||
**Fix**: If tests fail, rollback refactoring → Fix code, not tests
|
||
|
||
### ❌ Anti-Pattern 5: "Low Coverage is OK for Refactoring"
|
||
|
||
**Problem**: Refactor code with <75% coverage
|
||
**Risk**: Behavioral changes not caught by tests
|
||
**Fix**: Achieve ≥95% coverage BEFORE refactoring
|
||
|
||
---
|
||
|
||
## Automation Support
|
||
|
||
**Continuous Testing** (automatically run tests on file save):
|
||
|
||
### Option 1: File Watcher (entr)
|
||
|
||
```bash
|
||
# Install entr
|
||
go install github.com/eradman/entr@latest
|
||
|
||
# Auto-run tests on file change
|
||
find internal/query -name '*.go' | entr -c go test ./internal/query/...
|
||
```
|
||
|
||
### Option 2: IDE Integration
|
||
|
||
- **VS Code**: Go extension auto-runs tests on save
|
||
- **GoLand**: Configure test auto-run in settings
|
||
- **Vim**: Use vim-go with `:GoTestFunc` on save
|
||
|
||
### Option 3: Pre-Commit Hook
|
||
|
||
```bash
|
||
# .git/hooks/pre-commit
|
||
#!/bin/bash
|
||
go test ./... || exit 1
|
||
go test -cover ./... | grep -E 'coverage: [0-9]+' || exit 1
|
||
```
|
||
|
||
**Checklist**:
|
||
- [ ] Automation setup: YES / NO
|
||
- [ ] Tests run automatically: YES / NO
|
||
- [ ] Feedback time: ___s (target <5s)
|
||
|
||
---
|
||
|
||
## Notes
|
||
|
||
- **TDD Discipline**: Tests must pass after EVERY single change
|
||
- **Small Steps**: Each refactoring step should take <10 minutes
|
||
- **Fast Tests**: Test suite should run in <10 seconds for fast feedback
|
||
- **No Guessing**: If unsure about behavior, write test to document it
|
||
- **Coverage Goal**: ≥95% for code being refactored, ≥85% overall
|
||
|
||
---
|
||
|
||
**Version**: 1.0 (Iteration 1)
|
||
**Next Review**: Iteration 2 (refine based on usage data)
|
||
**Automation**: See Problem V1 for automated complexity checking integration
|