chore: clean up root directory cruft
- Delete duplicate install.sh (scripts/install.sh is canonical) - Delete BD-3S8-CHANGES.md (implementation now in git history) - Delete .saved-stashes/ (3 obsolete patch files) - Move internal dev docs to docs/dev-notes/: - ERROR_HANDLING_AUDIT.md - MAIN_TEST_CLEANUP_PLAN.md - MAIN_TEST_REFACTOR_NOTES.md - TEST_SUITE_AUDIT.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
913
docs/dev-notes/ERROR_HANDLING_AUDIT.md
Normal file
913
docs/dev-notes/ERROR_HANDLING_AUDIT.md
Normal file
@@ -0,0 +1,913 @@
|
||||
# Error Handling Audit Report
|
||||
**Date:** 2025-11-24
|
||||
**Issue:** bd-1qwo
|
||||
**Scope:** cmd/bd/*.go
|
||||
|
||||
This document audits error handling patterns across the beads CLI codebase to ensure consistency with the guidelines established in [ERROR_HANDLING.md](ERROR_HANDLING.md).
|
||||
|
||||
## Executive Summary
|
||||
|
||||
**Status:** 🟡 Needs Improvement
|
||||
**Files Audited:** create.go, init.go, sync.go, export.go, import.go
|
||||
**Patterns Found:**
|
||||
- ✅ Pattern A (Exit): Generally consistent
|
||||
- ⚠️ Pattern B (Warn): Some inconsistencies found
|
||||
- ✅ Pattern C (Ignore): Correctly applied
|
||||
|
||||
**Key Findings:**
|
||||
1. **Metadata operations** are handled inconsistently - some use Pattern A (fatal), some use Pattern B (warn)
|
||||
2. **File permission errors** mostly use Pattern B correctly
|
||||
3. **Cleanup operations** correctly use Pattern C
|
||||
4. **Similar auxiliary operations** sometimes use different patterns
|
||||
|
||||
---
|
||||
|
||||
## Pattern A: Exit Immediately ✅ MOSTLY CONSISTENT
|
||||
|
||||
### Correct Usage Examples
|
||||
|
||||
#### User Input Validation
|
||||
All validation failures correctly use Pattern A with clear error messages:
|
||||
|
||||
**create.go:31-32, 46-49, 57-58**
|
||||
```go
|
||||
if len(args) > 0 {
|
||||
fmt.Fprintf(os.Stderr, "Error: cannot specify both title and --file flag\n")
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**create.go:74-76, 107-108**
|
||||
```go
|
||||
tmpl, err := loadTemplate(fromTemplate)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**create.go:199-222** - ID validation
|
||||
```go
|
||||
requestedPrefix, err := validation.ValidateIDFormat(explicitID)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
#### Critical Database Operations
|
||||
Core database operations correctly use Pattern A:
|
||||
|
||||
**create.go:320-323**
|
||||
```go
|
||||
if err := store.CreateIssue(ctx, issue, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:77-78, 96-97, 104-105, 112-115**
|
||||
```go
|
||||
cwd, err := os.Getwd()
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to get current directory: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:201-204**
|
||||
```go
|
||||
store, err := sqlite.New(ctx, initDBPath)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to create database: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**sync.go:52-54, 59-60**
|
||||
```go
|
||||
if jsonlPath == "" {
|
||||
fmt.Fprintf(os.Stderr, "Error: not in a bd workspace (no .beads directory found)\n")
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**sync.go:82-83, 110-117, 122-124**
|
||||
```go
|
||||
if err := showSyncStatus(ctx); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**export.go:146-148**
|
||||
```go
|
||||
if format != "jsonl" {
|
||||
fmt.Fprintf(os.Stderr, "Error: only 'jsonl' format is currently supported\n")
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**export.go:205-207, 212-215, 223-225, 231-233, 239-241, 247-249**
|
||||
```go
|
||||
priorityMin, err := validation.ValidatePriority(priorityMinStr)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error parsing --priority-min: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**export.go:256-259**
|
||||
```go
|
||||
issues, err := store.SearchIssues(ctx, "", filter)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**export.go:270-277** - Safety check with clear guidance
|
||||
```go
|
||||
fmt.Fprintf(os.Stderr, "Error: refusing to export empty database over non-empty JSONL file\n")
|
||||
fmt.Fprintf(os.Stderr, " Database has 0 issues, JSONL has %d issues\n", existingCount)
|
||||
fmt.Fprintf(os.Stderr, " This would result in data loss!\n")
|
||||
fmt.Fprintf(os.Stderr, "Hint: Use --force to override this safety check, or delete the JSONL file first:\n")
|
||||
fmt.Fprintf(os.Stderr, " bd export -o %s --force\n", output)
|
||||
os.Exit(1)
|
||||
```
|
||||
|
||||
**import.go:42-44**
|
||||
```go
|
||||
if err := os.MkdirAll(dbDir, 0750); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to create database directory: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**import.go:55-58, 92-94**
|
||||
```go
|
||||
store, err = sqlite.New(rootCtx, dbPath)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to open database: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**import.go:76-84** - Interactive mode detection
|
||||
```go
|
||||
if input == "" && term.IsTerminal(int(os.Stdin.Fd())) {
|
||||
fmt.Fprintf(os.Stderr, "Error: No input specified.\n\n")
|
||||
fmt.Fprintf(os.Stderr, "Usage:\n")
|
||||
// ... helpful usage message
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**import.go:177-179**
|
||||
```go
|
||||
if err := json.Unmarshal([]byte(line), &issue); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error parsing line %d: %v\n", lineNum, err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**import.go:184-187**
|
||||
```go
|
||||
if err := scanner.Err(); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error reading input: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**import.go:199-202**
|
||||
```go
|
||||
cwd, err := os.Getwd()
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to get current directory: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**import.go:207-210**
|
||||
```go
|
||||
if err := store.SetConfig(initCtx, "issue_prefix", detectedPrefix); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to set issue prefix: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**import.go:247, 258, 260-261** - Error handling with detailed reports
|
||||
```go
|
||||
if result != nil && len(result.CollisionIDs) > 0 {
|
||||
fmt.Fprintf(os.Stderr, "\n=== Collision Detection Report ===\n")
|
||||
// ... detailed report
|
||||
os.Exit(1)
|
||||
}
|
||||
fmt.Fprintf(os.Stderr, "Import failed: %v\n", err)
|
||||
os.Exit(1)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Pattern B: Warn and Continue ⚠️ INCONSISTENCIES FOUND
|
||||
|
||||
### ✅ Correct Usage
|
||||
|
||||
#### Optional File Creation
|
||||
**create.go:333-334, 340-341**
|
||||
```go
|
||||
if err := store.AddLabel(ctx, issue.ID, label, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to add label %s: %v\n", label, err)
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:155-157, 161-163, 167-169**
|
||||
```go
|
||||
if err := createConfigYaml(localBeadsDir, false); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to create config.yaml: %v\n", err)
|
||||
// Non-fatal - continue anyway
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:236-238, 272-274, 284-286**
|
||||
```go
|
||||
if err := store.SetMetadata(ctx, "bd_version", Version); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to store version metadata: %v\n", err)
|
||||
// Non-fatal - continue anyway
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:247-248, 262-263**
|
||||
```go
|
||||
if err := store.SetMetadata(ctx, "repo_id", repoID); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to set repo_id: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
#### Git Hook Installation
|
||||
**init.go:332-336**
|
||||
```go
|
||||
if err := installGitHooks(); err != nil && !quiet {
|
||||
yellow := color.New(color.FgYellow).SprintFunc()
|
||||
fmt.Fprintf(os.Stderr, "\n%s Failed to install git hooks: %v\n", yellow("⚠"), err)
|
||||
fmt.Fprintf(os.Stderr, "You can try again with: %s\n\n", color.New(color.FgCyan).Sprint("bd doctor --fix"))
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:341-346** - Merge driver installation
|
||||
```go
|
||||
if err := installMergeDriver(); err != nil && !quiet {
|
||||
yellow := color.New(color.FgYellow).SprintFunc()
|
||||
fmt.Fprintf(os.Stderr, "\n%s Failed to install merge driver: %v\n", yellow("⚠"), err)
|
||||
fmt.Fprintf(os.Stderr, "You can try again with: %s\n\n", color.New(color.FgCyan).Sprint("bd doctor --fix"))
|
||||
}
|
||||
```
|
||||
|
||||
#### Import/Export Warnings
|
||||
**sync.go:156, 257, 329, 335**
|
||||
```go
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to count issues before import: %v\n", err)
|
||||
```
|
||||
|
||||
**sync.go:161-164**
|
||||
```go
|
||||
if orphaned, err := checkOrphanedDeps(ctx, store); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: orphaned dependency check failed: %v\n", err)
|
||||
} else if len(orphaned) > 0 {
|
||||
fmt.Fprintf(os.Stderr, "Warning: found %d orphaned dependencies: %v\n", len(orphaned), orphaned)
|
||||
}
|
||||
```
|
||||
|
||||
**sync.go:720-722, 740-743, 750-752, 760-762**
|
||||
```go
|
||||
if err := os.Chmod(jsonlPath, 0600); err != nil {
|
||||
// Non-fatal warning
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to set file permissions: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
**export.go:30, 59**
|
||||
```go
|
||||
if err := file.Close(); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to close file: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
**export.go:267**
|
||||
```go
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to read existing JSONL: %v\n", err)
|
||||
```
|
||||
|
||||
**import.go:98, 159**
|
||||
```go
|
||||
if err := f.Close(); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to close input file: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
### ⚠️ INCONSISTENCY: Metadata Operations
|
||||
|
||||
**Issue:** Metadata operations are handled inconsistently across files. Some treat them as fatal (Pattern A), others as warnings (Pattern B).
|
||||
|
||||
#### Pattern A (Exit) - Less Common
|
||||
**init.go:207-210**
|
||||
```go
|
||||
// Sets issue prefix - FATAL
|
||||
if err := store.SetConfig(ctx, "issue_prefix", prefix); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to set issue prefix: %v\n", err)
|
||||
_ = store.Close()
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:224-228**
|
||||
```go
|
||||
// Sets sync branch - FATAL
|
||||
if err := syncbranch.Set(ctx, store, branch); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: failed to set sync branch: %v\n", err)
|
||||
_ = store.Close()
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
#### Pattern B (Warn) - More Common
|
||||
**init.go:236-238**
|
||||
```go
|
||||
// Stores version metadata - WARNING
|
||||
if err := store.SetMetadata(ctx, "bd_version", Version); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to store version metadata: %v\n", err)
|
||||
// Non-fatal - continue anyway
|
||||
}
|
||||
```
|
||||
|
||||
**init.go:247-248, 262-263**
|
||||
```go
|
||||
// Stores repo_id and clone_id - WARNING
|
||||
if err := store.SetMetadata(ctx, "repo_id", repoID); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to set repo_id: %v\n", err)
|
||||
}
|
||||
if err := store.SetMetadata(ctx, "clone_id", cloneID); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to set clone_id: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
**sync.go:740-752** - Multiple metadata warnings
|
||||
```go
|
||||
if err := store.SetMetadata(ctx, "last_import_hash", currentHash); err != nil {
|
||||
// Non-fatal warning: Metadata update failures are intentionally non-fatal...
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to update last_import_hash: %v\n", err)
|
||||
}
|
||||
if err := store.SetMetadata(ctx, "last_import_time", exportTime); err != nil {
|
||||
// Non-fatal warning (see above comment about graceful degradation)
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to update last_import_time: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
**Recommendation:**
|
||||
Based on the documentation and intent, metadata operations should follow this pattern:
|
||||
- **Configuration metadata** (issue_prefix, sync.branch): **Pattern A** - These are fundamental to operation
|
||||
- **Tracking metadata** (bd_version, repo_id, last_import_hash): **Pattern B** - These enhance functionality but system works without them
|
||||
|
||||
**Action Required:** Review init.go:207-228 to ensure configuration vs. tracking metadata distinction is clear.
|
||||
|
||||
---
|
||||
|
||||
## Pattern C: Silent Ignore ✅ CONSISTENT
|
||||
|
||||
### Correct Usage
|
||||
|
||||
#### Resource Cleanup
|
||||
**init.go:209, 326-327**
|
||||
```go
|
||||
_ = store.Close()
|
||||
```
|
||||
|
||||
**sync.go:696-698, 701-703**
|
||||
```go
|
||||
defer func() {
|
||||
_ = tempFile.Close()
|
||||
if writeErr != nil {
|
||||
_ = os.Remove(tempPath)
|
||||
}
|
||||
}()
|
||||
```
|
||||
|
||||
#### Deferred Operations
|
||||
**create.go, init.go, sync.go** - Multiple instances of cleanup in defer blocks
|
||||
```go
|
||||
defer func() { _ = store.Close() }()
|
||||
```
|
||||
|
||||
All cleanup operations correctly use Pattern C with no user-visible output.
|
||||
|
||||
---
|
||||
|
||||
## Specific Inconsistencies Identified
|
||||
|
||||
### 1. Parent-Child Dependency Addition
|
||||
**create.go:327-335**
|
||||
```go
|
||||
// Pattern B - warn on parent-child dependency failure
|
||||
if parentID != "" {
|
||||
dep := &types.Dependency{
|
||||
IssueID: issue.ID,
|
||||
DependsOnID: parentID,
|
||||
Type: types.DepParentChild,
|
||||
}
|
||||
if err := store.AddDependency(ctx, dep, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to add parent-child dependency %s -> %s: %v\n", issue.ID, parentID, err)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**create.go:382-384** - Regular dependencies (same pattern)
|
||||
```go
|
||||
if err := store.AddDependency(ctx, dep, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to add dependency %s -> %s: %v\n", issue.ID, dependsOnID, err)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Correct - Dependencies are auxiliary to issue creation. The issue exists even if dependencies fail.
|
||||
|
||||
### 2. Label Addition
|
||||
**create.go:338-342**
|
||||
```go
|
||||
for _, label := range labels {
|
||||
if err := store.AddLabel(ctx, issue.ID, label, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to add label %s: %v\n", label, err)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Correct - Labels are auxiliary. The issue exists even if labels fail to attach.
|
||||
|
||||
### 3. File Permission Changes
|
||||
**sync.go:724-727**
|
||||
```go
|
||||
if err := os.Chmod(jsonlPath, 0600); err != nil {
|
||||
// Non-fatal warning
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to set file permissions: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Correct - File was already written successfully. Permissions are a security enhancement.
|
||||
|
||||
### 4. Database Mtime Updates
|
||||
**sync.go:756-762**
|
||||
```go
|
||||
if err := TouchDatabaseFile(dbPath, jsonlPath); err != nil {
|
||||
// Non-fatal warning
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to update database mtime: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Correct - This is metadata tracking for optimization, not critical functionality.
|
||||
|
||||
---
|
||||
|
||||
## Recommendations
|
||||
|
||||
### High Priority
|
||||
|
||||
1. **Document Metadata Distinction**
|
||||
- Create clear guidelines for "configuration metadata" vs "tracking metadata"
|
||||
- Configuration metadata (issue_prefix, sync.branch): Pattern A
|
||||
- Tracking metadata (bd_version, repo_id, last_import_*): Pattern B
|
||||
- Update ERROR_HANDLING.md with this distinction
|
||||
|
||||
2. **Review init.go Metadata Handling**
|
||||
- Lines 207-228: Ensure SetConfig operations are consistently Pattern A
|
||||
- Lines 236-263: Ensure SetMetadata operations are consistently Pattern B
|
||||
- Consider extracting metadata operations into a helper to enforce consistency
|
||||
|
||||
### Medium Priority
|
||||
|
||||
3. **Standardize Error Message Format**
|
||||
- All Pattern A errors: `Error: <message>`
|
||||
- All Pattern B warnings: `Warning: <message>`
|
||||
- Already mostly consistent, audit revealed good adherence
|
||||
|
||||
4. **Add Context to Warnings**
|
||||
- Pattern B warnings could benefit from actionable hints
|
||||
- Example (already good): init.go:332-336 suggests `bd doctor --fix`
|
||||
- Consider adding hints to other warnings where applicable
|
||||
|
||||
### Low Priority
|
||||
|
||||
5. **Consider Helper Functions**
|
||||
- Extract common patterns into helpers (as suggested in ERROR_HANDLING.md)
|
||||
- Example: `FatalError(format string, args ...interface{})`
|
||||
- Example: `WarnError(format string, args ...interface{})`
|
||||
- Would reduce boilerplate and enforce consistency
|
||||
|
||||
---
|
||||
|
||||
## Files Not Yet Audited
|
||||
|
||||
The following files in cmd/bd/ still need review:
|
||||
- daemon_sync.go
|
||||
- update.go
|
||||
- list.go
|
||||
- show.go
|
||||
- close.go
|
||||
- reopen.go
|
||||
- dep.go
|
||||
- label.go
|
||||
- comments.go
|
||||
- delete.go
|
||||
- compact.go
|
||||
- config.go
|
||||
- validate.go
|
||||
- doctor/* (doctor package files)
|
||||
- And ~50 more command files
|
||||
|
||||
**Next Steps:** Extend audit to remaining files, focusing on high-usage commands first.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
### Pattern Compliance Scorecard
|
||||
|
||||
| Pattern | Status | Compliance Rate | Issues Found |
|
||||
|---------|--------|-----------------|--------------|
|
||||
| Pattern A (Exit) | ✅ Excellent | ~95% | Minor: metadata distinction |
|
||||
| Pattern B (Warn) | ⚠️ Good | ~90% | Moderate: metadata handling |
|
||||
| Pattern C (Ignore) | ✅ Excellent | ~98% | None |
|
||||
|
||||
### Overall Assessment
|
||||
|
||||
The codebase demonstrates strong adherence to error handling patterns with a few specific areas needing clarification:
|
||||
|
||||
1. **Strengths:**
|
||||
- User input validation is consistently fatal
|
||||
- Core database operations correctly use Pattern A
|
||||
- Cleanup operations properly use Pattern C
|
||||
- Error messages are generally clear and actionable
|
||||
|
||||
2. **Improvement Areas:**
|
||||
- Metadata operations need clearer distinction between configuration and tracking
|
||||
- Some warnings could benefit from actionable hints
|
||||
- Helper functions could reduce boilerplate
|
||||
|
||||
3. **Risk Level:** 🟡 Low-Medium
|
||||
- No critical issues that would cause incorrect behavior
|
||||
- Inconsistencies are mostly about consistency, not correctness
|
||||
- System is already resilient with graceful degradation
|
||||
|
||||
---
|
||||
|
||||
## Phase 2 Audit: Additional cmd/bd Files (bd-3gc)
|
||||
|
||||
**Date:** 2025-11-28
|
||||
**Files Audited:** daemon_sync.go, list.go, show.go, dep.go, label.go, comments.go, delete.go, compact.go, config.go, validate.go
|
||||
**Notes:** update.go, close.go, reopen.go do not exist as separate files - functionality is in show.go
|
||||
|
||||
---
|
||||
|
||||
### daemon_sync.go ✅ MOSTLY CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Used for critical failures but returns early to channel instead of os.Exit
|
||||
```go
|
||||
// daemon_sync.go - Returns error to channel, caller decides
|
||||
if err != nil {
|
||||
log.log("daemon sync error: %v", err)
|
||||
return // Logs and returns, daemon continues
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern B (Warn):** Uses internal logging (log.log) which is appropriate for daemon background operations
|
||||
```go
|
||||
// Non-fatal warnings logged to internal log
|
||||
log.log("warning: failed to update metadata: %v", err)
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Appropriate - Daemon operations use internal logging since there's no interactive stderr. Background process errors are logged for debugging but don't crash the daemon.
|
||||
|
||||
---
|
||||
|
||||
### list.go ✅ CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Correctly applied for database errors and ID resolution
|
||||
```go
|
||||
// list.go:~200-210
|
||||
issues, err := store.SearchIssues(ctx, "", filter)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern B (Warn):** Used for non-critical label lookup failures in batch operations
|
||||
```go
|
||||
// Individual label fetch failures warn but continue
|
||||
for _, issue := range issues {
|
||||
labels, err := store.GetLabels(ctx, issue.ID)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to get labels for %s: %v\n", issue.ID, err)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Exit for critical failures, warn for auxiliary data fetch failures.
|
||||
|
||||
---
|
||||
|
||||
### show.go (includes update, close functionality) ✅ CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Correctly applied for ID resolution and issue retrieval
|
||||
```go
|
||||
// show.go - ID resolution
|
||||
fullID, err := utils.ResolvePartialID(ctx, store, args[0])
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error resolving %s: %v\n", args[0], err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern A (Exit):** Core update operations
|
||||
```go
|
||||
// show.go - UpdateIssue
|
||||
if err := store.UpdateIssue(ctx, fullID, updates, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - All critical operations use Pattern A correctly.
|
||||
|
||||
---
|
||||
|
||||
### dep.go ✅ CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Correctly applied for ID resolution and dependency operations
|
||||
```go
|
||||
// dep.go:37-44 - ID resolution
|
||||
resp, err := daemonClient.ResolveID(resolveArgs)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error resolving issue ID %s: %v\n", args[0], err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern B (Warn):** Used for cycle detection after successful dependency add
|
||||
```go
|
||||
// dep.go:111-133 - Cycle warning is non-fatal
|
||||
cycles, err := store.DetectCycles(ctx)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: Failed to check for cycles: %v\n", err)
|
||||
} else if len(cycles) > 0 {
|
||||
yellow := color.New(color.FgYellow).SprintFunc()
|
||||
fmt.Fprintf(os.Stderr, "\n%s Warning: Dependency cycle detected!\n", yellow("⚠"))
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Exit for core operations, warn for advisory checks (cycle detection).
|
||||
|
||||
---
|
||||
|
||||
### label.go ✅ CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Used for ID resolution in singleton operations
|
||||
```go
|
||||
// label.go:167-182 - labelListCmd
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error resolving issue ID %s: %v\n", args[0], err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern B (Warn):** Used for batch operations - continues on individual failures
|
||||
```go
|
||||
// label.go:32-35 - processBatchLabelOperation
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error %s label %s %s: %v\n", operation, operation, issueID, err)
|
||||
continue // Continue processing other issues
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Batch operations use continue pattern appropriately.
|
||||
|
||||
---
|
||||
|
||||
### comments.go ✅ CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Correctly applied for all comment operations
|
||||
```go
|
||||
// comments.go:45-50
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error getting comments: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern A with fallback:** Interesting pattern for daemon compatibility
|
||||
```go
|
||||
// comments.go:42-50 - Fallback to direct mode
|
||||
if isUnknownOperationError(err) {
|
||||
if err := fallbackToDirectMode("daemon does not support comment_list RPC"); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error getting comments: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
} else {
|
||||
fmt.Fprintf(os.Stderr, "Error getting comments: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Uses Pattern A but with smart fallback for daemon compatibility.
|
||||
|
||||
---
|
||||
|
||||
### delete.go ✅ MOSTLY CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Core deletion operations
|
||||
```go
|
||||
// delete.go:91-98
|
||||
issue, err := store.GetIssue(ctx, issueID)
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
if issue == nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: issue %s not found\n", issueID)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern B (Warn):** Used for auxiliary cleanup operations
|
||||
```go
|
||||
// delete.go:202-206 - Reference update warning
|
||||
if err := store.UpdateIssue(ctx, id, updates, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: Failed to update references in %s: %v\n", id, err)
|
||||
} else {
|
||||
updatedIssueCount++
|
||||
}
|
||||
|
||||
// delete.go:212-217 - Dependency removal warning
|
||||
if err := store.RemoveDependency(ctx, dep.IssueID, dep.DependsOnID, actor); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: Failed to remove dependency %s → %s: %v\n",
|
||||
dep.IssueID, dep.DependsOnID, err)
|
||||
}
|
||||
|
||||
// delete.go:235-237 - JSONL cleanup warning
|
||||
if err := removeIssueFromJSONL(issueID); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: Failed to remove from JSONL: %v\n", err)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Core deletion is fatal, cleanup operations are non-fatal warnings.
|
||||
|
||||
---
|
||||
|
||||
### compact.go ✅ CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Validation and core operations
|
||||
```go
|
||||
// compact.go:107-114 - Mode validation
|
||||
if activeModes == 0 {
|
||||
fmt.Fprintf(os.Stderr, "Error: must specify one mode: --analyze, --apply, or --auto\n")
|
||||
os.Exit(1)
|
||||
}
|
||||
if activeModes > 1 {
|
||||
fmt.Fprintf(os.Stderr, "Error: cannot use multiple modes together...\n")
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// compact.go:220-222 - Eligibility check
|
||||
if !eligible {
|
||||
fmt.Fprintf(os.Stderr, "Error: %s is not eligible for Tier %d compaction: %s\n", issueID, compactTier, reason)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern B (Warn):** Used for non-critical config loading and pruning
|
||||
```go
|
||||
// compact.go:916-919 - Config load warning
|
||||
cfg, err := configfile.Load(beadsDir)
|
||||
if err != nil {
|
||||
if !jsonOutput {
|
||||
fmt.Fprintf(os.Stderr, "Warning: could not load config for retention settings: %v\n", err)
|
||||
}
|
||||
}
|
||||
|
||||
// compact.go:929-932 - Pruning warning
|
||||
result, err := deletions.PruneDeletions(deletionsPath, retentionDays)
|
||||
if err != nil {
|
||||
if !jsonOutput {
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to prune deletions: %v\n", err)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Validation is fatal, housekeeping is non-fatal.
|
||||
|
||||
---
|
||||
|
||||
### config.go ✅ CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** All config operations are fatal since they require direct database access
|
||||
```go
|
||||
// config.go:40-43
|
||||
if err := ensureDirectMode("config set requires direct database access"); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// config.go:53-55
|
||||
if err := syncbranch.Set(ctx, store, value); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error setting config: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Config operations are intentionally all-or-nothing.
|
||||
|
||||
---
|
||||
|
||||
### validate.go ✅ MOSTLY CONSISTENT
|
||||
|
||||
**Pattern A (Exit):** Core validation failures
|
||||
```go
|
||||
// validate.go:28-32 - Daemon mode not supported
|
||||
if daemonClient != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: validate command not yet supported in daemon mode\n")
|
||||
fmt.Fprintf(os.Stderr, "Use: bd --no-daemon validate\n")
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// validate.go:64-68 - Issue fetch failure
|
||||
allIssues, err = store.SearchIssues(ctx, "", types.IssueFilter{})
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error fetching issues: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Pattern B (Warn):** Results contain errors but command completes with summary
|
||||
```go
|
||||
// validate.go:152-161 - hasFailures check determines exit code
|
||||
func (r *validationResults) hasFailures() bool {
|
||||
for _, result := range r.checks {
|
||||
if result.err != nil {
|
||||
return true
|
||||
}
|
||||
if result.issueCount > 0 && result.fixedCount < result.issueCount {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** ✅ Consistent - Validation command properly aggregates results and exits 1 only if issues found.
|
||||
|
||||
---
|
||||
|
||||
## Phase 2 Summary
|
||||
|
||||
### Additional Pattern Observations
|
||||
|
||||
1. **Batch Operations Pattern:** Multiple commands (label, delete) use a `continue` pattern for batch operations that correctly implements Pattern B semantics:
|
||||
```go
|
||||
for _, item := range items {
|
||||
if err := processItem(item); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Warning: %v\n", err)
|
||||
continue // Process remaining items
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
2. **Daemon Fallback Pattern:** Commands like comments.go implement a sophisticated fallback:
|
||||
- Try daemon RPC first
|
||||
- If daemon doesn't support operation, fall back to direct mode
|
||||
- Only exit on failure after all options exhausted
|
||||
|
||||
3. **Exit Code Propagation:** validate.go demonstrates proper exit code handling - aggregates results and returns appropriate exit code at the end.
|
||||
|
||||
### Updated Pattern Compliance Scorecard
|
||||
|
||||
| Pattern | Status | Compliance Rate | Notes |
|
||||
|---------|--------|-----------------|-------|
|
||||
| Pattern A (Exit) | ✅ Excellent | ~97% | Consistent across all audited files |
|
||||
| Pattern B (Warn) | ✅ Excellent | ~95% | Good use of continue pattern for batches |
|
||||
| Pattern C (Ignore) | ✅ Excellent | ~98% | Cleanup operations properly silent |
|
||||
|
||||
### Files Still Needing Audit
|
||||
|
||||
- doctor/* (doctor package files)
|
||||
- daemon.go
|
||||
- stats.go
|
||||
- duplicates.go
|
||||
- repair_deps.go
|
||||
- rename.go
|
||||
- merge.go
|
||||
- epic.go
|
||||
- ready.go
|
||||
- blocked.go
|
||||
- And ~30 more command files
|
||||
|
||||
---
|
||||
|
||||
**Audit Completed By:** Claude (bd-1qwo, bd-3gc)
|
||||
**Next Review:** After implementing recommendations and auditing remaining files
|
||||
147
docs/dev-notes/MAIN_TEST_CLEANUP_PLAN.md
Normal file
147
docs/dev-notes/MAIN_TEST_CLEANUP_PLAN.md
Normal file
@@ -0,0 +1,147 @@
|
||||
# main_test.go Cleanup Plan
|
||||
|
||||
## Problem
|
||||
main_test.go has 18 tests using deprecated global state (isDirty, flushTimer, flushMutex).
|
||||
These tests are slow (14 newTestStore() calls) and redundant with flush_manager_test.go.
|
||||
|
||||
## Root Cause
|
||||
- FlushManager refactoring (bd-52) moved flush logic to isolated FlushManager
|
||||
- Legacy path kept "for backward compatibility with tests"
|
||||
- main_test.go still tests the DEPRECATED legacy path
|
||||
- flush_manager_test.go tests the NEW FlushManager path
|
||||
|
||||
## Solution: Three-Phase Cleanup
|
||||
|
||||
### Phase 1: Remove Redundant Tests (THIS SESSION)
|
||||
|
||||
#### Tests to DELETE (covered by flush_manager_test.go):
|
||||
|
||||
1. **TestAutoFlushDirtyMarking** (line 22)
|
||||
- Tests that isDirty flag gets set
|
||||
- COVERED BY: TestFlushManagerMarkDirtyTriggersFlush
|
||||
- Uses: global isDirty, flushTimer
|
||||
|
||||
2. **TestAutoFlushDisabled** (line 59)
|
||||
- Tests that --no-auto-flush disables flushing
|
||||
- COVERED BY: TestFlushManagerDisabledDoesNotFlush
|
||||
- Uses: global autoFlushEnabled
|
||||
|
||||
3. **TestAutoFlushDebounce** (line 90)
|
||||
- ALREADY SKIPPED with note: "obsolete - tested in flush_manager_test.go"
|
||||
- DELETE entirely
|
||||
|
||||
4. **TestAutoFlushClearState** (line 184)
|
||||
- Tests clearAutoFlushState() resets flags
|
||||
- clearAutoFlushState() is legacy-only (no FlushManager equivalent yet)
|
||||
- Will be replaced when we add FlushManager.MarkClean()
|
||||
- DELETE (clearAutoFlushState tested implicitly in export/sync commands)
|
||||
|
||||
5. **TestAutoFlushConcurrency** (line 355)
|
||||
- Tests concurrent markDirtyAndScheduleFlush() calls
|
||||
- COVERED BY: TestFlushManagerConcurrentMarkDirty
|
||||
- Uses: global isDirty, flushTimer
|
||||
|
||||
6. **TestAutoFlushStoreInactive** (line 403)
|
||||
- Tests flush behavior when store is closed
|
||||
- COVERED BY: TestPerformFlushStoreInactive
|
||||
- Uses: global storeActive
|
||||
|
||||
7. **TestAutoFlushErrorHandling** (line 582)
|
||||
- Tests error scenarios (JSONL as directory)
|
||||
- COVERED BY: TestPerformFlushErrorHandling
|
||||
- Uses: newTestStore(), global state
|
||||
|
||||
#### Tests to KEEP (unique integration value):
|
||||
|
||||
1. **TestAutoFlushOnExit** (line 219)
|
||||
- Tests PersistentPostRun() calls flushManager.Shutdown()
|
||||
- Integration test: CLI lifecycle → flush behavior
|
||||
- NOT covered by flush_manager_test.go
|
||||
- **REFACTOR** to use FlushManager directly (not global state)
|
||||
|
||||
2. **TestAutoFlushJSONLContent** (line 446)
|
||||
- Tests actual JSONL file content after flush
|
||||
- Integration test: DB mutations → JSONL file output
|
||||
- NOT covered by flush_manager_test.go
|
||||
- **REFACTOR** to set up FlushManager properly
|
||||
|
||||
3. **Auto-import tests** (9 tests, lines 701-1412)
|
||||
- Test DB ↔ JSONL synchronization
|
||||
- Test merge conflict detection
|
||||
- Test status transition invariants
|
||||
- Integration tests with filesystem/git operations
|
||||
- **DEFER** to separate cleanup (different subsystem)
|
||||
|
||||
### Phase 2: Remove Legacy Path
|
||||
|
||||
After deleting redundant tests:
|
||||
|
||||
1. Add `MarkClean()` method to FlushManager
|
||||
2. Update `clearAutoFlushState()` to use `flushManager.MarkClean()`
|
||||
3. Remove legacy path from `markDirtyAndScheduleFlush()`
|
||||
4. Remove legacy path from `markDirtyAndScheduleFullExport()`
|
||||
|
||||
### Phase 3: Remove Global State
|
||||
|
||||
After removing legacy path:
|
||||
|
||||
1. Remove global variables:
|
||||
- `isDirty` (line 72 in main.go)
|
||||
- `flushTimer` (line 75 in main.go)
|
||||
- `flushMutex` (line 74 in main.go)
|
||||
|
||||
2. Update test cleanup code:
|
||||
- cli_fast_test.go: Remove isDirty/flushTimer reset
|
||||
- direct_mode_test.go: Remove isDirty/flushTimer save/restore
|
||||
|
||||
## Expected Impact
|
||||
|
||||
### Before:
|
||||
- 18 tests in main_test.go
|
||||
- 14 newTestStore() calls
|
||||
- ~15-20 seconds runtime (estimated)
|
||||
- Testing deprecated code path
|
||||
|
||||
### After Phase 1:
|
||||
- 11 tests in main_test.go (7 deleted)
|
||||
- ~6-8 newTestStore() calls (auto-import tests)
|
||||
- ~5-7 seconds runtime (estimated)
|
||||
- Testing only integration behavior
|
||||
|
||||
### After Phase 2:
|
||||
- Same test count
|
||||
- Cleaner code (no legacy path)
|
||||
- Tests use FlushManager directly
|
||||
|
||||
### After Phase 3:
|
||||
- Same test count
|
||||
- No global state pollution
|
||||
- Tests can run in parallel (t.Parallel())
|
||||
- ~2-3 seconds runtime (estimated)
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
1. Add t.Skip() to 7 redundant tests ✓
|
||||
2. Run tests to verify nothing breaks ✓
|
||||
3. Delete skipped tests ✓
|
||||
4. Refactor 2 keeper tests to use FlushManager
|
||||
5. Add FlushManager.MarkClean() method
|
||||
6. Remove legacy paths
|
||||
7. Remove global variables
|
||||
8. Run full test suite
|
||||
|
||||
## Files Modified
|
||||
|
||||
- `cmd/bd/main_test.go` - Delete 7 tests, refactor 2 tests
|
||||
- `cmd/bd/flush_manager.go` - Add MarkClean() method
|
||||
- `cmd/bd/autoflush.go` - Remove legacy paths
|
||||
- `cmd/bd/main.go` - Remove global variables (Phase 3)
|
||||
- `docs/MAIN_TEST_REFACTOR_NOTES.md` - Update with new approach
|
||||
|
||||
## References
|
||||
|
||||
- Original analysis: `docs/MAIN_TEST_REFACTOR_NOTES.md`
|
||||
- FlushManager implementation: `cmd/bd/flush_manager.go`
|
||||
- FlushManager tests: `cmd/bd/flush_manager_test.go`
|
||||
- Issue bd-52: FlushManager refactoring
|
||||
- Issue bd-159: Test config reference
|
||||
193
docs/dev-notes/MAIN_TEST_REFACTOR_NOTES.md
Normal file
193
docs/dev-notes/MAIN_TEST_REFACTOR_NOTES.md
Normal file
@@ -0,0 +1,193 @@
|
||||
# main_test.go Refactoring Notes (bd-1rh follow-up)
|
||||
|
||||
## Status: RESOLVED - Redundant Tests Deleted ✅
|
||||
|
||||
### Summary
|
||||
Attempted to refactor `main_test.go` (18 tests, 14 `newTestStore()` calls) to use shared DB pattern like P1 files. **Discovered fundamental incompatibility** with shared DB approach due to global state manipulation and integration test characteristics.
|
||||
|
||||
### What We Tried
|
||||
1. Created `TestAutoFlushSuite` and `TestAutoImportSuite` with shared DB
|
||||
2. Converted 18 individual tests to subtests
|
||||
3. Reduced from 14 DB setups to 2
|
||||
|
||||
### Problems Encountered
|
||||
|
||||
#### 1. **Deadlock Issue**
|
||||
- Tests call `flushToJSONL()` which accesses the database
|
||||
- Test cleanup (from `newTestStore()`) tries to close the database
|
||||
- Results in database lock contention and test timeouts
|
||||
- Stack trace shows: `database/sql.(*DB).Close()` waiting while `flushToJSONL()` is accessing DB
|
||||
|
||||
#### 2. **Global State Manipulation**
|
||||
These tests heavily manipulate package-level globals:
|
||||
- `autoFlushEnabled`, `isDirty`, `flushTimer`
|
||||
- `store`, `storeActive`, `storeMutex`
|
||||
- `dbPath` (used to compute JSONL path dynamically)
|
||||
- `flushFailureCount`, `lastFlushError`
|
||||
|
||||
#### 3. **Integration Test Characteristics**
|
||||
- Tests simulate end-to-end flush/import workflows
|
||||
- Tests capture stderr to verify error messages
|
||||
- Tests manipulate filesystem state directly
|
||||
- Tests create directories to force error conditions
|
||||
|
||||
### Key Differences from P1 Tests
|
||||
|
||||
| Aspect | P1 Tests (create, dep, etc.) | main_test.go |
|
||||
|--------|------------------------------|--------------|
|
||||
| **DB Usage** | Pure DB operations | Global state + DB + filesystem |
|
||||
| **Isolation** | Data-level only | Requires process-level isolation |
|
||||
| **Cleanup** | Simple | Complex (timers, goroutines, mutexes) |
|
||||
| **Pattern** | CRUD operations | Workflow simulation |
|
||||
|
||||
### Why Shared DB Doesn't Work
|
||||
|
||||
1. **`jsonlPath` is computed dynamically** from `dbPath` via `findJSONLPath()`
|
||||
- Not a global variable like in old tests
|
||||
- Changes to `dbPath` affect where JSONL files are written/read
|
||||
|
||||
2. **Tests need to control exact JSONL paths** for:
|
||||
- Creating files to force errors (making JSONL a directory)
|
||||
- Verifying files were/weren't created
|
||||
- Touching files to simulate git pull scenarios
|
||||
|
||||
3. **Concurrent access issues**:
|
||||
- Background flush operations may trigger during test cleanup
|
||||
- Global mutexes protect state but cause deadlocks with shared DB
|
||||
|
||||
### What Tests Actually Do
|
||||
|
||||
#### Auto-Flush Tests (9 tests)
|
||||
- Test global state flags (`isDirty`, `autoFlushEnabled`)
|
||||
- Test timer management (`flushTimer`)
|
||||
- Test concurrency (goroutines calling `markDirtyAndScheduleFlush()`)
|
||||
- Simulate program exit (PersistentPostRun behavior)
|
||||
- Force error conditions by making JSONL path a directory
|
||||
|
||||
#### Auto-Import Tests (9 tests)
|
||||
- Test JSONL -> DB sync when JSONL is newer
|
||||
- Test merge conflict detection (literal `<<<<<<<` markers in file)
|
||||
- Test JSON-encoded conflict markers (false positive prevention)
|
||||
- Test status transition invariants (closed_at management)
|
||||
- Manipulate file timestamps with `os.Chtimes()`
|
||||
|
||||
## Recommended Approach
|
||||
|
||||
### Option 1: Leave As-Is (RECOMMENDED)
|
||||
**Rationale**: These are integration tests, not unit tests. The overhead of 14 DB setups is acceptable for:
|
||||
- Tests that manipulate global state
|
||||
- Tests that simulate complex workflows
|
||||
- Tests that are relatively fast already (~0.5s each)
|
||||
|
||||
**Expected speedup**: Minimal (2-3x at most) vs. complexity cost
|
||||
|
||||
### Option 2: Refactor Without Shared DB
|
||||
**Changes**:
|
||||
1. Keep individual test functions (not suite)
|
||||
2. Reduce DB setups by **reusing test stores within related test groups**
|
||||
3. Add helpers to reset global state between tests
|
||||
4. Document which tests can share vs. need isolation
|
||||
|
||||
**Example**:
|
||||
```go
|
||||
func TestAutoFlushGroup(t *testing.T) {
|
||||
tmpDir := t.TempDir()
|
||||
testDB := filepath.Join(tmpDir, "test.db")
|
||||
testStore := newTestStore(t, testDB)
|
||||
|
||||
// Helper to reset state
|
||||
resetState := func() {
|
||||
autoFlushEnabled = true
|
||||
isDirty = false
|
||||
if flushTimer != nil {
|
||||
flushTimer.Stop()
|
||||
flushTimer = nil
|
||||
}
|
||||
}
|
||||
|
||||
t.Run("DirtyMarking", func(t *testing.T) {
|
||||
resetState()
|
||||
// test...
|
||||
})
|
||||
|
||||
t.Run("Disabled", func(t *testing.T) {
|
||||
resetState()
|
||||
// test...
|
||||
})
|
||||
}
|
||||
```
|
||||
|
||||
### Option 3: Mock/Stub Approach
|
||||
**Changes**:
|
||||
1. Introduce interfaces for `flushToJSONL` and `autoImportIfNewer`
|
||||
2. Mock the filesystem operations
|
||||
3. Test state transitions without actual DB/filesystem
|
||||
|
||||
**Trade-offs**: More refactoring, loses integration test value
|
||||
|
||||
## Files Modified (Reverted)
|
||||
- `cmd/bd/main_test.go` - Reverted to original
|
||||
- `cmd/bd/duplicates_test.go` - Fixed unused import (kept fix)
|
||||
|
||||
## Lessons Learned
|
||||
|
||||
1. **Not all tests benefit from shared DB pattern**
|
||||
- Integration tests need isolation
|
||||
- Global state manipulation requires careful handling
|
||||
|
||||
2. **P1 test pattern assumes**:
|
||||
- Pure DB operations
|
||||
- No global state
|
||||
- Data-level isolation sufficient
|
||||
|
||||
3. **Test classification matters**:
|
||||
- Unit tests: Share DB ✓
|
||||
- Integration tests: Need isolation ✓
|
||||
- Workflow tests: Need full process isolation ✓
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. **Document in TEST_SUITE_AUDIT.md** that main_test.go is P2 but **NOT a good candidate** for shared DB pattern
|
||||
2. **Update audit classification**: Move main_test.go to "Special Cases" category
|
||||
3. **Focus P2 efforts** on `integrity_test.go` and `export_import_test.go` instead
|
||||
|
||||
## 2025-11-21 Update: Solution Implemented ✅
|
||||
|
||||
### What We Did
|
||||
Rather than forcing shared DB pattern on integration tests, we **deleted redundant tests** that were duplicating coverage from `flush_manager_test.go`.
|
||||
|
||||
### Key Insight
|
||||
After FlushManager refactoring (bd-52), `main_test.go` was testing the DEPRECATED legacy path while `flush_manager_test.go` tested the NEW FlushManager. Solution: delete the redundant legacy tests.
|
||||
|
||||
### Changes Made
|
||||
1. **Deleted 7 redundant tests** (407 lines):
|
||||
- TestAutoFlushDirtyMarking (→ TestFlushManagerMarkDirtyTriggersFlush)
|
||||
- TestAutoFlushDisabled (→ TestFlushManagerDisabledDoesNotFlush)
|
||||
- TestAutoFlushDebounce (already skipped, obsolete)
|
||||
- TestAutoFlushClearState (clearAutoFlushState tested in export/sync)
|
||||
- TestAutoFlushConcurrency (→ TestFlushManagerConcurrentMarkDirty)
|
||||
- TestAutoFlushStoreInactive (→ TestPerformFlushStoreInactive)
|
||||
- TestAutoFlushErrorHandling (→ TestPerformFlushErrorHandling)
|
||||
|
||||
2. **Kept 2 integration tests**:
|
||||
- TestAutoFlushOnExit (PersistentPostRun behavior)
|
||||
- TestAutoFlushJSONLContent (DB → JSONL file content)
|
||||
|
||||
3. **Updated clearAutoFlushState()** to no-op when FlushManager exists
|
||||
|
||||
### Results
|
||||
- **Before**: 18 tests, 1079 lines, ~15-20s
|
||||
- **After**: 11 tests, 672 lines, ~5-7s (estimated)
|
||||
- **Speedup**: ~3x faster
|
||||
- **All tests passing**: ✅
|
||||
|
||||
### Future Work (Optional)
|
||||
- Phase 2: Remove legacy path from `markDirtyAndScheduleFlush()` entirely
|
||||
- Phase 3: Remove global variables (isDirty, flushTimer, flushMutex)
|
||||
- These are deferred as they provide diminishing returns vs. complexity
|
||||
|
||||
## References
|
||||
- Original issue: bd-1rh (Phase 2 test suite optimization)
|
||||
- Pattern source: `label_test.go`, P1 refactored files
|
||||
- Related: bd-159 (test config issues), bd-270 (merge conflict detection)
|
||||
- Solution documented: `docs/MAIN_TEST_CLEANUP_PLAN.md`
|
||||
289
docs/dev-notes/TEST_SUITE_AUDIT.md
Normal file
289
docs/dev-notes/TEST_SUITE_AUDIT.md
Normal file
@@ -0,0 +1,289 @@
|
||||
# cmd/bd Test Suite Audit (bd-c49)
|
||||
|
||||
## Executive Summary
|
||||
|
||||
**Original State**: 280 tests across 76 test files, each creating isolated database setups
|
||||
**Phase 1 Complete**: 6 P1 test files refactored with shared DB setup (bd-1rh)
|
||||
**Achieved Speedup**: P1 tests now run in 0.43 seconds (vs. estimated 10+ minutes before)
|
||||
**Remaining Work**: P2 and P3 files still use isolated DB setups
|
||||
|
||||
## Analysis Categories
|
||||
|
||||
### Category 1: Pure DB Tests (Can Share DB Setup) - 150+ tests
|
||||
|
||||
These tests only interact with the database and can safely share a single DB setup per suite:
|
||||
|
||||
#### High Priority Candidates (P1 - Start Here):
|
||||
|
||||
1. ✓ **create_test.go** (11 tests) → `TestCreateSuite` **DONE (bd-y6d)**
|
||||
- All tests: `TestCreate_*`
|
||||
- Before: 11 separate `newTestStore()` calls
|
||||
- After: 1 shared DB, 11 subtests
|
||||
- Result: **10x faster**
|
||||
|
||||
2. **label_test.go** (1 suite with 11 subtests) → **Already optimal!**
|
||||
- Uses helper pattern with single DB setup
|
||||
- This is the TEMPLATE for refactoring!
|
||||
|
||||
3. ✓ **dep_test.go** (9 tests) → `TestDependencySuite` **DONE (bd-1rh)**
|
||||
- All tests: `TestDep_*`
|
||||
- Before: 4 `newTestStore()` calls
|
||||
- After: 1 shared DB, 4 subtests (+ command init tests)
|
||||
- Result: **4x faster**
|
||||
|
||||
4. ✓ **list_test.go** (3 tests) → `TestListCommandSuite` + `TestListQueryCapabilitiesSuite` **DONE (bd-1rh)**
|
||||
- Before: 2 `newTestStore()` calls
|
||||
- After: 2 shared DBs (split to avoid data pollution), multiple subtests
|
||||
- Result: **2x faster**
|
||||
|
||||
5. ✓ **comments_test.go** (3 tests) → `TestCommentsSuite` **DONE (bd-1rh)**
|
||||
- Before: 2 `newTestStore()` calls
|
||||
- After: 1 shared DB, 2 subtest groups with 6 total subtests
|
||||
- Result: **2x faster**
|
||||
|
||||
6. ✓ **stale_test.go** (6 tests) → Individual test functions **DONE (bd-1rh)**
|
||||
- Before: 5 `newTestStore()` calls
|
||||
- After: 6 individual test functions (shared DB caused data pollution)
|
||||
- Result: **Slight improvement** (data isolation was necessary)
|
||||
|
||||
7. ✓ **ready_test.go** (4 tests) → `TestReadySuite` **DONE (bd-1rh)**
|
||||
- Before: 3 `newTestStore()` calls
|
||||
- After: 1 shared DB, 3 subtests
|
||||
- Result: **3x faster**
|
||||
|
||||
8. **reopen_test.go** (1 test) → Leave as-is or merge
|
||||
- Single test, minimal benefit from refactoring
|
||||
|
||||
#### Medium Priority Candidates (P2):
|
||||
|
||||
9. **main_test.go** (18 tests) → `TestMainSuite`
|
||||
- Current: 14 `newTestStore()` calls
|
||||
- Proposed: 1-2 shared DBs (may need isolation for some)
|
||||
- Expected speedup: **5-7x faster**
|
||||
|
||||
10. **integrity_test.go** (6 tests) → `TestIntegritySuite`
|
||||
- Current: 15 `newTestStore()` calls (many helper calls)
|
||||
- Proposed: 1 shared DB, 6 subtests
|
||||
- Expected speedup: **10x faster**
|
||||
|
||||
11. **export_import_test.go** (4 tests) → `TestExportImportSuite`
|
||||
- Current: 4 `newTestStore()` calls
|
||||
- Proposed: 1 shared DB, 4 subtests
|
||||
- Expected speedup: **4x faster**
|
||||
|
||||
### Category 2: Tests Needing Selective Isolation (60+ tests)
|
||||
|
||||
These have a mix - some can share DB, some need isolation:
|
||||
|
||||
#### Daemon Tests (Already have integration tags):
|
||||
- **daemon_test.go** (15 tests) - Mix of DB and daemon lifecycle
|
||||
- Propose: Separate suites for DB-only vs daemon lifecycle tests
|
||||
|
||||
- **daemon_autoimport_test.go** (2 tests)
|
||||
- **daemon_crash_test.go** (2 tests)
|
||||
- **daemon_lock_test.go** (6 tests)
|
||||
- **daemon_parent_test.go** (1 test)
|
||||
- **daemon_sync_test.go** (6 tests)
|
||||
- **daemon_sync_branch_test.go** (11 tests)
|
||||
- **daemon_watcher_test.go** (7 tests)
|
||||
|
||||
**Recommendation**: Keep daemon tests isolated (they already have `//go:build integration` tags)
|
||||
|
||||
#### Git Operation Tests:
|
||||
- **git_sync_test.go** (1 test)
|
||||
- **sync_test.go** (16 tests)
|
||||
- **sync_local_only_test.go** (2 tests)
|
||||
- **import_uncommitted_test.go** (2 tests)
|
||||
|
||||
**Recommendation**: Keep git tests isolated (need real git repos)
|
||||
|
||||
### Category 3: Already Well-Optimized (20+ tests)
|
||||
|
||||
Tests that already use good patterns:
|
||||
|
||||
1. **label_test.go** - Uses helper struct with shared DB ✓
|
||||
2. **delete_test.go** - Has `//go:build integration` tag ✓
|
||||
3. All daemon tests - Have `//go:build integration` tags ✓
|
||||
|
||||
### Category 4: Special Cases (50+ tests)
|
||||
|
||||
#### CLI Integration Tests:
|
||||
- **cli_fast_test.go** (17 tests) - End-to-end CLI testing
|
||||
- Keep isolated, already tagged `//go:build integration`
|
||||
|
||||
#### Import/Export Tests:
|
||||
- **import_bug_test.go** (1 test)
|
||||
- **import_cancellation_test.go** (2 tests)
|
||||
- **import_idempotent_test.go** (3 tests)
|
||||
- **import_multipart_id_test.go** (2 tests)
|
||||
- **export_mtime_test.go** (3 tests)
|
||||
- **export_test.go** (1 test)
|
||||
|
||||
**Recommendation**: Most can share DB within their suite
|
||||
|
||||
#### Filesystem/Init Tests:
|
||||
- **init_test.go** (8 tests)
|
||||
- **init_hooks_test.go** (3 tests)
|
||||
- **reinit_test.go** (1 test)
|
||||
- **onboard_test.go** (1 test)
|
||||
|
||||
**Recommendation**: Need isolation (modify filesystem)
|
||||
|
||||
#### Validation/Utility Tests:
|
||||
- **validate_test.go** (9 tests)
|
||||
- **template_test.go** (5 tests)
|
||||
- **template_security_test.go** (2 tests)
|
||||
- **markdown_test.go** (2 tests)
|
||||
- **output_test.go** (2 tests)
|
||||
- **version_test.go** (2 tests)
|
||||
- **config_test.go** (2 tests)
|
||||
|
||||
**Recommendation**: Can share DB or may not need DB at all
|
||||
|
||||
#### Migration Tests:
|
||||
- **migrate_test.go** (3 tests)
|
||||
- **migrate_hash_ids_test.go** (4 tests)
|
||||
- **repair_deps_test.go** (4 tests)
|
||||
|
||||
**Recommendation**: Need isolation (modify DB schema)
|
||||
|
||||
#### Doctor Tests:
|
||||
- **doctor_test.go** (13 tests)
|
||||
- **doctor/legacy_test.go** tests
|
||||
|
||||
**Recommendation**: Mix - some can share, some need isolation
|
||||
|
||||
#### Misc Tests:
|
||||
- ✅ **compact_test.go** (10 tests → 1 suite + 4 standalone = Phase 2 DONE)
|
||||
- **duplicates_test.go** (5 tests)
|
||||
- **epic_test.go** (3 tests)
|
||||
- **hooks_test.go** (6 tests)
|
||||
- **info_test.go** (5 tests)
|
||||
- **nodb_test.go** (6 tests)
|
||||
- **restore_test.go** (6 tests)
|
||||
- **worktree_test.go** (2 tests)
|
||||
- **scripttest_test.go** (1 test)
|
||||
- **direct_mode_test.go** (1 test)
|
||||
- **autostart_test.go** (3 tests)
|
||||
- **autoimport_test.go** (9 tests)
|
||||
- **deletion_tracking_test.go** (12 tests)
|
||||
- **rename_prefix_test.go** (3 tests)
|
||||
- **rename_prefix_repair_test.go** (1 test)
|
||||
- **status_test.go** (3 tests)
|
||||
- **sync_merge_test.go** (4 tests)
|
||||
- **jsonl_integrity_test.go** (2 tests)
|
||||
- **export_staleness_test.go** (5 tests)
|
||||
- **export_integrity_integration_test.go** (3 tests)
|
||||
- **flush_manager_test.go** (12 tests)
|
||||
- **daemon_debouncer_test.go** (8 tests)
|
||||
- **daemon_rotation_test.go** (4 tests)
|
||||
- **daemons_test.go** (2 tests)
|
||||
- **daemon_watcher_platform_test.go** (3 tests)
|
||||
- **helpers_test.go** (4 tests)
|
||||
|
||||
## Proposed Refactoring Plan
|
||||
|
||||
### Phase 1: High Priority (P1) - Quick Wins ✓ COMPLETE
|
||||
All P1 files refactored for immediate speedup:
|
||||
|
||||
1. ✓ **create_test.go** (bd-y6d) - Template refactor → `TestCreateSuite`
|
||||
2. ✓ **dep_test.go** - Dependency tests → `TestDependencySuite`
|
||||
3. ✓ **stale_test.go** - Stale issue tests → Individual test functions (data isolation required)
|
||||
4. ✓ **comments_test.go** - Comment tests → `TestCommentsSuite`
|
||||
5. ✓ **list_test.go** - List/search tests → `TestListCommandSuite` + `TestListQueryCapabilitiesSuite`
|
||||
6. ✓ **ready_test.go** - Ready state tests → `TestReadySuite`
|
||||
|
||||
**Results**: All P1 tests now run in **0.43 seconds** (vs. estimated 10+ minutes before)
|
||||
|
||||
**Pattern to follow**: Use `label_test.go` as the template!
|
||||
|
||||
```go
|
||||
func TestCreateSuite(t *testing.T) {
|
||||
tmpDir := t.TempDir()
|
||||
testDB := filepath.Join(tmpDir, ".beads", "beads.db")
|
||||
s := newTestStore(t, testDB)
|
||||
ctx := context.Background()
|
||||
|
||||
t.Run("BasicIssue", func(t *testing.T) { /* test */ })
|
||||
t.Run("WithDescription", func(t *testing.T) { /* test */ })
|
||||
// ... etc
|
||||
}
|
||||
```
|
||||
|
||||
### Phase 2: Medium Priority (P2) - Moderate Gains
|
||||
After Phase 1 success:
|
||||
|
||||
1. **main_test.go** - Audit for DB-only vs CLI tests
|
||||
2. **integrity_test.go** - Many helper calls, big win
|
||||
3. **export_import_test.go** - Already has helper pattern
|
||||
|
||||
### Phase 3: Special Cases (P3) - Complex Refactors
|
||||
Handle tests that need mixed isolation:
|
||||
|
||||
1. Review daemon tests for DB-only portions
|
||||
2. Review CLI tests for unit-testable logic
|
||||
3. Consider utility functions that don't need DB
|
||||
|
||||
## Success Metrics
|
||||
|
||||
### Before (Current):
|
||||
- **279-280 tests**
|
||||
- Each with `newTestStore()` = **280 DB initializations**
|
||||
- Estimated time: **8+ minutes**
|
||||
|
||||
### After (Proposed):
|
||||
- **10-15 test suites** for DB tests = **~15 DB initializations**
|
||||
- **~65 isolated tests** (daemon, git, filesystem) = **~65 DB initializations**
|
||||
- **Total: ~80 DB initializations** (down from 280)
|
||||
- Expected time: **1-2 minutes** (5-8x speedup)
|
||||
|
||||
### Per-Suite Expectations:
|
||||
|
||||
| Suite | Current | Proposed | Speedup |
|
||||
|-------|---------|----------|---------|
|
||||
| TestCreateSuite | 11 DBs | 1 DB | 10x |
|
||||
| TestDependencySuite | 4 DBs | 1 DB | 4x |
|
||||
| TestStaleSuite | 5 DBs | 1 DB | 5x |
|
||||
| TestIntegritySuite | 15 DBs | 1 DB | 15x |
|
||||
| TestMainSuite | 14 DBs | 1-2 DBs | 7-14x |
|
||||
|
||||
## Implementation Strategy
|
||||
|
||||
1. **Use label_test.go as template** - It already shows the pattern!
|
||||
2. **Start with create_test.go (bd-y6d)** - Clear, simple, 11 tests
|
||||
3. **Validate speedup** - Measure before/after for confidence
|
||||
4. **Apply pattern to other P1 files**
|
||||
5. **Document pattern in test_helpers_test.go** for future tests
|
||||
|
||||
## Key Insights
|
||||
|
||||
1. **~150 tests** can immediately benefit from shared DB setup
|
||||
2. **~65 tests** need isolation (daemon, git, filesystem)
|
||||
3. **~65 tests** need analysis (mixed or may not need DB)
|
||||
4. **label_test.go shows the ideal pattern** - use it as the template!
|
||||
5. **Primary bottleneck**: Repeated `newTestStore()` calls
|
||||
6. **Quick wins**: Files with 5+ tests using `newTestStore()`
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. ✓ Complete this audit (bd-c49)
|
||||
2. ✓ Refactor create_test.go (bd-y6d) using label_test.go pattern
|
||||
3. ✓ Measure and validate speedup
|
||||
4. ✓ Apply to remaining P1 files (bd-1rh)
|
||||
5. → Tackle P2 files (main_test.go, integrity_test.go, export_import_test.go)
|
||||
6. → Document best practices
|
||||
|
||||
## Phase 1 Completion Summary (bd-1rh)
|
||||
|
||||
**Status**: ✓ COMPLETE - All 6 P1 test files refactored
|
||||
**Runtime**: 0.43 seconds for all P1 tests
|
||||
**Speedup**: Estimated 10-20x improvement
|
||||
**Goal**: Under 2 minutes for full test suite after all phases - ON TRACK
|
||||
|
||||
### Key Learnings:
|
||||
|
||||
1. **Shared DB pattern works well** for most pure DB tests
|
||||
2. **Data pollution can occur** when tests create overlapping data (e.g., stale_test.go)
|
||||
3. **Solution for pollution**: Either use unique ID prefixes per subtest OR split into separate suites
|
||||
4. **ID prefix validation** requires test IDs to match "test-*" pattern
|
||||
5. **SQLite datetime functions** needed for timestamp manipulation in tests
|
||||
Reference in New Issue
Block a user