From 793bbc6a61b5f7c1f987828a29f5c47a951611ec Mon Sep 17 00:00:00 2001 From: kite Date: Sat, 27 Jun 2026 10:43:45 +0800 Subject: [PATCH] test: improve coverage for telemetry package and exclude extensions from build targets Add comprehensive tests for events, metrics, provider, shutdown, span, and exporter in the telemetry package. Update Makefile to exclude the extensions directory from test, fmt, vet, and check targets. --- Makefile | 12 ++- internal/telemetry/events_test.go | 152 ++++++++++++++++++++++++++++ internal/telemetry/exporter_test.go | 76 ++++++++++++++ internal/telemetry/metrics_test.go | 34 ++++++- internal/telemetry/provider_test.go | 114 +++++++++++++++++++++ internal/telemetry/shutdown_test.go | 9 ++ internal/telemetry/span_test.go | 101 +++++++++++++++++- 7 files changed, 490 insertions(+), 8 deletions(-) create mode 100644 internal/telemetry/exporter_test.go 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")) }