Skip to content

[test] Add tests for logger.SlogHandler.Handle via attrKeyString helper - #8730

Merged
lpcox merged 2 commits into
mainfrom
add-tests-slog-handle-coverage-6724c18a6f00a07e
Jul 5, 2026
Merged

[test] Add tests for logger.SlogHandler.Handle via attrKeyString helper#8730
lpcox merged 2 commits into
mainfrom
add-tests-slog-handle-coverage-6724c18a6f00a07e

Conversation

@github-actions

@github-actions github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Function Analyzed

  • Package: internal/logger
  • Function: SlogHandler.Handle
  • Previous Coverage: 95.2%
  • New Coverage: 100.0%
  • Complexity: Medium (attribute key conversion, branch on key type)

Why This Function?

SlogHandler.Handle had an unreachable defensive branch: the inline fmt.Sprint fallback for non-string attribute keys. Since the slog API always provides string keys (via a.Key), this branch could never be exercised through the public API.

The fix extracts the key-conversion logic into a named helper attrKeyString(v any) string, which:

  1. Makes the defensive fallback independently testable
  2. Brings Handle from 95.2% to 100% coverage
  3. Adds an explicit contract test for the string-vs-fallback behaviour

Changes

internal/logger/slog_adapter.go

  • Extracted inline key conversion from Handle() into new package-level helper attrKeyString(v any) string
  • Handle() now calls attrKeyString(attrs[i]) instead of inlining the type assertion

internal/logger/slog_adapter_test.go

  • Updated TestSlogHandler_Handle_NonStringKeyFallback comment to reflect the refactor
  • Added TestAttrKeyString with 6 table-driven test cases:
    • String value is returned as-is
    • Empty string is returned as-is
    • Integer falls back to fmt.Sprint
    • Boolean falls back to fmt.Sprint
    • nil falls back to fmt.Sprint
    • float64 falls back to fmt.Sprint

Tests Added

  • ✅ Happy path: string key returned unchanged
  • ✅ Edge case: empty string key
  • ✅ Non-string fallback: int, bool, nil, float64
  • ✅ Full branch coverage for both arms of the type assertion

Coverage Report

Before: Handle 95.2%  |  logger package 98.5%
After:  Handle 100.0% |  attrKeyString 100.0%  |  logger package 98.7%
Improvement: Handle +4.8 pp

Test Execution

All tests pass:

--- PASS: TestAttrKeyString/string_value_is_returned_as-is
--- PASS: TestAttrKeyString/empty_string_is_returned_as-is
--- PASS: TestAttrKeyString/integer_falls_back_to_fmt.Sprint
--- PASS: TestAttrKeyString/boolean_falls_back_to_fmt.Sprint
--- PASS: TestAttrKeyString/nil_falls_back_to_fmt.Sprint
--- PASS: TestAttrKeyString/float64_falls_back_to_fmt.Sprint
ok  github.com/github/gh-aw-mcpg/internal/logger  coverage: 98.7% of statements

Generated by Test Coverage Improver
Next run will target the next most complex under-tested function

Warning

Firewall blocked 7 domains

The following domains were blocked by the firewall during workflow execution:

  • awmgmcpg
  • go.opentelemetry.io
  • go.yaml.in
  • golang.org
  • google.golang.org
  • gopkg.in
  • proxy.golang.org

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

network:
  allowed:
    - defaults
    - "awmgmcpg"
    - "go.opentelemetry.io"
    - "go.yaml.in"
    - "golang.org"
    - "google.golang.org"
    - "gopkg.in"
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by Test Coverage Improver · 722 AIC · ⊞ 6.9K ·

Extract the defensive non-string attribute key conversion from SlogHandler.Handle
into a named helper attrKeyString, and add comprehensive table-driven tests for it.

Previously SlogHandler.Handle had 95.2% coverage: the fmt.Sprint fallback
branch for non-string keys (defensive dead code - slog always provides string
keys via a.Key) was unreachable through the slog public API.

By extracting the logic into attrKeyString, the fallback path is now testable
independently of the slog record machinery, bringing Handle to 100% coverage
and adding an explicit contract test for the string-vs-fallback behavior.

Coverage change:
  Handle:        95.2% -> 100.0%
  attrKeyString: (new)  -> 100.0%
  logger package: 98.5% -> 98.7%

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox
lpcox marked this pull request as ready for review July 5, 2026 19:35
Copilot AI review requested due to automatic review settings July 5, 2026 19:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 refactors internal/logger.SlogHandler.Handle to extract attribute-key stringification into a small helper (attrKeyString), enabling direct unit tests for the previously untestable defensive fallback branch and increasing coverage for the Handle path.

Changes:

  • Extracted key conversion logic from SlogHandler.Handle into attrKeyString(v any) string.
  • Updated/added tests to cover both the normal string-key path and the defensive non-string fallback behavior via table-driven cases.
Show a summary per file
File Description
internal/logger/slog_adapter.go Refactors attribute key conversion into attrKeyString and uses it in SlogHandler.Handle.
internal/logger/slog_adapter_test.go Updates the existing test comment and adds TestAttrKeyString table tests to cover fallback behavior.

Review details

Tip

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

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread internal/logger/slog_adapter_test.go Outdated
Comment on lines 811 to 814
// TestSlogHandler_Handle_NonStringKeyFallback tests the defensive non-string key path.
// This exercises the fmt.Sprint fallback for non-string attribute keys.
// In production slog always provides string keys, but the attrKeyString helper
// also handles unexpected types via fmt.Sprint.
func TestSlogHandler_Handle_NonStringKeyFallback(t *testing.T) {
@lpcox

lpcox commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

@copilot address review feedback

Copilot AI commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

@copilot address review feedback

Done — renamed TestSlogHandler_Handle_NonStringKeyFallback to TestSlogHandler_Handle_StringKeyFormatting and updated its leading comment to clarify it tests the normal string-key formatting path (the non-string fallback is already covered by TestAttrKeyString). Commit: b3b3453.

Copilot AI requested a review from lpcox July 5, 2026 19:40
Copilot finished work on behalf of lpcox July 5, 2026 19:40
@lpcox
lpcox merged commit 63d579e into main Jul 5, 2026
23 checks passed
@lpcox
lpcox deleted the add-tests-slog-handle-coverage-6724c18a6f00a07e branch July 5, 2026 19:43
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