From 5ab7ff0f84cea3abe6571b948ff0740636396f51 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Wed, 15 Oct 2025 19:33:53 -0700 Subject: [PATCH] Add comprehensive unit tests for CreateIssues (bd-241) - Added 12 table-driven test cases covering all validation scenarios - Added 2 rollback tests (validation error + DB conflict) - Added nil item detection to prevent panic - Fixed nil pointer panic in CreateIssues - Tests verify ID uniqueness, timestamps, closed_at invariant - All tests pass Amp-Thread-ID: https://ampcode.com/threads/T-61c584cd-d873-4a1a-bfa6-d739b630b3e5 Co-authored-by: Amp --- .beads/issues.jsonl | 2 +- internal/storage/sqlite/sqlite.go | 6 +- internal/storage/sqlite/sqlite_test.go | 426 +++++++++++++++++++++++++ 3 files changed, 432 insertions(+), 2 deletions(-) diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index a8937099..d99d6082 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -156,7 +156,7 @@ {"id":"bd-239","title":"Fix import.go to normalize closed_at before creating issues","description":"Modify cmd/bd/import.go to enforce closed_at invariant before calling CreateIssue/CreateIssues.\n\nNormalize data: if status=closed, ensure closed_at is set; if status!=closed, ensure closed_at is nil.","design":"```go\nif _, ok := rawData[\"status\"]; ok {\n updates[\"status\"] = issue.Status\n\n // Enforce closed_at invariant\n if issue.Status == types.StatusClosed {\n // Status is closed: ensure closed_at is set\n if issue.ClosedAt == nil {\n now := time.Now()\n updates[\"closed_at\"] = now\n } else {\n updates[\"closed_at\"] = *issue.ClosedAt\n }\n } else {\n // Status is not closed: ensure closed_at is NULL\n updates[\"closed_at\"] = nil\n }\n}\n```","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-15T14:21:11.837762-07:00","updated_at":"2025-10-15T16:27:28.022212-07:00","closed_at":"2025-10-15T16:27:28.022212-07:00","dependencies":[{"issue_id":"bd-239","depends_on_id":"bd-224","type":"parent-child","created_at":"2025-10-15T14:21:11.838751-07:00","created_by":"stevey"},{"issue_id":"bd-239","depends_on_id":"bd-240","type":"blocks","created_at":"2025-10-15T14:22:36.49138-07:00","created_by":"stevey"}]} {"id":"bd-24","title":"Support ID space partitioning for parallel worker agents","description":"Enable external orchestrators (like AI worker swarms) to control issue ID assignment. Add --id flag to 'bd create' for explicit ID specification. Optionally support 'bd config set next_id N' to set the starting point for auto-increment. Storage layer already supports pre-assigned IDs (sqlite.go:52-71), just need CLI wiring. This keeps beads simple while letting orchestrators implement their own ID partitioning strategies to minimize merge conflicts. Complementary to bd-9's collision resolution.","status":"closed","priority":1,"issue_type":"feature","created_at":"2025-10-14T14:43:06.910467-07:00","updated_at":"2025-10-15T16:27:21.997209-07:00","closed_at":"2025-10-15T03:01:29.569795-07:00"} {"id":"bd-240","title":"Add CreateIssues interface method to Storage","description":"Add CreateIssues to the Storage interface in storage/storage.go\n\nNon-breaking addition to interface for batch issue creation.","design":"```go\n// storage/storage.go\ntype Storage interface {\n CreateIssue(ctx context.Context, issue *types.Issue, actor string) error\n CreateIssues(ctx context.Context, issues []*types.Issue, actor string) error // NEW\n // ... rest unchanged\n}\n```","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-15T14:21:21.252413-07:00","updated_at":"2025-10-15T18:30:09.264339-07:00","closed_at":"2025-10-15T18:30:09.264339-07:00","dependencies":[{"issue_id":"bd-240","depends_on_id":"bd-222","type":"parent-child","created_at":"2025-10-15T14:21:21.253617-07:00","created_by":"stevey"},{"issue_id":"bd-240","depends_on_id":"bd-224","type":"blocks","created_at":"2025-10-15T14:21:21.254504-07:00","created_by":"stevey"}]} -{"id":"bd-241","title":"Add comprehensive unit tests for CreateIssues","description":"Test coverage for CreateIssues:\n- Empty batch\n- Single issue\n- Multiple issues\n- Mixed ID assignment (explicit + auto-generated)\n- Validation errors\n- Duplicate ID errors\n- Rollback on error\n- Verify closed_at invariant enforced","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-15T14:21:47.237196-07:00","updated_at":"2025-10-15T16:27:21.998143-07:00","dependencies":[{"issue_id":"bd-241","depends_on_id":"bd-222","type":"parent-child","created_at":"2025-10-15T14:21:47.246448-07:00","created_by":"stevey"},{"issue_id":"bd-241","depends_on_id":"bd-240","type":"blocks","created_at":"2025-10-15T14:21:47.247811-07:00","created_by":"stevey"}]} +{"id":"bd-241","title":"Add comprehensive unit tests for CreateIssues","description":"Test coverage for CreateIssues:\n- Empty batch\n- Single issue\n- Multiple issues\n- Mixed ID assignment (explicit + auto-generated)\n- Validation errors\n- Duplicate ID errors\n- Rollback on error\n- Verify closed_at invariant enforced","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-15T14:21:47.237196-07:00","updated_at":"2025-10-15T19:16:35.461354-07:00","closed_at":"2025-10-15T19:16:35.461354-07:00","dependencies":[{"issue_id":"bd-241","depends_on_id":"bd-222","type":"parent-child","created_at":"2025-10-15T14:21:47.246448-07:00","created_by":"stevey"},{"issue_id":"bd-241","depends_on_id":"bd-240","type":"blocks","created_at":"2025-10-15T14:21:47.247811-07:00","created_by":"stevey"}]} {"id":"bd-242","title":"Update import.go to use CreateIssues for bulk imports","description":"Modify cmd/bd/import.go to use CreateIssues instead of CreateIssue loop.\n\nAfter bd-224, import already normalizes closed_at, so this is straightforward:\n1. Normalize all issues in batch (closed_at handling)\n2. Call CreateIssues once with full batch\n3. Much simpler than current loop","design":"```go\n// After normalizing all issues\nfor _, issue := range issues {\n if issue.Status == types.StatusClosed {\n if issue.ClosedAt == nil {\n now := time.Now()\n issue.ClosedAt = \u0026now\n }\n } else {\n issue.ClosedAt = nil\n }\n}\n\n// Single batch call (5-15x faster!)\nreturn store.CreateIssues(ctx, issues, \"import\")\n```","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-15T14:21:47.258493-07:00","updated_at":"2025-10-15T16:27:21.998773-07:00","dependencies":[{"issue_id":"bd-242","depends_on_id":"bd-222","type":"parent-child","created_at":"2025-10-15T14:21:47.259318-07:00","created_by":"stevey"},{"issue_id":"bd-242","depends_on_id":"bd-240","type":"blocks","created_at":"2025-10-15T14:21:47.25982-07:00","created_by":"stevey"}]} {"id":"bd-243","title":"Document CreateIssues API and update EXTENDING.md","description":"Documentation updates:\n- Godoc for CreateIssues with usage guidance\n- Add batch import examples\n- Update EXTENDING.md with batch usage patterns\n- Performance notes in README.md\n- When to use CreateIssue vs CreateIssues","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-15T14:21:47.398473-07:00","updated_at":"2025-10-15T16:27:21.99948-07:00","dependencies":[{"issue_id":"bd-243","depends_on_id":"bd-222","type":"parent-child","created_at":"2025-10-15T14:21:47.398904-07:00","created_by":"stevey"},{"issue_id":"bd-243","depends_on_id":"bd-240","type":"blocks","created_at":"2025-10-15T14:21:47.399336-07:00","created_by":"stevey"}]} {"id":"bd-244","title":"Implement SQLiteStorage.CreateIssues with atomic ID range reservation","description":"Core implementation of CreateIssues in internal/storage/sqlite/sqlite.go\n\nKey optimizations:\n- Single connection + transaction\n- Atomic ID range reservation (generate N IDs in one counter update)\n- Prepared statement for bulk inserts\n- All-or-nothing atomicity\n\nExpected 5-10x speedup for N\u003e10 issues.","design":"Implementation phases per ULTRATHINK_BD222.md:\n\n1. **Validation**: Pre-validate all issues (calls Issue.Validate() which enforces closed_at invariant from bd-224)\n2. **Connection \u0026 Transaction**: BEGIN IMMEDIATE (same as CreateIssue)\n3. **Batch ID Generation**: Reserve range [nextID, nextID+N) in single counter update\n4. **Bulk Insert**: Prepared statement loop (defer multi-VALUE INSERT optimization)\n5. **Bulk Events**: Record creation events for all issues\n6. **Bulk Dirty**: Mark all issues dirty for export\n7. **Commit**: All-or-nothing transaction commit\n\nSee ULTRATHINK_BD222.md lines 344-541 for full implementation details.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-15T14:21:53.433641-07:00","updated_at":"2025-10-15T18:31:28.771539-07:00","closed_at":"2025-10-15T18:31:28.771539-07:00","dependencies":[{"issue_id":"bd-244","depends_on_id":"bd-222","type":"parent-child","created_at":"2025-10-15T14:21:53.435109-07:00","created_by":"stevey"},{"issue_id":"bd-244","depends_on_id":"bd-240","type":"blocks","created_at":"2025-10-15T14:21:53.43563-07:00","created_by":"stevey"},{"issue_id":"bd-244","depends_on_id":"bd-241","type":"blocks","created_at":"2025-10-15T14:22:17.181984-07:00","created_by":"stevey"},{"issue_id":"bd-244","depends_on_id":"bd-242","type":"blocks","created_at":"2025-10-15T14:22:17.195635-07:00","created_by":"stevey"}]} diff --git a/internal/storage/sqlite/sqlite.go b/internal/storage/sqlite/sqlite.go index fa6c5f74..5ef728f9 100644 --- a/internal/storage/sqlite/sqlite.go +++ b/internal/storage/sqlite/sqlite.go @@ -518,9 +518,13 @@ func (s *SQLiteStorage) CreateIssues(ctx context.Context, issues []*types.Issue, return nil } - // Phase 1: Set timestamps and validate all issues first (fail-fast) + // Phase 1: Check for nil and validate all issues first (fail-fast) now := time.Now() for i, issue := range issues { + if issue == nil { + return fmt.Errorf("issue %d is nil", i) + } + issue.CreatedAt = now issue.UpdatedAt = now diff --git a/internal/storage/sqlite/sqlite_test.go b/internal/storage/sqlite/sqlite_test.go index 5833009d..4d8adb1d 100644 --- a/internal/storage/sqlite/sqlite_test.go +++ b/internal/storage/sqlite/sqlite_test.go @@ -192,6 +192,432 @@ func TestGetIssueNotFound(t *testing.T) { } } +func TestCreateIssues(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + tests := []struct { + name string + issues []*types.Issue + wantErr bool + checkFunc func(t *testing.T, issues []*types.Issue) + }{ + { + name: "empty batch", + issues: []*types.Issue{}, + wantErr: false, + checkFunc: func(t *testing.T, issues []*types.Issue) { + if len(issues) != 0 { + t.Errorf("expected 0 issues, got %d", len(issues)) + } + }, + }, + { + name: "single issue", + issues: []*types.Issue{ + { + Title: "Single issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + }, + wantErr: false, + checkFunc: func(t *testing.T, issues []*types.Issue) { + if len(issues) != 1 { + t.Fatalf("expected 1 issue, got %d", len(issues)) + } + if issues[0].ID == "" { + t.Error("issue ID should be set") + } + if !issues[0].CreatedAt.After(time.Time{}) { + t.Error("CreatedAt should be set") + } + if !issues[0].UpdatedAt.After(time.Time{}) { + t.Error("UpdatedAt should be set") + } + }, + }, + { + name: "multiple issues", + issues: []*types.Issue{ + { + Title: "Issue 1", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + { + Title: "Issue 2", + Status: types.StatusInProgress, + Priority: 2, + IssueType: types.TypeBug, + }, + { + Title: "Issue 3", + Status: types.StatusOpen, + Priority: 3, + IssueType: types.TypeFeature, + }, + }, + wantErr: false, + checkFunc: func(t *testing.T, issues []*types.Issue) { + if len(issues) != 3 { + t.Fatalf("expected 3 issues, got %d", len(issues)) + } + for i, issue := range issues { + if issue.ID == "" { + t.Errorf("issue %d: ID should be set", i) + } + if !issue.CreatedAt.After(time.Time{}) { + t.Errorf("issue %d: CreatedAt should be set", i) + } + if !issue.UpdatedAt.After(time.Time{}) { + t.Errorf("issue %d: UpdatedAt should be set", i) + } + } + // Verify IDs are unique + ids := make(map[string]bool) + for _, issue := range issues { + if ids[issue.ID] { + t.Errorf("duplicate ID found: %s", issue.ID) + } + ids[issue.ID] = true + } + }, + }, + { + name: "mixed ID assignment - explicit and auto-generated", + issues: []*types.Issue{ + { + ID: "custom-1", + Title: "Custom ID 1", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + { + Title: "Auto ID", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + { + ID: "custom-2", + Title: "Custom ID 2", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + }, + wantErr: false, + checkFunc: func(t *testing.T, issues []*types.Issue) { + if len(issues) != 3 { + t.Fatalf("expected 3 issues, got %d", len(issues)) + } + if issues[0].ID != "custom-1" { + t.Errorf("expected ID 'custom-1', got %s", issues[0].ID) + } + if issues[1].ID == "" || issues[1].ID == "custom-1" || issues[1].ID == "custom-2" { + t.Errorf("expected auto-generated ID, got %s", issues[1].ID) + } + if issues[2].ID != "custom-2" { + t.Errorf("expected ID 'custom-2', got %s", issues[2].ID) + } + }, + }, + { + name: "validation error - missing title", + issues: []*types.Issue{ + { + Title: "Valid issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + { + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + }, + wantErr: true, + checkFunc: func(t *testing.T, issues []*types.Issue) { + // Should not be called on error + }, + }, + { + name: "validation error - invalid priority", + issues: []*types.Issue{ + { + Title: "Test", + Status: types.StatusOpen, + Priority: 10, + IssueType: types.TypeTask, + }, + }, + wantErr: true, + checkFunc: func(t *testing.T, issues []*types.Issue) { + // Should not be called on error + }, + }, + { + name: "validation error - invalid status", + issues: []*types.Issue{ + { + Title: "Test", + Status: "invalid", + Priority: 1, + IssueType: types.TypeTask, + }, + }, + wantErr: true, + checkFunc: func(t *testing.T, issues []*types.Issue) { + // Should not be called on error + }, + }, + { + name: "duplicate ID error", + issues: []*types.Issue{ + { + ID: "duplicate-id", + Title: "First issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + { + ID: "duplicate-id", + Title: "Second issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + }, + wantErr: true, + checkFunc: func(t *testing.T, issues []*types.Issue) { + // Should not be called on error + }, + }, + { + name: "closed_at invariant - open status with closed_at", + issues: []*types.Issue{ + { + Title: "Invalid closed_at", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + ClosedAt: &time.Time{}, + }, + }, + wantErr: true, + checkFunc: func(t *testing.T, issues []*types.Issue) { + // Should not be called on error + }, + }, + { + name: "closed_at invariant - closed status without closed_at", + issues: []*types.Issue{ + { + Title: "Missing closed_at", + Status: types.StatusClosed, + Priority: 1, + IssueType: types.TypeTask, + }, + }, + wantErr: true, + checkFunc: func(t *testing.T, issues []*types.Issue) { + // Should not be called on error + }, + }, + { + name: "nil item in batch", + issues: []*types.Issue{ + { + Title: "Valid issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + nil, + }, + wantErr: true, + checkFunc: func(t *testing.T, issues []*types.Issue) { + // Should not be called on error + }, + }, + { + name: "valid closed issue with closed_at", + issues: []*types.Issue{ + { + Title: "Properly closed", + Status: types.StatusClosed, + Priority: 1, + IssueType: types.TypeTask, + ClosedAt: func() *time.Time { t := time.Now(); return &t }(), + }, + }, + wantErr: false, + checkFunc: func(t *testing.T, issues []*types.Issue) { + if len(issues) != 1 { + t.Fatalf("expected 1 issue, got %d", len(issues)) + } + if issues[0].Status != types.StatusClosed { + t.Errorf("expected closed status, got %s", issues[0].Status) + } + if issues[0].ClosedAt == nil { + t.Error("ClosedAt should be set for closed issue") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := store.CreateIssues(ctx, tt.issues, "test-user") + if (err != nil) != tt.wantErr { + t.Errorf("CreateIssues() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if tt.wantErr { + // Verify IDs weren't auto-generated on error (timestamps may be set) + for i, issue := range tt.issues { + if issue == nil { + continue + } + // Allow pre-set IDs (custom-1, existing-id, duplicate-id, etc.) + hasCustomID := issue.ID != "" && (issue.ID == "custom-1" || issue.ID == "custom-2" || + issue.ID == "duplicate-id" || issue.ID == "existing-id") + if !hasCustomID && issue.ID != "" { + t.Errorf("issue %d: ID should not be auto-generated on error, got %s", i, issue.ID) + } + } + } + + if !tt.wantErr && tt.checkFunc != nil { + tt.checkFunc(t, tt.issues) + } + }) + } +} + +func TestCreateIssuesRollback(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + t.Run("rollback on validation error", func(t *testing.T) { + // Create a valid issue first + validIssue := &types.Issue{ + Title: "Valid issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + err := store.CreateIssue(ctx, validIssue, "test-user") + if err != nil { + t.Fatalf("failed to create valid issue: %v", err) + } + + // Try to create batch with one valid and one invalid issue + issues := []*types.Issue{ + { + Title: "Another valid issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + { + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + } + + err = store.CreateIssues(ctx, issues, "test-user") + if err == nil { + t.Fatal("expected error for invalid batch, got nil") + } + + // Verify the "Another valid issue" was rolled back by searching all issues + filter := types.IssueFilter{} + allIssues, err := store.SearchIssues(ctx, "", filter) + if err != nil { + t.Fatalf("failed to search issues: %v", err) + } + + // Should only have the first valid issue, not the rolled-back one + if len(allIssues) != 1 { + t.Errorf("expected 1 issue after rollback, got %d", len(allIssues)) + } + + if len(allIssues) > 0 && allIssues[0].ID != validIssue.ID { + t.Errorf("expected only the first valid issue, got %s", allIssues[0].ID) + } + }) + + t.Run("rollback on conflict with existing ID", func(t *testing.T) { + // Create an issue with explicit ID + existingIssue := &types.Issue{ + ID: "existing-id", + Title: "Existing issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + err := store.CreateIssue(ctx, existingIssue, "test-user") + if err != nil { + t.Fatalf("failed to create existing issue: %v", err) + } + + // Try to create batch with conflicting ID + issues := []*types.Issue{ + { + Title: "Should rollback", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + { + ID: "existing-id", + Title: "Conflict", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + }, + } + + err = store.CreateIssues(ctx, issues, "test-user") + if err == nil { + t.Fatal("expected error for duplicate ID, got nil") + } + + // Verify rollback - "Should rollback" issue should not exist + filter := types.IssueFilter{} + allIssues, err := store.SearchIssues(ctx, "", filter) + if err != nil { + t.Fatalf("failed to search issues: %v", err) + } + + // Count should only include the pre-existing issues + foundRollback := false + for _, issue := range allIssues { + if issue.Title == "Should rollback" { + foundRollback = true + break + } + } + + if foundRollback { + t.Error("expected rollback of all issues in batch, but 'Should rollback' was found") + } + }) +} + func TestUpdateIssue(t *testing.T) { store, cleanup := setupTestDB(t) defer cleanup()