Skip to content

Commit 1e210cc

Browse files
authored
[test-improver] Improve tests for logger/rpc_formatter (#4332)
## File Analyzed - **Test File**: `internal/logger/rpc_logger_test.go` - **Package**: `internal/logger` - **Lines Added**: +94 ## Improvements Made ### 1. Increased Coverage for `formatRPCMessage` Added 3 new table-driven test cases covering untested branches in `rpc_formatter.go`: - ✅ **Empty `ServerID`** — verifies no server/direction prefix is added when `serverID == ""`; only the payload size and payload content appear - ✅ **Empty `Payload`** — verifies the payload string is omitted from output when payload is empty - ✅ **Error with no payload** — verifies an error-only response (no payload) formats correctly with `err:` prefix ### 2. Increased Coverage for `formatRPCMessageMarkdown` Added 3 new table-driven test cases covering untested branches: - ✅ **Empty `ServerID`** — verifies no bold server prefix (`**server**→`) is produced - ✅ **Empty `Payload`** — verifies no `\`\`\`json` code block is emitted when payload is empty - ✅ **`tools/call` with non-string `name` in params** — verifies the tool-name extraction guard (checking `toolName, ok := params["name"].(string)`) correctly skips appending a backtick-wrapped name when the value is not a string ### 3. Increased Coverage for `formatJSONWithoutFields` Added 2 new table-driven test cases: - ✅ **Nil `fieldsToRemove` slice** — verifies all fields are preserved when the removal list is nil - ✅ **Non-existent field removal is a no-op** — verifies the function handles attempts to remove fields that aren't in the JSON gracefully ## Test Execution All tests pass: ``` === RUN TestFormatRPCMessage --- PASS: TestFormatRPCMessage (0.00s) --- PASS: TestFormatRPCMessage/outbound_request --- PASS: TestFormatRPCMessage/inbound_response_with_error --- PASS: TestFormatRPCMessage/client_request --- PASS: TestFormatRPCMessage/empty_server_ID_omits_server_prefix --- PASS: TestFormatRPCMessage/empty_payload_omits_payload_from_output --- PASS: TestFormatRPCMessage/response_with_error_and_no_payload === RUN TestFormatRPCMessageMarkdown --- PASS: TestFormatRPCMessageMarkdown (0.00s) ...10 subtests all PASS... === RUN TestFormatJSONWithoutFields --- PASS: TestFormatJSONWithoutFields (0.00s) ...8 subtests all PASS... ok github.com/github/gh-aw-mcpg/internal/logger 0.006s ``` ## Why These Changes? `rpc_formatter.go` contains the core text/markdown formatting logic for all RPC message logging. The existing tests covered the "happy path" scenarios (non-empty server, non-empty payload, valid JSON). However, several defensive branches in the code — guarding against empty `ServerID`, empty `Payload`, and non-string type assertions — had zero test coverage. These new tests exercise those branches directly, documenting the expected behavior and guarding against regressions. --- *Generated by Test Improver Workflow* *Focuses on better patterns, increased coverage, and more stable tests* > Generated by [Test Improver](https://github.com/github/gh-aw-mcpg/actions/runs/24776282832/agentic_workflow) · ● 6.3M · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-improver%22&type=pullrequests) <!-- gh-aw-agentic-workflow: Test Improver, engine: copilot, model: auto, id: 24776282832, workflow_id: test-improver, run: https://github.com/github/gh-aw-mcpg/actions/runs/24776282832 --> <!-- gh-aw-workflow-id: test-improver -->
2 parents bd075b0 + f328f6b commit 1e210cc

1 file changed

Lines changed: 94 additions & 0 deletions

File tree

internal/logger/rpc_logger_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,42 @@ func TestFormatRPCMessage(t *testing.T) {
5555
},
5656
want: []string{"client←tools/call", "200b"},
5757
},
58+
{
59+
name: "empty server ID omits server prefix",
60+
info: &RPCMessageInfo{
61+
Direction: RPCDirectionOutbound,
62+
MessageType: RPCMessageRequest,
63+
ServerID: "",
64+
Method: "tools/list",
65+
PayloadSize: 30,
66+
Payload: `{"method":"tools/list"}`,
67+
},
68+
want: []string{"30b", `{"method":"tools/list"}`},
69+
},
70+
{
71+
name: "empty payload omits payload from output",
72+
info: &RPCMessageInfo{
73+
Direction: RPCDirectionOutbound,
74+
MessageType: RPCMessageRequest,
75+
ServerID: "github",
76+
Method: "tools/list",
77+
PayloadSize: 0,
78+
Payload: "",
79+
},
80+
want: []string{"github→tools/list", "0b"},
81+
},
82+
{
83+
name: "response with error and no payload",
84+
info: &RPCMessageInfo{
85+
Direction: RPCDirectionInbound,
86+
MessageType: RPCMessageResponse,
87+
ServerID: "backend",
88+
PayloadSize: 0,
89+
Payload: "",
90+
Error: "connection reset",
91+
},
92+
want: []string{"backend←resp", "0b", "err:connection reset"},
93+
},
5894
}
5995

6096
for _, tt := range tests {
@@ -177,6 +213,46 @@ func TestFormatRPCMessageMarkdown(t *testing.T) {
177213
want: []string{"**github**→`tools/call`", "```json", `"params"`},
178214
notWant: []string{`"jsonrpc"`, `"method"`},
179215
},
216+
{
217+
name: "tools/call with non-string name in params does not append tool name",
218+
info: &RPCMessageInfo{
219+
Direction: RPCDirectionOutbound,
220+
MessageType: RPCMessageRequest,
221+
ServerID: "github",
222+
Method: "tools/call",
223+
PayloadSize: 60,
224+
Payload: `{"jsonrpc":"2.0","method":"tools/call","params":{"name":42,"arguments":{}}}`,
225+
},
226+
// name is not a string so tool name extraction should not add backtick-wrapped name
227+
want: []string{"**github**→`tools/call`"},
228+
notWant: []string{"**github**→`tools/call` `"},
229+
},
230+
{
231+
name: "empty server ID produces no server prefix",
232+
info: &RPCMessageInfo{
233+
Direction: RPCDirectionOutbound,
234+
MessageType: RPCMessageRequest,
235+
ServerID: "",
236+
Method: "tools/list",
237+
PayloadSize: 40,
238+
Payload: `{"jsonrpc":"2.0","method":"tools/list","params":{"key":"val"}}`,
239+
},
240+
want: []string{"```json", `"params"`},
241+
notWant: []string{"**"},
242+
},
243+
{
244+
name: "empty payload produces no code block",
245+
info: &RPCMessageInfo{
246+
Direction: RPCDirectionOutbound,
247+
MessageType: RPCMessageRequest,
248+
ServerID: "github",
249+
Method: "tools/list",
250+
PayloadSize: 0,
251+
Payload: "",
252+
},
253+
want: []string{"**github**→`tools/list`"},
254+
notWant: []string{"```json"},
255+
},
180256
}
181257

182258
for _, tt := range tests {
@@ -258,6 +334,24 @@ func TestFormatJSONWithoutFields(t *testing.T) {
258334
wantValid: true,
259335
wantEmpty: false,
260336
},
337+
{
338+
name: "nil fields to remove leaves all fields",
339+
input: `{"jsonrpc":"2.0","method":"tools/list","id":1}`,
340+
fieldsToRemove: nil,
341+
wantContains: []string{`"jsonrpc"`, `"method"`, `"id"`},
342+
wantNotContain: []string{},
343+
wantValid: true,
344+
wantEmpty: false,
345+
},
346+
{
347+
name: "removing non-existent field is a no-op",
348+
input: `{"id":1,"result":{}}`,
349+
fieldsToRemove: []string{"jsonrpc", "method"},
350+
wantContains: []string{`"id"`, `"result"`},
351+
wantNotContain: []string{`"jsonrpc"`, `"method"`},
352+
wantValid: true,
353+
wantEmpty: false,
354+
},
261355
}
262356

263357
for _, tt := range tests {

0 commit comments

Comments
 (0)