Skip to content

[test-improver] Improve tests for config tracing package#5076

Merged
lpcox merged 2 commits into
mainfrom
test-improver/config-tracing-6e08b9caa01f313b
May 4, 2026
Merged

[test-improver] Improve tests for config tracing package#5076
lpcox merged 2 commits into
mainfrom
test-improver/config-tracing-6e08b9caa01f313b

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot commented May 4, 2026

Test Improvements: config_tracing_test.go

File Analyzed

  • Test File: internal/config/config_tracing_test.go
  • Package: internal/config
  • Lines Added: +123 lines (11 new test functions)

Improvements Made

1. Increased Coverage

  • GetSampleRate on nil receiver — prevented panic, returns default (was untested)
  • GetSampleRate with nil SampleRate field — returns default (was untested)
  • GetSampleRate with explicit 0.0 — distinct from unset, returns 0.0 (was untested)
  • applyDefaults when cfg.Gateway is nil — no panic (was untested)
  • applyDefaults when cfg.Gateway.Tracing is nil — no panic (was untested)
  • applyDefaults when ServiceName is already set — preserves custom value (was untested)
  • StdinConverter full field mapping from StdinOpenTelemetryConfigTracingConfig
  • StdinConverter default ServiceName applied when stdin value is empty
  • StdinConverter early-return when stdinCfg.Gateway is nil
  • StdinConverter early-return when stdinCfg.Gateway.OpenTelemetry is nil

Previous Coverage (config_tracing.go functions):

  • GetSampleRate: 66.7%
  • init: 53.8%

New Coverage:

  • GetSampleRate: 100.0%
  • init: 92.3%

2. Better Testing Patterns

  • ✅ Each new test targets exactly one behavior with a descriptive name
  • require.NotPanics used for nil-safety assertions
  • require.NotNil guards before accessing pointer fields in stdin converter tests
  • ✅ Test names follow TestType_Scenario convention

Test Execution

All new and existing tests pass (pre-existing TestFetchAndFixSchema_NetworkError failure is unrelated — network-dependent test that fails in CI sandbox):

=== RUN   TestGetSampleRate_NilReceiver
--- PASS: TestGetSampleRate_NilReceiver (0.00s)
=== RUN   TestGetSampleRate_NilSampleRate
--- PASS: TestGetSampleRate_NilSampleRate (0.00s)
=== RUN   TestGetSampleRate_ZeroExplicitRate
--- PASS: TestGetSampleRate_ZeroExplicitRate (0.00s)
=== RUN   TestTracingDefaults_ServiceNameAlreadySet
--- PASS: TestTracingDefaults_ServiceNameAlreadySet (0.00s)
=== RUN   TestTracingDefaults_NilGateway
--- PASS: TestTracingDefaults_NilGateway (0.00s)
=== RUN   TestTracingDefaults_NilTracing
--- PASS: TestTracingDefaults_NilTracing (0.00s)
=== RUN   TestStdinConverter_OTelConfig
--- PASS: TestStdinConverter_OTelConfig (0.00s)
=== RUN   TestStdinConverter_OTelConfig_DefaultsServiceName
--- PASS: TestStdinConverter_OTelConfig_DefaultsServiceName (0.00s)
=== RUN   TestStdinConverter_OTelConfig_NilGateway
--- PASS: TestStdinConverter_OTelConfig_NilGateway (0.00s)
=== RUN   TestStdinConverter_OTelConfig_NilOpenTelemetry
--- PASS: TestStdinConverter_OTelConfig_NilOpenTelemetry (0.00s)

Why These Changes?

config_tracing_test.go had clear coverage gaps: GetSampleRate was only tested with a non-nil pointer (missing nil receiver and nil-field cases), and the init-registered callbacks for DefaultsSetter and StdinConverter had uncovered early-return paths and the full converter mapping path was untested. These are important paths — nil-safety on a value receiver method and correct behavior of the stdin JSON config pipeline — that could regress silently without tests.


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:

  • invalidhostthatdoesnotexist12345.com

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

network:
  allowed:
    - defaults
    - "invalidhostthatdoesnotexist12345.com"

See Network Configuration for more information.

Generated by Test Improver · ● 2M ·

- Add GetSampleRate tests for nil receiver and nil SampleRate (100% coverage, was 66.7%)
- Add GetSampleRate test for explicit zero rate
- Add tests for init-registered DefaultsSetter (nil Gateway, nil Tracing, already-set ServiceName)
- Add tests for init-registered StdinConverter (full field mapping, default ServiceName, nil Gateway, nil OpenTelemetry)
- init function coverage improved from 53.8% to 92.3%
- config_tracing.go overall coverage: GetSampleRate 100% (was 66.7%), init 92.3% (was 53.8%)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox lpcox marked this pull request as ready for review May 4, 2026 16:32
Copilot AI review requested due to automatic review settings May 4, 2026 16:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR expands unit test coverage for the internal/config tracing configuration feature, focusing on nil-safety and stdin (JSON) conversion behavior around OpenTelemetry tracing settings.

Changes:

  • Adds GetSampleRate coverage for nil receiver / nil SampleRate / explicit 0.0 cases.
  • Adds coverage for tracing defaults behavior, including preserving an explicitly-set ServiceName.
  • Adds coverage for the init-registered stdin converter mapping and its early-return paths.
Show a summary per file
File Description
internal/config/config_tracing_test.go Adds targeted tests for tracing sample-rate behavior, default application, and stdin converter field mapping / nil-safety.

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

Comment thread internal/config/config_tracing_test.go Outdated
Comment on lines +326 to +331
// TestTracingDefaults_NilGateway verifies that applyDefaults does not panic when
// cfg.Gateway is nil (tracing is not configured).
func TestTracingDefaults_NilGateway(t *testing.T) {
cfg := &Config{}
require.NotPanics(t, func() { applyDefaults(cfg) },
"applyDefaults must not panic when Gateway is nil")
Comment on lines +281 to +287
// TestGetSampleRate_NilReceiver verifies that calling GetSampleRate on a nil
// *TracingConfig returns the default sample rate rather than panicking.
func TestGetSampleRate_NilReceiver(t *testing.T) {
var cfg *TracingConfig
assert.InDelta(t, DefaultTracingSampleRate, cfg.GetSampleRate(), 0.001,
"nil receiver must return DefaultTracingSampleRate")
}
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 4, 2026

@copilot address the review feedback and fix the ci failures

Copy link
Copy Markdown
Contributor

Copilot AI commented May 4, 2026

@copilot address the review feedback and fix the ci failures

Done in c4e578a:

  • CI lint failure: Fixed formatting in internal/server/circuit_breaker.go, circuit_breaker_test.go, and internal/strutil/format_duration_test.go (pre-existing gofmt issues in those files).
  • Review feedback on TestTracingDefaults_NilGateway: Updated to call cfg.EnsureGatewayDefaults() instead of applyDefaults directly — this reflects the real call pattern used by production loaders and also verifies that EnsureGatewayDefaults initialises the Gateway field when nil.

Copilot finished work on behalf of lpcox May 4, 2026 16:42
Copilot AI requested a review from lpcox May 4, 2026 16:42
@lpcox lpcox merged commit 5c67427 into main May 4, 2026
16 checks passed
@lpcox lpcox deleted the test-improver/config-tracing-6e08b9caa01f313b branch May 4, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants