[test-improver] Improve tests for cmd tracing helpers

[github-actions]

Test Improvements: tracing_helpers_test.go

File Analyzed

• Test File: internal/cmd/tracing_helpers_test.go
• Package: internal/cmd
• Lines of Code: 158 → 183 (+25 lines)

Improvements Made

1. New Test Coverage

• ✅ Added TestLogTracingWarnf — covers logTracingWarnf which had 0% coverage; verifies the function prefixes output with "Warning: " using the standard log package

2. Corrected Misleading Test

• ✅ Renamed "unreachable endpoint falls back to noop provider and emits warning" → "configured endpoint creates SDK provider without warning" to accurately reflect OTel HTTP OTLP's lazy-connection behavior
• ✅ Removed incorrect comment claiming otlptracehttp.New "performs a dial during provider creation" (HTTP OTLP is lazily connected; construction succeeds for any URL)
• ✅ Removed conditional if warnMsg != "" { assert.Contains(...) } guard — this silently allowed the assertion to be skipped on every run; replaced with unconditional assertions
• ✅ Added assert.True(t, provider.IsEnabled()) — verifies that a configured endpoint produces a real SDK provider (not a noop), which is the correct expectation

3. Expanded SDK Provider Coverage

• ✅ Added "sdk provider shuts down cleanly" subtest to TestShutdownTracingProviderWithTimeout — the existing test only covered the noop provider path; the new subtest creates a real SDK provider and verifies clean shutdown

Coverage Improvement

Function	Before	After
logTracingWarnf	0%	100%
initTracingProviderWithFallback	62.5%	62.5% (error path requires SDK internals to fail)
shutdownTracingProviderWithTimeout	71.4%	71.4% (error path requires Shutdown to return an error)

Note: The remaining uncovered branches in initTracingProviderWithFallback and shutdownTracingProviderWithTimeout are error paths that require the OTel SDK to return errors (e.g., exporter construction or shutdown failure). These cannot be triggered in unit tests without mocking the SDK — the HTTP OTLP exporter is lazy and always succeeds at construction time.

Test Execution

All tests pass:

=== RUN   TestInitTracingProviderWithFallback
=== RUN   TestInitTracingProviderWithFallback/nil_config_returns_noop_provider_without_error
=== RUN   TestInitTracingProviderWithFallback/valid_config_with_no_endpoint_returns_noop_provider_without_warning
=== RUN   TestInitTracingProviderWithFallback/configured_endpoint_creates_SDK_provider_without_warning
--- PASS: TestInitTracingProviderWithFallback (0.00s)
=== RUN   TestShutdownTracingProviderWithTimeout
=== RUN   TestShutdownTracingProviderWithTimeout/noop_provider_shuts_down_cleanly
=== RUN   TestShutdownTracingProviderWithTimeout/sdk_provider_shuts_down_cleanly
--- PASS: TestShutdownTracingProviderWithTimeout (0.00s)
=== RUN   TestLogTracingWarnf
--- PASS: TestLogTracingWarnf (0.00s)
ok  github.com/github/gh-aw-mcpg/internal/cmd  0.012s

Why These Changes?

tracing_helpers_test.go was selected because logTracingWarnf was at 0% coverage despite being a simple, directly-testable function. The existing "unreachable endpoint" test also contained a subtle bug — the conditional assertion if warnMsg != "" meant the assertion never ran (since HTTP OTLP never fails lazily), so the test passed vacuously. This is a real correctness issue: the test appeared to validate error-fallback behavior but was silently a no-op. The fix removes the conditional and accurately documents the actual behavior.

---

Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• index.crates.io

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "index.crates.io"

See Network Configuration for more information.

Generated by Test Improver · 1.6K AIC · ⊞ 29.5K · ◷

[Copilot]

Pull request overview

Improves unit test coverage and correctness for the internal/cmd tracing helpers, including adding direct coverage for logTracingWarnf and adjusting tracing-provider tests to reflect OTLP/HTTP exporter lazy-construction behavior.

Changes:

• Added TestLogTracingWarnf to cover the Warning: prefix behavior emitted via the standard log package.
• Updated the “unreachable endpoint” init-provider test to instead assert SDK provider creation without warnings (reflecting lazy OTLP/HTTP exporter construction).
• Added an SDK-provider shutdown subtest to TestShutdownTracingProviderWithTimeout.

Show a summary per file

File	Description
internal/cmd/tracing_helpers_test.go	Expands and corrects tests for tracing helper initialization, shutdown, and warning logging.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 1/1 changed files
• Comments generated: 2

[lpcox]

@copilot address review feedback

[Copilot]

@copilot address review feedback

Added t.Cleanup(func() { otel.SetTracerProvider(noop.NewTracerProvider()) }) to both subtests that install a real SDK provider as the global OTel tracer provider — the "configured endpoint creates SDK provider without warning" subtest in TestInitTracingProviderWithFallback and the "sdk provider shuts down cleanly" subtest in TestShutdownTracingProviderWithTimeout. This ensures background batcher goroutines are cleaned up and the global is reset to noop regardless of test ordering. Addressed in commit Fix global OTel provider leak in SDK tracing subtests.