diff --git a/cmd/opencodereview/output_helpers_test.go b/cmd/opencodereview/output_helpers_test.go new file mode 100644 index 0000000..74d8106 --- /dev/null +++ b/cmd/opencodereview/output_helpers_test.go @@ -0,0 +1,328 @@ +package main + +import ( + "bytes" + "encoding/json" + "os" + "strings" + "testing" + "time" + + "github.com/open-code-review/open-code-review/internal/agent" + "github.com/open-code-review/open-code-review/internal/model" +) + +func TestHasSubtaskErrors(t *testing.T) { + tests := []struct { + name string + warnings []agent.AgentWarning + want bool + }{ + {"nil warnings", nil, false}, + {"empty", []agent.AgentWarning{}, false}, + {"no subtask errors", []agent.AgentWarning{{Type: "other", Message: "msg"}}, false}, + {"has subtask error", []agent.AgentWarning{{Type: "subtask_error", Message: "fail"}}, true}, + {"mixed", []agent.AgentWarning{{Type: "warn"}, {Type: "subtask_error"}}, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := hasSubtaskErrors(tc.warnings) + if got != tc.want { + t.Errorf("hasSubtaskErrors() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestWrapByRunes(t *testing.T) { + tests := []struct { + name string + text string + maxW int + lines int + }{ + {"empty", "", 80, 0}, + {"short line", "hello", 80, 1}, + {"exact width", strings.Repeat("a", 10), 10, 1}, + {"wraps long line", strings.Repeat("word ", 25), 20, 7}, + {"respects newlines", "line1\nline2\nline3", 80, 3}, + {"wrap with newlines", "short\n" + strings.Repeat("x", 50), 20, 4}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := wrapByRunes(tc.text, tc.maxW) + if len(got) != tc.lines { + t.Errorf("wrapByRunes() got %d lines, want %d\nlines: %v", len(got), tc.lines, got) + } + }) + } +} + +func TestWrapSingleRuneLine(t *testing.T) { + tests := []struct { + name string + line string + maxW int + min int + }{ + {"short line unchanged", "hello", 100, 1}, + {"wraps at space", "hello world foo bar baz", 12, 2}, + {"no space to wrap", strings.Repeat("x", 30), 10, 3}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := wrapSingleRuneLine(tc.line, tc.maxW) + if len(got) < tc.min { + t.Errorf("got %d lines, want at least %d", len(got), tc.min) + } + }) + } +} + +func TestRuneWrapCut(t *testing.T) { + // Short line returns full length + runes := []rune("short") + cut := runeWrapCut(runes, 100) + if cut != len(runes) { + t.Errorf("expected %d, got %d", len(runes), cut) + } + + // Cuts at space + runes = []rune("hello world test") + cut = runeWrapCut(runes, 11) + if runes[cut] != ' ' && cut != 11 { + t.Errorf("expected cut at space boundary, got %d (char=%c)", cut, runes[cut]) + } +} + +func TestVisibleRunesLen(t *testing.T) { + tests := []struct { + input string + want int + }{ + {"hello", 5}, + {"", 0}, + {"\x01\x02\x03", 0}, + {"a\x01b", 2}, + {"\x7f", 0}, + } + for _, tc := range tests { + got := visibleRunesLen([]rune(tc.input)) + if got != tc.want { + t.Errorf("visibleRunesLen(%q) = %d, want %d", tc.input, got, tc.want) + } + } +} + +func TestSplitToLines(t *testing.T) { + tests := []struct { + input string + want int + }{ + {"a\nb\nc", 3}, + {"a\nb\nc\n", 3}, + {"single", 1}, + {"crlf\r\nline", 2}, + {"", 0}, + } + for _, tc := range tests { + got := splitToLines(tc.input) + if len(got) != tc.want { + t.Errorf("splitToLines(%q) = %d lines, want %d", tc.input, len(got), tc.want) + } + } +} + +func TestBuildDiffLines(t *testing.T) { + t.Run("empty suggestion returns nil", func(t *testing.T) { + c := model.LlmComment{ExistingCode: "old", SuggestionCode: ""} + got := buildDiffLines(c) + if got != nil { + t.Errorf("expected nil, got %v", got) + } + }) + + t.Run("empty existing returns nil", func(t *testing.T) { + c := model.LlmComment{ExistingCode: "", SuggestionCode: "new"} + got := buildDiffLines(c) + if got != nil { + t.Errorf("expected nil, got %v", got) + } + }) + + t.Run("diff computed", func(t *testing.T) { + c := model.LlmComment{ + ExistingCode: "line1\nline2\n", + SuggestionCode: "line1\nmodified\n", + } + got := buildDiffLines(c) + if len(got) == 0 { + t.Error("expected non-empty diff lines") + } + }) +} + +func TestStatusBadge(t *testing.T) { + tests := []struct { + status string + substr string + }{ + {"added", "[A]"}, + {"modified", "[M]"}, + {"deleted", "[D]"}, + {"renamed", "[R]"}, + {"binary", "[B]"}, + {"scan", "[S]"}, + {"unknown", "[?]"}, + } + for _, tc := range tests { + got := statusBadge(tc.status) + if !strings.Contains(got, tc.substr) { + t.Errorf("statusBadge(%q) = %q, expected to contain %q", tc.status, got, tc.substr) + } + } +} + +func TestOutputJSON(t *testing.T) { + // Redirect stdout to capture output + old := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + comments := []model.LlmComment{ + {Path: "a.go", Content: "fix bug", StartLine: 1, EndLine: 5}, + } + err := outputJSON(comments) + + w.Close() + os.Stdout = old + + if err != nil { + t.Fatalf("outputJSON error: %v", err) + } + + var buf bytes.Buffer + buf.ReadFrom(r) + + var out jsonOutput + if err := json.Unmarshal(buf.Bytes(), &out); err != nil { + t.Fatalf("unmarshal output: %v", err) + } + if out.Status != "success" { + t.Errorf("status = %q, want success", out.Status) + } + if len(out.Comments) != 1 { + t.Errorf("expected 1 comment, got %d", len(out.Comments)) + } +} + +func TestOutputJSON_NoComments(t *testing.T) { + old := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + err := outputJSON(nil) + + w.Close() + os.Stdout = old + + if err != nil { + t.Fatalf("outputJSON error: %v", err) + } + + var buf bytes.Buffer + buf.ReadFrom(r) + + var out jsonOutput + json.Unmarshal(buf.Bytes(), &out) + if out.Message == "" { + t.Error("expected non-empty message when no comments") + } +} + +func TestOutputJSONWithWarnings(t *testing.T) { + old := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + comments := []model.LlmComment{{Path: "b.go", Content: "test"}} + warnings := []agent.AgentWarning{{Type: "subtask_error", File: "c.go", Message: "failed"}} + err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}) + + w.Close() + os.Stdout = old + + if err != nil { + t.Fatalf("error: %v", err) + } + + var buf bytes.Buffer + buf.ReadFrom(r) + + var out jsonOutput + json.Unmarshal(buf.Bytes(), &out) + if out.Status != "completed_with_errors" { + t.Errorf("status = %q, want completed_with_errors", out.Status) + } + if out.Summary == nil { + t.Fatal("expected non-nil summary") + } + if out.Summary.FilesReviewed != 5 { + t.Errorf("FilesReviewed = %d, want 5", out.Summary.FilesReviewed) + } + if out.ToolCalls == nil || out.ToolCalls.Total != 3 { + t.Errorf("ToolCalls.Total = %v", out.ToolCalls) + } +} + +func TestOutputJSONWithWarnings_NoCommentsNoErrors(t *testing.T) { + old := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + warnings := []agent.AgentWarning{{Type: "warning", Message: "something"}} + err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil) + + w.Close() + os.Stdout = old + + if err != nil { + t.Fatalf("error: %v", err) + } + + var buf bytes.Buffer + buf.ReadFrom(r) + + var out jsonOutput + json.Unmarshal(buf.Bytes(), &out) + if out.Status != "completed_with_warnings" { + t.Errorf("status = %q, want completed_with_warnings", out.Status) + } + if out.Message == "" { + t.Error("expected non-empty message") + } +} + +func TestOutputJSONNoFiles(t *testing.T) { + old := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + err := outputJSONNoFiles() + + w.Close() + os.Stdout = old + + if err != nil { + t.Fatalf("error: %v", err) + } + + var buf bytes.Buffer + buf.ReadFrom(r) + + var out jsonOutput + json.Unmarshal(buf.Bytes(), &out) + if out.Status != "skipped" { + t.Errorf("status = %q, want skipped", out.Status) + } +} diff --git a/internal/config/testconnection/testconnection_test.go b/internal/config/testconnection/testconnection_test.go new file mode 100644 index 0000000..9af8567 --- /dev/null +++ b/internal/config/testconnection/testconnection_test.go @@ -0,0 +1,92 @@ +package testconnection + +import ( + "testing" +) + +func TestLoadDefault(t *testing.T) { + conv, err := LoadDefault() + if err != nil { + t.Fatalf("LoadDefault: %v", err) + } + if conv == nil { + t.Fatal("expected non-nil conversation") + } + if conv.Timeout <= 0 { + t.Errorf("expected positive timeout, got %d", conv.Timeout) + } + if len(conv.Messages) == 0 { + t.Fatal("expected at least one message") + } + hasSystem := false + hasUser := false + for _, m := range conv.Messages { + switch m.Role { + case "system": + hasSystem = true + case "user": + hasUser = true + } + } + if !hasSystem { + t.Error("expected a system message") + } + if !hasUser { + t.Error("expected a user message") + } +} + +func TestResolveLang(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"", "English"}, + {"Chinese", "Chinese"}, + {"Japanese", "Japanese"}, + } + for _, tc := range tests { + got := resolveLang(tc.input) + if got != tc.want { + t.Errorf("resolveLang(%q) = %q, want %q", tc.input, got, tc.want) + } + } +} + +func TestApplyLanguage(t *testing.T) { + conv := &LlmConversation{ + Messages: []ChatMessage{ + {Role: "system", Content: "You are a bot."}, + {Role: "user", Content: "Hello"}, + }, + } + + conv.ApplyLanguage("Chinese") + + if conv.Messages[0].Content == "You are a bot." { + t.Error("expected system message to be modified") + } + expected := "You are a bot.\n\nAlways respond in Chinese." + if conv.Messages[0].Content != expected { + t.Errorf("system content = %q, want %q", conv.Messages[0].Content, expected) + } + // User message should not be modified + if conv.Messages[1].Content != "Hello" { + t.Errorf("user content should not change, got %q", conv.Messages[1].Content) + } +} + +func TestApplyLanguage_EmptyLang(t *testing.T) { + conv := &LlmConversation{ + Messages: []ChatMessage{ + {Role: "system", Content: "Base."}, + }, + } + + conv.ApplyLanguage("") + + expected := "Base.\n\nAlways respond in English." + if conv.Messages[0].Content != expected { + t.Errorf("content = %q, want %q", conv.Messages[0].Content, expected) + } +} diff --git a/internal/config/toolsconfig/toolsconfig_test.go b/internal/config/toolsconfig/toolsconfig_test.go new file mode 100644 index 0000000..bd25bb1 --- /dev/null +++ b/internal/config/toolsconfig/toolsconfig_test.go @@ -0,0 +1,102 @@ +package toolsconfig + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" +) + +func TestLoad_Default(t *testing.T) { + tools, err := Load("") + if err != nil { + t.Fatalf("Load default tools: %v", err) + } + if len(tools) == 0 { + t.Fatal("expected at least one tool from embedded config") + } + // Verify first tool has required fields + first := tools[0] + if first.Name == "" { + t.Error("expected non-empty tool name") + } + if first.Definition == nil { + t.Error("expected non-nil definition") + } +} + +func TestLoad_CustomFile(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "tools.json") + data := `[ + {"name": "test_tool", "plan_task": true, "main_task": false, "definition": {"name": "test_tool"}} + ]` + os.WriteFile(path, []byte(data), 0644) + + tools, err := Load(path) + if err != nil { + t.Fatalf("Load custom file: %v", err) + } + if len(tools) != 1 { + t.Fatalf("expected 1 tool, got %d", len(tools)) + } + if tools[0].Name != "test_tool" { + t.Errorf("expected name=test_tool, got %s", tools[0].Name) + } + if !tools[0].PlanTask { + t.Error("expected PlanTask=true") + } + if tools[0].MainTask { + t.Error("expected MainTask=false") + } +} + +func TestLoad_FileNotFound(t *testing.T) { + _, err := Load("/nonexistent/tools.json") + if err == nil { + t.Error("expected error for nonexistent file") + } +} + +func TestLoad_InvalidJSON(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "tools.json") + os.WriteFile(path, []byte("not json"), 0644) + + _, err := Load(path) + if err == nil { + t.Error("expected error for invalid JSON") + } +} + +func TestToolDefsByPhase(t *testing.T) { + def := json.RawMessage(`{"name": "test"}`) + tests := []struct { + name string + entry ToolConfigEntry + planOnly bool + wantOk bool + }{ + {"plan_task and planOnly=true", ToolConfigEntry{PlanTask: true, MainTask: false, Definition: def}, true, true}, + {"plan_task and planOnly=false", ToolConfigEntry{PlanTask: true, MainTask: false, Definition: def}, false, false}, + {"main_task and planOnly=false", ToolConfigEntry{PlanTask: false, MainTask: true, Definition: def}, false, true}, + {"main_task and planOnly=true", ToolConfigEntry{PlanTask: false, MainTask: true, Definition: def}, true, false}, + {"both and planOnly=true", ToolConfigEntry{PlanTask: true, MainTask: true, Definition: def}, true, true}, + {"both and planOnly=false", ToolConfigEntry{PlanTask: true, MainTask: true, Definition: def}, false, true}, + {"neither", ToolConfigEntry{PlanTask: false, MainTask: false, Definition: def}, true, false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, ok := tc.entry.ToolDefsByPhase(tc.planOnly) + if ok != tc.wantOk { + t.Errorf("ToolDefsByPhase(planOnly=%v) ok=%v, want %v", tc.planOnly, ok, tc.wantOk) + } + if tc.wantOk && got == nil { + t.Error("expected non-nil definition when ok=true") + } + if !tc.wantOk && got != nil { + t.Error("expected nil definition when ok=false") + } + }) + } +} diff --git a/internal/model/model_test.go b/internal/model/model_test.go new file mode 100644 index 0000000..8da62b8 --- /dev/null +++ b/internal/model/model_test.go @@ -0,0 +1,192 @@ +package model + +import ( + "encoding/json" + "testing" +) + +func TestDiff_JSONRoundTrip(t *testing.T) { + d := Diff{ + OldPath: "a.go", + NewPath: "b.go", + Diff: "@@ -1 +1 @@\n-old\n+new", + IsBinary: false, + IsDeleted: false, + IsNew: true, + IsRenamed: true, + Insertions: 5, + Deletions: 3, + } + + data, err := json.Marshal(d) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var got Diff + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if got != d { + t.Errorf("roundtrip mismatch:\n got %+v\n want %+v", got, d) + } +} + +func TestPreviewEntry_JSONRoundTrip(t *testing.T) { + e := PreviewEntry{ + Path: "main.go", + Status: "modified", + Insertions: 10, + Deletions: 2, + WillReview: true, + ExcludeReason: ExcludeNone, + } + + data, err := json.Marshal(e) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var got PreviewEntry + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if got != e { + t.Errorf("roundtrip mismatch:\n got %+v\n want %+v", got, e) + } +} + +func TestPreviewEntry_ExcludeReasonOmitEmpty(t *testing.T) { + e := PreviewEntry{ + Path: "a.go", + ExcludeReason: ExcludeNone, + } + data, _ := json.Marshal(e) + var m map[string]any + json.Unmarshal(data, &m) + if _, ok := m["exclude_reason"]; ok { + t.Error("expected exclude_reason to be omitted when empty") + } + + e.ExcludeReason = ExcludeUserRule + data, _ = json.Marshal(e) + json.Unmarshal(data, &m) + if m["exclude_reason"] != "user_exclude" { + t.Errorf("expected exclude_reason=user_exclude, got %v", m["exclude_reason"]) + } +} + +func TestExcludeReasonConstants(t *testing.T) { + constants := map[ExcludeReason]string{ + ExcludeNone: "", + ExcludeUserRule: "user_exclude", + ExcludeExtension: "unsupported_ext", + ExcludeDefaultPath: "default_path", + ExcludeDeleted: "deleted", + ExcludeBinary: "binary", + } + for k, v := range constants { + if string(k) != v { + t.Errorf("ExcludeReason constant mismatch: got %q, want %q", string(k), v) + } + } +} + +func TestScanItem_AsDiff(t *testing.T) { + item := &ScanItem{ + Path: "file.go", + Content: "package main\n", + IsBinary: false, + LineCount: 1, + } + + d := item.AsDiff() + if d == nil { + t.Fatal("expected non-nil Diff") + } + if d.OldPath != "file.go" { + t.Errorf("OldPath = %q, want file.go", d.OldPath) + } + if d.NewPath != "file.go" { + t.Errorf("NewPath = %q, want file.go", d.NewPath) + } + if d.NewFileContent != "package main\n" { + t.Errorf("NewFileContent = %q", d.NewFileContent) + } + if d.IsBinary { + t.Error("expected IsBinary=false") + } + if d.Insertions != 1 { + t.Errorf("Insertions = %d, want 1", d.Insertions) + } +} + +func TestScanItem_AsDiff_Nil(t *testing.T) { + var item *ScanItem + d := item.AsDiff() + if d != nil { + t.Error("expected nil Diff for nil ScanItem") + } +} + +func TestScanItem_AsDiff_Binary(t *testing.T) { + item := &ScanItem{ + Path: "image.png", + IsBinary: true, + } + d := item.AsDiff() + if !d.IsBinary { + t.Error("expected IsBinary=true") + } +} + +func TestLlmComment_JSON(t *testing.T) { + c := LlmComment{ + Path: "main.go", + Content: "fix this", + SuggestionCode: "new code", + ExistingCode: "old code", + StartLine: 10, + EndLine: 15, + Thinking: "reasoning", + } + + data, err := json.Marshal(c) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var got LlmComment + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if got != c { + t.Errorf("roundtrip mismatch:\n got %+v\n want %+v", got, c) + } +} + +func TestCodeReviewResult_JSON(t *testing.T) { + r := CodeReviewResult{ + RelevantFile: "api.go", + SuggestionContent: "suggestion", + ExistingCode: "old", + SuggestionCode: "new", + } + + data, err := json.Marshal(r) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var got CodeReviewResult + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if got != r { + t.Errorf("roundtrip mismatch:\n got %+v\n want %+v", got, r) + } +} diff --git a/internal/stdout/stdout_test.go b/internal/stdout/stdout_test.go new file mode 100644 index 0000000..aed675b --- /dev/null +++ b/internal/stdout/stdout_test.go @@ -0,0 +1,30 @@ +package stdout + +import ( + "io" + "os" + "testing" +) + +func TestWriter_Default(t *testing.T) { + w := Writer() + if w != os.Stdout { + t.Error("expected default Writer to be os.Stdout") + } +} + +func TestQuiet(t *testing.T) { + restore := Quiet() + + w := Writer() + if w != io.Discard { + t.Error("expected Writer to be io.Discard after Quiet()") + } + + restore() + + w = Writer() + if w != os.Stdout { + t.Error("expected Writer to be os.Stdout after restore") + } +} diff --git a/internal/telemetry/config_test.go b/internal/telemetry/config_test.go new file mode 100644 index 0000000..4edb8ce --- /dev/null +++ b/internal/telemetry/config_test.go @@ -0,0 +1,265 @@ +package telemetry + +import ( + "os" + "path/filepath" + "testing" +) + +func TestDefaultConfig(t *testing.T) { + cfg := DefaultConfig() + if cfg.Enabled { + t.Error("expected Enabled=false by default") + } + if cfg.ServiceName != "open-code-review" { + t.Errorf("expected ServiceName=open-code-review, got %s", cfg.ServiceName) + } + if cfg.Exporter != "console" { + t.Errorf("expected Exporter=console, got %s", cfg.Exporter) + } + if cfg.OTLPEndpoint != "" { + t.Errorf("expected empty OTLPEndpoint, got %s", cfg.OTLPEndpoint) + } + if cfg.OTLPProtocol != "grpc" { + t.Errorf("expected OTLPProtocol=grpc, got %s", cfg.OTLPProtocol) + } + if cfg.ContentLog { + t.Error("expected ContentLog=false by default") + } +} + +func TestResolveEnv(t *testing.T) { + tests := []struct { + name string + envs map[string]string + check func(t *testing.T, cfg Config) + }{ + { + name: "enable telemetry", + envs: map[string]string{"OCR_ENABLE_TELEMETRY": "1"}, + check: func(t *testing.T, cfg Config) { + if !cfg.Enabled { + t.Error("expected Enabled=true") + } + }, + }, + { + name: "custom service name", + envs: map[string]string{"OTEL_SERVICE_NAME": "my-service"}, + check: func(t *testing.T, cfg Config) { + if cfg.ServiceName != "my-service" { + t.Errorf("expected ServiceName=my-service, got %s", cfg.ServiceName) + } + }, + }, + { + name: "otlp endpoint sets exporter to otlp", + envs: map[string]string{"OTEL_EXPORTER_OTLP_ENDPOINT": "localhost:4317"}, + check: func(t *testing.T, cfg Config) { + if cfg.Exporter != "otlp" { + t.Errorf("expected Exporter=otlp, got %s", cfg.Exporter) + } + if cfg.OTLPEndpoint != "localhost:4317" { + t.Errorf("expected OTLPEndpoint=localhost:4317, got %s", cfg.OTLPEndpoint) + } + }, + }, + { + name: "otlp protocol override", + envs: map[string]string{"OTEL_EXPORTER_OTLP_PROTOCOL": "http/json"}, + check: func(t *testing.T, cfg Config) { + if cfg.OTLPProtocol != "http/json" { + t.Errorf("expected OTLPProtocol=http/json, got %s", cfg.OTLPProtocol) + } + }, + }, + { + name: "content logging enabled", + envs: map[string]string{"OCR_CONTENT_LOGGING": "1"}, + check: func(t *testing.T, cfg Config) { + if !cfg.ContentLog { + t.Error("expected ContentLog=true") + } + }, + }, + { + name: "no env vars leaves defaults", + envs: map[string]string{}, + check: func(t *testing.T, cfg Config) { + if cfg.Enabled { + t.Error("expected Enabled=false") + } + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Clear relevant env vars + envKeys := []string{ + "OCR_ENABLE_TELEMETRY", "OTEL_SERVICE_NAME", + "OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_PROTOCOL", + "OCR_CONTENT_LOGGING", + } + for _, k := range envKeys { + t.Setenv(k, "") + os.Unsetenv(k) + } + for k, v := range tc.envs { + t.Setenv(k, v) + } + + cfg := DefaultConfig() + resolveEnv(&cfg) + tc.check(t, cfg) + }) + } +} + +func TestLoadFromJSON(t *testing.T) { + t.Run("nonexistent file returns nil error", func(t *testing.T) { + cfg := DefaultConfig() + err := LoadFromJSON(&cfg, "/nonexistent/path/config.json") + if err != nil { + t.Errorf("expected nil error for missing file, got %v", err) + } + }) + + t.Run("malformed json returns nil error", func(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "config.json") + os.WriteFile(path, []byte("{invalid json"), 0644) + + cfg := DefaultConfig() + err := LoadFromJSON(&cfg, path) + if err != nil { + t.Errorf("expected nil error for malformed JSON, got %v", err) + } + }) + + t.Run("no telemetry section", func(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "config.json") + os.WriteFile(path, []byte(`{"other": "value"}`), 0644) + + cfg := DefaultConfig() + err := LoadFromJSON(&cfg, path) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if cfg.Enabled { + t.Error("expected Enabled to remain false") + } + }) + + t.Run("all fields set", func(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "config.json") + data := `{ + "telemetry": { + "enabled": true, + "exporter": "otlp", + "otlp_endpoint": "collector:4317", + "content_logging": true + } + }` + os.WriteFile(path, []byte(data), 0644) + + cfg := DefaultConfig() + err := LoadFromJSON(&cfg, path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !cfg.Enabled { + t.Error("expected Enabled=true") + } + if cfg.Exporter != "otlp" { + t.Errorf("expected Exporter=otlp, got %s", cfg.Exporter) + } + if cfg.OTLPEndpoint != "collector:4317" { + t.Errorf("expected OTLPEndpoint=collector:4317, got %s", cfg.OTLPEndpoint) + } + if !cfg.ContentLog { + t.Error("expected ContentLog=true") + } + }) + + t.Run("otlp_endpoint auto-sets exporter to otlp", func(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "config.json") + data := `{"telemetry": {"otlp_endpoint": "localhost:4317"}}` + os.WriteFile(path, []byte(data), 0644) + + cfg := DefaultConfig() + err := LoadFromJSON(&cfg, path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Exporter != "otlp" { + t.Errorf("expected Exporter=otlp, got %s", cfg.Exporter) + } + }) + + t.Run("exporter not overridden if already non-default", func(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "config.json") + data := `{"telemetry": {"exporter": "otlp"}}` + os.WriteFile(path, []byte(data), 0644) + + cfg := DefaultConfig() + cfg.Exporter = "custom" + err := LoadFromJSON(&cfg, path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Exporter != "custom" { + t.Errorf("expected Exporter to remain custom, got %s", cfg.Exporter) + } + }) +} + +func TestResolveConfig(t *testing.T) { + t.Run("empty path uses only env and defaults", func(t *testing.T) { + envKeys := []string{ + "OCR_ENABLE_TELEMETRY", "OTEL_SERVICE_NAME", + "OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_PROTOCOL", + "OCR_CONTENT_LOGGING", + } + for _, k := range envKeys { + t.Setenv(k, "") + os.Unsetenv(k) + } + + cfg := ResolveConfig("") + if cfg.Enabled { + t.Error("expected disabled with no env") + } + }) + + t.Run("env overrides json", func(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "config.json") + data := `{"telemetry": {"enabled": false}}` + os.WriteFile(path, []byte(data), 0644) + + t.Setenv("OCR_ENABLE_TELEMETRY", "1") + + cfg := ResolveConfig(path) + if !cfg.Enabled { + t.Error("expected env to override json: Enabled should be true") + } + }) +} + +func TestHomeConfigPath(t *testing.T) { + path := HomeConfigPath() + if path == "" { + t.Skip("could not determine home dir") + } + if !filepath.IsAbs(path) { + t.Errorf("expected absolute path, got %s", path) + } + if filepath.Base(path) != "config.json" { + t.Errorf("expected config.json at end, got %s", filepath.Base(path)) + } +} diff --git a/internal/telemetry/events_test.go b/internal/telemetry/events_test.go new file mode 100644 index 0000000..88e8b02 --- /dev/null +++ b/internal/telemetry/events_test.go @@ -0,0 +1,50 @@ +package telemetry + +import ( + "testing" + "time" +) + +func TestFormatDuration(t *testing.T) { + tests := []struct { + dur time.Duration + want string + }{ + {0, "0s"}, + {1500 * time.Millisecond, "1.5s"}, + {60 * time.Second, "1m0s"}, + {123 * time.Millisecond, "123ms"}, + {2*time.Minute + 30*time.Second, "2m30s"}, + } + for _, tc := range tests { + got := FormatDuration(tc.dur) + if got != tc.want { + t.Errorf("FormatDuration(%v) = %q, want %q", tc.dur, got, tc.want) + } + } +} + +func TestSummarizeArgs(t *testing.T) { + tests := []struct { + name string + args map[string]any + want string + }{ + {"nil map", nil, ""}, + {"empty map", map[string]any{}, ""}, + {"path key returns quoted", map[string]any{"path": "foo/bar.go"}, `"foo/bar.go"`}, + {"search key returns quoted", map[string]any{"search": "hello"}, `"hello"`}, + {"query key returns quoted", map[string]any{"query": "world"}, `"world"`}, + {"pattern key returns quoted", map[string]any{"pattern": "*.go"}, `"*.go"`}, + {"generic short value", map[string]any{"foo": "bar"}, "foo=bar"}, + {"long value skipped", map[string]any{"data": string(make([]byte, 60))}, ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := summarizeArgs(tc.args) + if got != tc.want { + t.Errorf("summarizeArgs(%v) = %q, want %q", tc.args, got, tc.want) + } + }) + } +} diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go new file mode 100644 index 0000000..c678964 --- /dev/null +++ b/internal/telemetry/metrics_test.go @@ -0,0 +1,30 @@ +package telemetry + +import ( + "context" + "fmt" + "testing" + "time" +) + +func TestRecordFunctions_DisabledTelemetry(t *testing.T) { + // Reset state so telemetry is disabled + initialized = false + shutdownFuncs = nil + + ctx := context.Background() + + // These should all be no-ops when telemetry is disabled + RecordReviewDuration(ctx, 5*time.Second) + RecordFilesReviewed(ctx, 10) + RecordCommentsGenerated(ctx, 3) + RecordLLMRequest(ctx, "gpt-4", 2*time.Second, 1000, "ok") + RecordToolCall(ctx, "file_read", 100*time.Millisecond, true) + RecordToolCall(ctx, "file_read", 100*time.Millisecond, false) +} + +func TestCheckMetricErr(t *testing.T) { + // Should not panic + checkMetricErr(nil) + checkMetricErr(fmt.Errorf("some error")) +} diff --git a/internal/telemetry/provider_test.go b/internal/telemetry/provider_test.go new file mode 100644 index 0000000..1454e54 --- /dev/null +++ b/internal/telemetry/provider_test.go @@ -0,0 +1,52 @@ +package telemetry + +import ( + "context" + "testing" +) + +func TestIsEnabled_NotInitialized(t *testing.T) { + // Reset global state + initialized = false + shutdownFuncs = nil + + if IsEnabled() { + t.Error("expected IsEnabled()=false when not initialized") + } +} + +func TestIsEnabled_InitializedButNoShutdowns(t *testing.T) { + initialized = true + shutdownFuncs = nil + defer func() { + initialized = false + }() + + if IsEnabled() { + t.Error("expected IsEnabled()=false when no shutdown funcs registered") + } +} + +func TestIsEnabled_WithShutdowns(t *testing.T) { + initialized = true + shutdownFuncs = []func(context.Context) error{ + func(ctx context.Context) error { return nil }, + } + defer func() { + initialized = false + shutdownFuncs = nil + }() + + if !IsEnabled() { + t.Error("expected IsEnabled()=true when shutdown funcs registered") + } +} + +func TestContentLogging_Disabled(t *testing.T) { + initialized = false + shutdownFuncs = nil + + if ContentLogging() { + t.Error("expected ContentLogging()=false when telemetry disabled") + } +} diff --git a/internal/telemetry/shutdown_test.go b/internal/telemetry/shutdown_test.go new file mode 100644 index 0000000..a56f999 --- /dev/null +++ b/internal/telemetry/shutdown_test.go @@ -0,0 +1,71 @@ +package telemetry + +import ( + "context" + "errors" + "testing" + "time" +) + +func TestShutdown_NoFuncs(t *testing.T) { + shutdownFuncs = nil + err := Shutdown(context.Background()) + if err != nil { + t.Errorf("expected nil error, got %v", err) + } +} + +func TestShutdown_Success(t *testing.T) { + called := false + shutdownFuncs = []func(context.Context) error{ + func(ctx context.Context) error { + called = true + return nil + }, + } + defer func() { shutdownFuncs = nil }() + + err := Shutdown(context.Background()) + if err != nil { + t.Errorf("expected nil error, got %v", err) + } + if !called { + t.Error("expected shutdown function to be called") + } + if shutdownFuncs != nil { + t.Error("expected shutdownFuncs to be cleared after shutdown") + } +} + +func TestShutdown_WithErrors(t *testing.T) { + shutdownFuncs = []func(context.Context) error{ + func(ctx context.Context) error { return nil }, + func(ctx context.Context) error { return errors.New("fail1") }, + func(ctx context.Context) error { return errors.New("fail2") }, + } + defer func() { shutdownFuncs = nil }() + + err := Shutdown(context.Background()) + if err == nil { + t.Fatal("expected error") + } + if shutdownFuncs != nil { + t.Error("expected shutdownFuncs to be cleared even on error") + } +} + +func TestShutdownWithTimeout(t *testing.T) { + called := false + shutdownFuncs = []func(context.Context) error{ + func(ctx context.Context) error { + called = true + return nil + }, + } + defer func() { shutdownFuncs = nil }() + + ShutdownWithTimeout(context.Background(), 5*time.Second) + if !called { + t.Error("expected shutdown function to be called via ShutdownWithTimeout") + } +} diff --git a/internal/telemetry/span_test.go b/internal/telemetry/span_test.go new file mode 100644 index 0000000..033561b --- /dev/null +++ b/internal/telemetry/span_test.go @@ -0,0 +1,43 @@ +package telemetry + +import ( + "fmt" + "testing" + + "go.opentelemetry.io/otel/attribute" +) + +func TestAnyToAttr(t *testing.T) { + tests := []struct { + name string + key string + val interface{} + want attribute.KeyValue + }{ + {"string", "k", "hello", attribute.String("k", "hello")}, + {"int", "k", 42, attribute.Int64("k", 42)}, + {"int64", "k", int64(100), attribute.Int64("k", 100)}, + {"bool", "k", true, attribute.Bool("k", true)}, + {"float64", "k", 3.14, attribute.Float64("k", 3.14)}, + {"default fallback", "k", []int{1, 2}, attribute.String("k", fmt.Sprintf("%v", []int{1, 2}))}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := AnyToAttr(tc.key, tc.val) + if got != tc.want { + t.Errorf("AnyToAttr(%q, %v) = %v, want %v", tc.key, tc.val, got, tc.want) + } + }) + } +} + +func TestSetAttr_NilSpan(t *testing.T) { + // Should not panic + SetAttr(nil, "key", "value") +} + +func TestRecordToolResult_NilSpan(t *testing.T) { + // Should not panic + RecordToolResult(nil, "tool", 100, nil) + RecordToolResult(nil, "tool", 100, fmt.Errorf("err")) +}