From ed89d1f5de3feaeda902428d5c010f735ef05a16 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 23 Jun 2026 00:19:39 +0000 Subject: [PATCH 1/2] Improve tests for cmd tracing helpers - Add TestLogTracingWarnf to cover the previously untested logTracingWarnf function (was 0% coverage, now 100%) - Replace misleading 'unreachable endpoint falls back to noop' test with an accurate test that documents HTTP OTLP's lazy-connection behavior: the exporter construction succeeds even for unreachable endpoints and the provider is a real SDK (non-noop) instance - Add 'sdk provider shuts down cleanly' subtest to TestShutdownTracingProviderWithTimeout to cover the SDK provider code path (previously only the noop path was tested) - Strengthen assertions: remove conditional 'if warnMsg != ""' guard that silently allowed assertions to be skipped; all assertions are now unconditional Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/cmd/tracing_helpers_test.go | 55 ++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/internal/cmd/tracing_helpers_test.go b/internal/cmd/tracing_helpers_test.go index 00de614d9..3e406200a 100644 --- a/internal/cmd/tracing_helpers_test.go +++ b/internal/cmd/tracing_helpers_test.go @@ -1,7 +1,9 @@ package cmd import ( + "bytes" "context" + "log" "testing" "github.com/spf13/cobra" @@ -9,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/github/gh-aw-mcpg/internal/config" + "github.com/github/gh-aw-mcpg/internal/tracing" ) func TestRegisterTracingFlags_DefaultsFromEnv(t *testing.T) { @@ -84,29 +87,23 @@ func TestInitTracingProviderWithFallback(t *testing.T) { assert.False(t, warnCalled, "No warning expected when no endpoint is configured") }) - t.Run("unreachable endpoint falls back to noop provider and emits warning", func(t *testing.T) { - var warnMsg string + t.Run("configured endpoint creates SDK provider without warning", func(t *testing.T) { + // HTTP OTLP exporters are lazily connected: otlptracehttp.New succeeds even + // for unreachable endpoints, so InitProvider does not return an error and the + // warn callback is never invoked. The provider is a real SDK (non-noop) instance. + var warnCalled bool cfg := &config.TracingConfig{ - // Use a bogus address that will immediately fail during startup. - // InitProvider performs a dial during provider creation when an - // endpoint is set, so this exercises the error-fallback branch. Endpoint: "https://127.0.0.1:1/does-not-exist", } provider := initTracingProviderWithFallback( context.Background(), cfg, "tracing init failed: %v", - func(format string, args ...any) { - warnMsg = format - }, + func(format string, args ...any) { warnCalled = true }, ) - // Regardless of whether the provider initialisation fails, - // the fallback must always return a non-nil provider. - require.NotNil(t, provider, "Fallback provider must not be nil") - // If the init failed, the warning callback must have been called. - if warnMsg != "" { - assert.Contains(t, warnMsg, "tracing init failed") - } + require.NotNil(t, provider, "Provider must not be nil") + assert.False(t, warnCalled, "OTLP exporter construction is lazy; no warning expected") + assert.True(t, provider.IsEnabled(), "Configured endpoint should produce an SDK (non-noop) provider") }) } @@ -129,6 +126,21 @@ func TestShutdownTracingProviderWithTimeout(t *testing.T) { }) assert.False(t, warnCalled, "Shutdown of noop provider should not produce a warning") }) + + t.Run("sdk provider shuts down cleanly", func(t *testing.T) { + // HTTP OTLP exporters are lazy; construction succeeds even for unreachable endpoints. + provider, err := tracing.InitProvider(context.Background(), &config.TracingConfig{ + Endpoint: "http://127.0.0.1:14318", + }) + require.NoError(t, err) + require.True(t, provider.IsEnabled(), "Expected a real SDK provider with a configured endpoint") + + var warnCalled bool + shutdownTracingProviderWithTimeout(provider, func(format string, args ...any) { + warnCalled = true + }) + assert.False(t, warnCalled, "SDK provider with no pending spans should shut down without error") + }) } func TestSetupCommandTracing(t *testing.T) { @@ -156,3 +168,16 @@ func TestSetupCommandTracing(t *testing.T) { assert.False(t, shutdownWarnCalled) }) } + +// TestLogTracingWarnf verifies that logTracingWarnf prefixes the message with +// "Warning: " and writes the formatted string to the default log output. +func TestLogTracingWarnf(t *testing.T) { + var buf bytes.Buffer + oldOutput := log.Writer() + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(oldOutput) }) + + logTracingWarnf("disk cache failed: %s", "permission denied") + + assert.Contains(t, buf.String(), "Warning: disk cache failed: permission denied") +} From 598d9f5cca4b8e7fdcea31ced27dea63d65d63ca Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 23 Jun 2026 20:00:08 +0000 Subject: [PATCH 2/2] Fix global OTel provider leak in SDK tracing subtests --- internal/cmd/tracing_helpers_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/cmd/tracing_helpers_test.go b/internal/cmd/tracing_helpers_test.go index 3e406200a..755ffc76a 100644 --- a/internal/cmd/tracing_helpers_test.go +++ b/internal/cmd/tracing_helpers_test.go @@ -9,6 +9,8 @@ import ( "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/trace/noop" "github.com/github/gh-aw-mcpg/internal/config" "github.com/github/gh-aw-mcpg/internal/tracing" @@ -91,6 +93,9 @@ func TestInitTracingProviderWithFallback(t *testing.T) { // HTTP OTLP exporters are lazily connected: otlptracehttp.New succeeds even // for unreachable endpoints, so InitProvider does not return an error and the // warn callback is never invoked. The provider is a real SDK (non-noop) instance. + // Reset the global OTel provider to noop after the subtest to avoid leaking + // background batcher goroutines and making other tests order-dependent. + t.Cleanup(func() { otel.SetTracerProvider(noop.NewTracerProvider()) }) var warnCalled bool cfg := &config.TracingConfig{ Endpoint: "https://127.0.0.1:1/does-not-exist", @@ -129,6 +134,9 @@ func TestShutdownTracingProviderWithTimeout(t *testing.T) { t.Run("sdk provider shuts down cleanly", func(t *testing.T) { // HTTP OTLP exporters are lazy; construction succeeds even for unreachable endpoints. + // Reset the global OTel provider to noop after the subtest so that a shut-down + // provider is not left as the global, which would make later tests order-dependent. + t.Cleanup(func() { otel.SetTracerProvider(noop.NewTracerProvider()) }) provider, err := tracing.InitProvider(context.Background(), &config.TracingConfig{ Endpoint: "http://127.0.0.1:14318", })