Skip to content

[test] Add tests for logger.formatJSONWithoutFields and isEffectivelyEmpty#9793

Open
github-actions[bot] wants to merge 2 commits into
mainfrom
test-coverage/rpc-format-functions-d9105dde5c0d9c50
Open

[test] Add tests for logger.formatJSONWithoutFields and isEffectivelyEmpty#9793
github-actions[bot] wants to merge 2 commits into
mainfrom
test-coverage/rpc-format-functions-d9105dde5c0d9c50

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Test Coverage Improvement: logger.formatJSONWithoutFields

Function Analyzed

  • Package: internal/logger
  • Function: formatJSONWithoutFields and isEffectivelyEmpty
  • File: internal/logger/rpc_format.go
  • Previous Coverage: 90.0% (formatJSONWithoutFields), indirect-only (isEffectivelyEmpty)
  • New Coverage: 90.0% (remaining 10% is a json.Marshal error branch that is unreachable by design)
  • Complexity: Medium

Why This Function?

The functions formatJSONWithoutFields and isEffectivelyEmpty in rpc_format.go had no dedicated test file. They were only exercised indirectly through formatRPCMessageMarkdown tests in rpc_logger_test.go. Direct tests improve:

  • Test clarity: failures pinpoint the exact function rather than surfacing through higher-level logging tests
  • Regression protection: edge cases like nil field lists, JSON arrays (treated as invalid), prettified-input compaction, and params: false/\"\" (zero-like but non-nil) are now explicitly documented and verified
  • Coverage justification: the sole uncovered line is return jsonStr, false, false inside if err := json.Marshal(data); err != nil — this branch is unreachable because map[string]interface{} values are always JSON-serializable after successful json.Unmarshal

Tests Added

  • TestFormatJSONWithoutFields_Direct — 10 table-driven cases:
    • Invalid JSON (plain text, empty string, JSON arrays, truncated JSON)
    • Field removal edge cases (nil field list, empty list, nonexistent fields, remove-all)
    • Params-null empty detection
    • Nested object preservation after removal
  • TestIsEffectivelyEmpty_Direct — 8 table-driven cases:
    • Nil and empty maps
    • params: null (empty), params: \"\" / params: false (non-nil → not empty)
    • Multi-field map with params: null (not empty)
  • TestFormatJSONWithoutFields_OutputIsCompact — verifies prettified JSON input is always compacted to a single line

Coverage Report

Before: formatJSONWithoutFields 90.0% (indirect tests only)
After:  formatJSONWithoutFields 90.0% (direct tests; unreachable branch documented)
        isEffectivelyEmpty 100% with direct coverage

Note: The only uncovered branch (json.Marshal error at line 142) cannot be triggered for map[string]interface{} produced by a successful json.Unmarshal. This is intentional defensive code added to satisfy gosec G104.

Test Execution

All tests pass:

ok  github.com/github/gh-aw-mcpg/internal/logger  0.358s

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 · 286.6 AIC · ⊞ 7K ·

Add a dedicated rpc_format_test.go file that directly tests the
formatJSONWithoutFields and isEffectivelyEmpty functions in the
logger package.

These functions were previously only tested indirectly through the
RPC logging infrastructure (formatRPCMessageMarkdown). The new tests:

- TestFormatJSONWithoutFields_Direct: 10 cases covering invalid JSON
  (plain text, empty string, JSON arrays, truncated JSON), field removal
  edge cases (nil/empty lists, nonexistent fields, remove-all), and
  empty/non-empty detection of params:null patterns
- TestIsEffectivelyEmpty_Direct: 8 cases covering nil/empty maps,
  params:null, params with various zero-like values (string, bool),
  and multi-field cases
- TestFormatJSONWithoutFields_OutputIsCompact: verifies prettified
  JSON input is always compacted to a single line with no extra
  whitespace

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

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

Adds direct logger formatting tests, though much overlaps existing coverage and valid JSON output is not fully asserted.

Changes:

  • Adds table-driven JSON formatting and empty-state tests.
  • Tests compact output for formatted JSON.
Show a summary per file
File Description
internal/logger/rpc_format_test.go Adds direct tests for logger JSON formatting helpers.

Review details

Tip

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

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

Comment thread internal/logger/rpc_format_test.go Outdated
Comment on lines +105 to +109
if !tt.wantIsValidJSON {
assert.Equal(t, tt.wantFormatted, gotFormatted, "formatted string mismatch for invalid JSON")
} else {
// For valid JSON output, verify it's still valid JSON and not just whitespace
assert.NotEmpty(t, gotFormatted, "valid JSON result should not be empty string")
Comment thread internal/logger/rpc_format_test.go Outdated
Comment on lines +9 to +11
// TestFormatJSONWithoutFields_Direct tests formatJSONWithoutFields directly,
// independently of the RPC logging infrastructure.
func TestFormatJSONWithoutFields_Direct(t *testing.T) {
@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — gVisor

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: gVisor (runsc) kernel-level isolation

Part Surface Op Result Expected Status
A MCP reads data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) gateway refused — Error [-32602]: unknown tool for all 7 BLOCKED
C CLI reads data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) refused — gh: GH_TOKEN not set BLOCKED
E CLI GraphQL mutations (addReaction/addStar/createIssue) refused — gh: GH_TOKEN not set BLOCKED

Gateway tool list: 22 read-only tools exposed; all write tools absent (gateway-filtered).

Overall: PASS

References: §29851616859

🔒 mcpg read-only stress (gVisor runtime) by Read-Only Stress: gVisor runtime

@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — default

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: default AWF (normal container isolation)

Part Surface Op Result Expected Status
A MCP reads (list_issues/list_prs/get_file/list_commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) Error [-32602]: unknown tool BLOCKED
C CLI reads (list_issues/get_file via github CLI) data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) gh: GH_TOKEN not set (unauthenticated) BLOCKED
E CLI GraphQL mutations (addReaction/addStar/createIssue) gh: GH_TOKEN not set (unauthenticated) BLOCKED

Overall: PASS

Part B: gateway returns Error [-32602]: unknown tool for all write tools — write tools absent from gateway-filtered list, enforcing read-only at MCP layer.
Parts D & E: gh CLI unauthenticated (no GH_TOKEN); all REST/GraphQL writes rejected.

References: §29851616717

🔒 mcpg read-only stress (default AWF runtime) by Read-Only Stress: default runtime

@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — docker-sbx

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: docker-sbx (KVM-isolated microVM)

Part Surface Op Result Expected Status
A MCP reads (issues/PRs/file/commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) unknown tool [-32602] BLOCKED
C CLI reads (list_issues, get_file_contents) data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) auth error (GH_TOKEN not set) BLOCKED
E CLI GraphQL mutations (addReaction/addStar/createIssue) auth error (GH_TOKEN not set) BLOCKED

Overall: PASS

Part B: gateway returned MCP error [-32602]: unknown tool for all 7 write tools — write tools absent from gateway-permitted list (gateway enforcement).
Parts D+E: gh CLI unauthenticated; all REST/GraphQL write attempts rejected before reaching GitHub API.

References: §29851616749

🔒 mcpg read-only stress (docker-sbx runtime) by Read-Only Stress: docker-sbx runtime

@lpcox

lpcox commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

@copilot address review feedback

Copilot AI commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@copilot address review feedback

Addressed in 46b23b8.

I consolidated the duplicated helper coverage into the existing direct tests, removed internal/logger/rpc_format_test.go, and updated TestFormatJSONWithoutFields to assert expected JSON for valid cases with assert.JSONEq (plus compact-output check). I also merged the genuinely new isEffectivelyEmpty edge cases into TestIsEffectivelyEmpty.

Copilot finished work on behalf of lpcox July 21, 2026 22:51
Copilot AI requested a review from lpcox July 21, 2026 22:51
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