Skip to content

chore(observability): add tests for previously-uncovered helpers#2511

Open
mastermanas805 wants to merge 2 commits intosupabase:masterfrom
mastermanas805:test/observability-helpers-coverage
Open

chore(observability): add tests for previously-uncovered helpers#2511
mastermanas805 wants to merge 2 commits intosupabase:masterfrom
mastermanas805:test/observability-helpers-coverage

Conversation

@mastermanas805
Copy link
Copy Markdown

@mastermanas805 mastermanas805 commented Apr 26, 2026

What

Adds unit tests across internal/observability/ for helpers that had no coverage. No production source files are touched.

Coverage delta

Package Before After
internal/observability 17.7% 81.4%

(+63.7 percentage points, 7 test files, ~480 LOC of additive tests.)

What's covered

Synchronous setup paths

  • enablePrometheusMetrics — starts on a free port, shuts down on context cancellation
  • enableOpenTelemetryMetrics — both grpc and http/protobuf exporter protocols (success path) plus the unsupported-protocol error path
  • enableOpenTelemetryTracing — both grpc and http/protobuf exporter protocols (success path) plus the unsupported-protocol error path
  • ConfigureProfiler — disabled config no-op; enabled config starts pprof server on a free port and shuts down on context cancel
  • ConfigureMetrics and ConfigureTracingnil context guards (panic-with-message), disabled-config branches
  • ConfigureLogging — covers the file-output branch, the configured-Fields branch, and the LOG_SQL_ALL branch (resets loggingOnce from inside the package's own tests so the branches actually run)

Pure helpers

  • Meter, Tracer, ObtainMetricCounter, openTelemetryResource
  • cleanup.go::WaitForCleanup
  • request-logger.go: NewLogEntry, GetLogEntry (fallback + attached), LogEntrySetField, LogEntrySetFields, logEntry.Panic
  • request-tracing.go: getChiRoutePattern (with and without chi route context), traceChiRoutesSafely, traceChiRouteURLParamsSafely, addMetricAttributes, interceptingResponseWriter (delegation of WriteHeader, Write, Header), countStatusCodesSafely (nil-counter no-op + real-counter with chi route context), RequestTracing() middleware (invokes next handler)
  • profiler.go::ProfilerHandler.ServeHTTP — every documented pprof path plus 404 fallthrough
  • logging.go::NewCustomFormatter and setPopLogger (across SQL log modes)

Error / nil-context branches

  • enableOpenTelemetryMetrics and enableOpenTelemetryTracing reject unsupported exporter protocols with a typed error
  • ConfigureMetrics(nil, ...) and ConfigureTracing(nil, ...) panic with the documented message

How to verify (for reviewers)

go test ./internal/observability/ -count=1 -v
go test -cover ./internal/observability/

The -cover line should report 81.4%.

Verification I ran locally

  • go test ./internal/observability/ -count=1 -v — all tests green
  • go test ./... -count=1 -p 1 -race — full module suite green
  • gofmt -l on all touched files — empty
  • go vet ./... — empty

What's deliberately out of scope

The remaining ~18.6% gap is in code paths that need real infrastructure beyond a test process can stand up reliably:

  • setPopLogger's closure body — runs only when the gobuffalo/pop ORM logs (requires a live *pop.Connection, i.e. a DB)
  • The defer/recover branches in traceChiRoutesSafely and traceChiRouteURLParamsSafely — defensive code; reaching them requires a malformed request that itself panics during recovery and would be brittle to assert against
  • ConfigureMetrics / ConfigureTracing / ConfigureLogging paths past the sync.Once first call — covered by directly testing the underlying enable* functions instead

Notes

  • No production code was touched.
  • Each new test file follows the repo convention <source>_test.go matching <source>.go.
  • Style matches surrounding tests: table-driven slice-of-cases for input variations, scenario-based t.Run blocks for tests with distinct setups, require.* for hard assertions.
  • metrics_test.go::TestConfigureMetricsDisabled and logging_test.go::TestConfigureLoggingWithFile reset the package-level metricsOnce / loggingOnce so the branches under test actually run. The reset is contained to the test files and never touches production callers.
  • freeLocalPort test helper finds a free TCP port on 127.0.0.1 for tests that need to start real listeners; it lives in profiler_test.go and is shared with metrics_test.go (same package).

internal/utilities/strings.go, context.go, and io.go held small pure-ish
helpers but had no test coverage. Add focused unit tests so behaviour
changes get caught: round-trip tests for StringValue and StringPtr; a
context-key formatting check plus set/read/replace/derive cases for
WithRequestID and GetRequestID; cancellation and completion cases for
WaitForCleanup; and behaviour assertions for SafeClose covering the
happy path, the error path, and io.NopCloser.

Coverage for the package goes from 50.8% to 62.4% with no behaviour
changes.

Signed-off-by: Manas Srivastava <mastermanas805@gmail.com>
@mastermanas805 mastermanas805 force-pushed the test/observability-helpers-coverage branch from e7265dd to b37633c Compare April 26, 2026 07:09
internal/observability had 17.7% statement coverage. Most production
helpers were untested, including the pprof handler, the chi-aware
tracing helpers, the intercepting response writer, the request-logger
helpers, the cleanup wrapper, and the disabled-config and error
branches of the configure-* setup functions.

Adds focused, infra-free unit tests across cleanup, profiler, metrics,
tracing, request-tracing, and request-logger files. Tests for
enable* success paths cancel the supplied context immediately and
let the cleanup goroutines wind down rather than asserting on
listener state.

Coverage for the package goes from 17.7% to 81.4% with no behaviour
changes. The remaining gap is in functions that need a live
*pop.Connection (the gobuffalo ORM), defensive panic-recover branches,
and code paths past the package-level sync.Once first call; those
need integration-style fixtures that this PR deliberately doesn't
introduce.

Signed-off-by: Manas Srivastava <mastermanas805@gmail.com>
@mastermanas805 mastermanas805 force-pushed the test/observability-helpers-coverage branch from b37633c to fc7648d Compare April 26, 2026 07:12
@mastermanas805 mastermanas805 changed the title test(observability): add tests for previously-uncovered helpers chore(observability): add tests for previously-uncovered helpers Apr 26, 2026
@mastermanas805 mastermanas805 marked this pull request as ready for review April 26, 2026 07:14
@mastermanas805 mastermanas805 requested a review from a team as a code owner April 26, 2026 07:14
@mastermanas805
Copy link
Copy Markdown
Author

@fadymak requesting your review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant