diff --git a/Makefile b/Makefile index fd1e389..bb83bc0 100644 --- a/Makefile +++ b/Makefile @@ -31,8 +31,10 @@ endef build: $(GO) build -ldflags "$(LD_FLAGS)" -o $(DIST_DIR)/$(BINARY_NAME) ./cmd/opencodereview +PACKAGES := $(shell $(GO) list ./... | grep -v /extensions/) + test: - LC_ALL=C $(GO) test -v -race -count=1 ./... + LC_ALL=C $(GO) test -v -race -count=1 $(PACKAGES) clean: rm -rf $(DIST_DIR) @@ -44,15 +46,15 @@ help: build $(DIST_DIR)/$(BINARY_NAME) -h fmt: - $(GO) fmt ./... + $(GO) fmt $(PACKAGES) vet: - LC_ALL=C $(GO) vet ./... + LC_ALL=C $(GO) vet $(PACKAGES) check: $(GO) mod tidy - $(GO) fmt ./... - LC_ALL=C $(GO) vet ./... + $(GO) fmt $(PACKAGES) + LC_ALL=C $(GO) vet $(PACKAGES) @echo "check passed" # ── Cross-platform targets ─────────────────────────────────────────────────── diff --git a/internal/telemetry/events_test.go b/internal/telemetry/events_test.go index 88e8b02..1bcd170 100644 --- a/internal/telemetry/events_test.go +++ b/internal/telemetry/events_test.go @@ -1,10 +1,162 @@ package telemetry import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "strings" "testing" "time" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/resource" + sdktrace "go.opentelemetry.io/otel/sdk/trace" ) +func setupEnabledTelemetry(t *testing.T) { + t.Helper() + tp := sdktrace.NewTracerProvider(sdktrace.WithResource(resource.Default())) + mp := sdkmetric.NewMeterProvider(sdkmetric.WithResource(resource.Default())) + otel.SetTracerProvider(tp) + otel.SetMeterProvider(mp) + tracerProvider = tp + meterProvider = mp + initialized = true + shutdownFuncs = []func(context.Context) error{ + func(ctx context.Context) error { return tp.Shutdown(ctx) }, + func(ctx context.Context) error { return mp.Shutdown(ctx) }, + } + initMetricsOnce = false + t.Cleanup(func() { + _ = tp.Shutdown(context.Background()) + _ = mp.Shutdown(context.Background()) + tracerProvider = nil + meterProvider = nil + initialized = false + shutdownFuncs = nil + initMetricsOnce = false + }) +} + +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + oldStderr := os.Stderr + os.Stderr = w + defer func() { os.Stderr = oldStderr }() + + fn() + w.Close() + + var buf bytes.Buffer + _, _ = io.Copy(&buf, r) + return buf.String() +} + +func TestEvent_Enabled(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + Event(ctx, "test.event", attribute.String("key", "value")) +} + +func TestEvent_Disabled(t *testing.T) { + initialized = false + shutdownFuncs = nil + defer func() { initialized = false }() + Event(context.Background(), "test.event") +} + +func TestEvent_NilCtx(t *testing.T) { + setupEnabledTelemetry(t) + Event(nil, "test.event") //nolint:staticcheck +} + +func TestEventf_Enabled(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + Eventf(ctx, "test.eventf", "hello world", attribute.Int("count", 5)) +} + +func TestErrorEvent_Enabled(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + ErrorEvent(ctx, "test.error", errors.New("something failed"), attribute.String("detail", "extra")) +} + +func TestErrorEvent_NilErr(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + ErrorEvent(ctx, "test.error", nil) +} + +func TestErrorEvent_NilCtx(t *testing.T) { + setupEnabledTelemetry(t) + ErrorEvent(nil, "test.error", errors.New("fail")) //nolint:staticcheck +} + +func TestErrorEvent_Disabled(t *testing.T) { + initialized = false + shutdownFuncs = nil + defer func() { initialized = false }() + ErrorEvent(context.Background(), "test.error", errors.New("fail")) +} + +func TestPhaseEvent_Success(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + PhaseEvent(ctx, "scan", "main.go", 500*time.Millisecond, nil) +} + +func TestPhaseEvent_WithError(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + PhaseEvent(ctx, "scan", "main.go", 500*time.Millisecond, errors.New("parse error")) +} + +func TestPrintTraceSummary_WithTokenDetails(t *testing.T) { + PrintTraceSummary(5, 10, 1000, 200, 1200, 0, 0, 3*time.Second) +} + +func TestPrintTraceSummary_WithCacheTokens(t *testing.T) { + PrintTraceSummary(3, 2, 500, 100, 600, 200, 50, 2*time.Second) +} + +func TestPrintTraceSummary_NoTokenDetails(t *testing.T) { + PrintTraceSummary(2, 1, 0, 0, 500, 0, 0, 1*time.Second) +} + +func TestPrintToolCallStarted_WithArgs(t *testing.T) { + PrintToolCallStarted("file_read", map[string]any{"path": "main.go"}) +} + +func TestPrintToolCallStarted_NoArgs(t *testing.T) { + PrintToolCallStarted("list_files", nil) +} + +func TestPrintToolCallFinished(t *testing.T) { + PrintToolCallFinished("file_read", 123*time.Millisecond) +} + +func TestPrintToolCallError(t *testing.T) { + out := captureStderr(t, func() { + PrintToolCallError("file_read", fmt.Errorf("permission denied")) + }) + if !strings.Contains(out, "✘ file_read") { + t.Errorf("expected tool name with X mark, got %q", out) + } + if !strings.Contains(out, "permission denied") { + t.Errorf("expected error message, got %q", out) + } +} + func TestFormatDuration(t *testing.T) { tests := []struct { dur time.Duration diff --git a/internal/telemetry/exporter_test.go b/internal/telemetry/exporter_test.go new file mode 100644 index 0000000..64f8573 --- /dev/null +++ b/internal/telemetry/exporter_test.go @@ -0,0 +1,76 @@ +package telemetry + +import ( + "context" + "testing" + + "go.opentelemetry.io/otel/sdk/resource" +) + +func TestNewStdoutTraceExporter(t *testing.T) { + exp, err := newStdoutTraceExporter() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if exp == nil { + t.Error("expected non-nil exporter") + } +} + +func TestNewStdoutMetricExporter(t *testing.T) { + exp, err := newStdoutMetricExporter() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if exp == nil { + t.Error("expected non-nil exporter") + } +} + +func TestInitConsoleProviders(t *testing.T) { + tracerProvider = nil + meterProvider = nil + shutdownFuncs = nil + defer func() { + for _, fn := range shutdownFuncs { + _ = fn(context.Background()) + } + tracerProvider = nil + meterProvider = nil + shutdownFuncs = nil + }() + + initConsoleProviders(resource.Default()) + if tracerProvider == nil { + t.Error("expected tracerProvider to be set after initConsoleProviders") + } + if meterProvider == nil { + t.Error("expected meterProvider to be set after initConsoleProviders") + } + if len(shutdownFuncs) != 2 { + t.Errorf("expected 2 shutdown funcs, got %d", len(shutdownFuncs)) + } +} + +func TestInitOTLPProviders_InvalidEndpoint(t *testing.T) { + tracerProvider = nil + meterProvider = nil + shutdownFuncs = nil + defer func() { + for _, fn := range shutdownFuncs { + _ = fn(context.Background()) + } + tracerProvider = nil + meterProvider = nil + shutdownFuncs = nil + }() + + cfg := Config{ + Exporter: "otlp", + OTLPEndpoint: "localhost:0", + } + initOTLPProviders(context.Background(), resource.Default(), cfg) + if tracerProvider == nil { + t.Error("expected tracerProvider to be set (OTLP exporter creation is lazy)") + } +} diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go index c678964..a028527 100644 --- a/internal/telemetry/metrics_test.go +++ b/internal/telemetry/metrics_test.go @@ -24,7 +24,39 @@ func TestRecordFunctions_DisabledTelemetry(t *testing.T) { } func TestCheckMetricErr(t *testing.T) { - // Should not panic checkMetricErr(nil) checkMetricErr(fmt.Errorf("some error")) } + +func TestRecordFunctions_EnabledTelemetry(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + + RecordReviewDuration(ctx, 5*time.Second) + RecordFilesReviewed(ctx, 10) + RecordCommentsGenerated(ctx, 3) + RecordLLMRequest(ctx, "gpt-4", 2*time.Second, 1000, "ok") + RecordLLMRequest(ctx, "gpt-4", 1*time.Second, 0, "error") + RecordToolCall(ctx, "file_read", 100*time.Millisecond, true) + RecordToolCall(ctx, "file_read", 200*time.Millisecond, false) +} + +func TestEnsureMetrics_Idempotent(t *testing.T) { + setupEnabledTelemetry(t) + ensureMetrics() + ensureMetrics() + if mReviewDuration == nil { + t.Error("expected mReviewDuration to be initialized") + } + if mFilesReviewed == nil { + t.Error("expected mFilesReviewed to be initialized") + } +} + +func TestGetMeter(t *testing.T) { + setupEnabledTelemetry(t) + m := getMeter() + if m == nil { + t.Error("expected non-nil meter") + } +} diff --git a/internal/telemetry/provider_test.go b/internal/telemetry/provider_test.go index 1454e54..3895636 100644 --- a/internal/telemetry/provider_test.go +++ b/internal/telemetry/provider_test.go @@ -2,6 +2,7 @@ package telemetry import ( "context" + "os" "testing" ) @@ -50,3 +51,116 @@ func TestContentLogging_Disabled(t *testing.T) { t.Error("expected ContentLogging()=false when telemetry disabled") } } + +func TestContentLogging_EnabledWithEnv(t *testing.T) { + setupEnabledTelemetry(t) + t.Setenv("OCR_CONTENT_LOGGING", "1") + if !ContentLogging() { + t.Error("expected ContentLogging()=true when enabled and env var set") + } +} + +func TestContentLogging_EnabledWithoutEnv(t *testing.T) { + setupEnabledTelemetry(t) + t.Setenv("OCR_CONTENT_LOGGING", "") + os.Unsetenv("OCR_CONTENT_LOGGING") + if ContentLogging() { + t.Error("expected ContentLogging()=false when enabled but env var not set") + } +} + +func TestInit_AlreadyInitialized(t *testing.T) { + initialized = true + shutdownFuncs = nil + defer func() { + initialized = false + shutdownFuncs = nil + }() + + result := Init(context.Background()) + if result { + t.Error("expected false when already initialized with no shutdown funcs") + } +} + +func TestInit_AlreadyInitializedWithShutdowns(t *testing.T) { + initialized = true + shutdownFuncs = []func(context.Context) error{ + func(ctx context.Context) error { return nil }, + } + defer func() { + initialized = false + shutdownFuncs = nil + }() + + result := Init(context.Background()) + if !result { + t.Error("expected true when already initialized with shutdown funcs") + } +} + +func TestInit_DisabledByDefault(t *testing.T) { + initialized = false + shutdownFuncs = nil + tracerProvider = nil + meterProvider = nil + + 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) + } + + defer func() { + initialized = false + shutdownFuncs = nil + tracerProvider = nil + meterProvider = nil + }() + + result := Init(context.Background()) + if result { + t.Error("expected false when telemetry is not enabled via env") + } +} + +func TestInit_EnabledConsole(t *testing.T) { + initialized = false + shutdownFuncs = nil + tracerProvider = nil + meterProvider = nil + + t.Setenv("OCR_ENABLE_TELEMETRY", "1") + envKeys := []string{ + "OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_PROTOCOL", + } + for _, k := range envKeys { + t.Setenv(k, "") + os.Unsetenv(k) + } + + defer func() { + for _, fn := range shutdownFuncs { + _ = fn(context.Background()) + } + initialized = false + shutdownFuncs = nil + tracerProvider = nil + meterProvider = nil + }() + + result := Init(context.Background()) + if !result { + t.Error("expected true when telemetry is enabled") + } + if tracerProvider == nil { + t.Error("expected tracerProvider to be set") + } + if meterProvider == nil { + t.Error("expected meterProvider to be set") + } +} diff --git a/internal/telemetry/shutdown_test.go b/internal/telemetry/shutdown_test.go index a56f999..e8c8dcf 100644 --- a/internal/telemetry/shutdown_test.go +++ b/internal/telemetry/shutdown_test.go @@ -69,3 +69,12 @@ func TestShutdownWithTimeout(t *testing.T) { t.Error("expected shutdown function to be called via ShutdownWithTimeout") } } + +func TestShutdownWithTimeout_Error(t *testing.T) { + shutdownFuncs = []func(context.Context) error{ + func(ctx context.Context) error { return errors.New("shutdown fail") }, + } + defer func() { shutdownFuncs = nil }() + + ShutdownWithTimeout(context.Background(), 5*time.Second) +} diff --git a/internal/telemetry/span_test.go b/internal/telemetry/span_test.go index 033561b..a8ca9d1 100644 --- a/internal/telemetry/span_test.go +++ b/internal/telemetry/span_test.go @@ -1,10 +1,13 @@ package telemetry import ( + "context" + "errors" "fmt" "testing" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" ) func TestAnyToAttr(t *testing.T) { @@ -31,13 +34,107 @@ func TestAnyToAttr(t *testing.T) { } } +func TestStartSpan_Enabled(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + newCtx, span := StartSpan(ctx, "test.span") + defer span.End() + if newCtx == nil { + t.Error("expected non-nil context") + } + if !span.SpanContext().IsValid() { + t.Error("expected valid span context when telemetry is enabled") + } +} + +func TestStartSpan_Disabled(t *testing.T) { + initialized = false + shutdownFuncs = nil + defer func() { initialized = false }() + + ctx := context.Background() + newCtx, span := StartSpan(ctx, "test.span") + if newCtx != ctx { + t.Error("expected same context when disabled") + } + _ = span +} + +func TestStartSpan_WithOptions(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + _, span := StartSpan(ctx, "test.span", trace.WithAttributes(attribute.String("key", "val"))) + defer span.End() + if !span.SpanContext().IsValid() { + t.Error("expected valid span") + } +} + +func TestEndSpan_NoError(t *testing.T) { + setupEnabledTelemetry(t) + _, span := StartSpan(context.Background(), "test.span") + EndSpan(span, nil) +} + +func TestEndSpan_WithError(t *testing.T) { + setupEnabledTelemetry(t) + _, span := StartSpan(context.Background(), "test.span") + EndSpan(span, errors.New("something went wrong")) +} + +func TestStartToolSpan_Enabled(t *testing.T) { + setupEnabledTelemetry(t) + ctx := context.Background() + newCtx, span := StartToolSpan(ctx, "file_read") + defer span.End() + if newCtx == nil { + t.Error("expected non-nil context") + } + if !span.SpanContext().IsValid() { + t.Error("expected valid span for tool") + } +} + +func TestGetTracer(t *testing.T) { + setupEnabledTelemetry(t) + tracer := getTracer() + if tracer == nil { + t.Error("expected non-nil tracer") + } +} + +func TestSetAttr_AllTypes(t *testing.T) { + setupEnabledTelemetry(t) + _, span := StartSpan(context.Background(), "test.setattr") + defer span.End() + + SetAttr(span, "str", "hello") + SetAttr(span, "int", 42) + SetAttr(span, "int64", int64(100)) + SetAttr(span, "bool", true) + SetAttr(span, "float64", 3.14) + SetAttr(span, "default", []int{1, 2}) +} + func TestSetAttr_NilSpan(t *testing.T) { - // Should not panic SetAttr(nil, "key", "value") } +func TestRecordToolResult_Success(t *testing.T) { + setupEnabledTelemetry(t) + _, span := StartSpan(context.Background(), "test.tool") + RecordToolResult(span, "file_read", 123, nil) + span.End() +} + +func TestRecordToolResult_Error(t *testing.T) { + setupEnabledTelemetry(t) + _, span := StartSpan(context.Background(), "test.tool") + RecordToolResult(span, "file_read", 456, errors.New("read failed")) + span.End() +} + func TestRecordToolResult_NilSpan(t *testing.T) { - // Should not panic RecordToolResult(nil, "tool", 100, nil) RecordToolResult(nil, "tool", 100, fmt.Errorf("err")) }