Commit 43c792b
authored
[test-improver] Improve tests for server/system_tools package (#5715)
# Test Improvements: system_tools_test.go
## File Analyzed
- **Test File**: `internal/server/system_tools_test.go`
- **Package**: `internal/server`
## Improvements Made
### 1. Fixed Bound Asserter Scope in Subtests
Three table-driven test functions created a `assert := assert.New(t)`
bound asserter against the **outer** test's `*testing.T`, then used it
inside `t.Run(...)` subtests where the inner `t` shadows the outer one.
**Problem:** When an assertion fails inside a subtest, the failure is
attributed to the *parent* test rather than the failing subtest. This
makes test output misleading and debugging harder — you see `--- FAIL:
TestHandleRequest_ToolsCall_InvalidJSON` instead of the specific subcase
like `--- FAIL: TestHandleRequest_ToolsCall_InvalidJSON/null_params`.
**Fix:** Moved `assert := assert.New(t)` inside each `t.Run` closure so
the bound asserter is correctly scoped to the subtest's `t`.
Affected functions:
- `TestHandleRequest_ToolsCall_InvalidJSON`
- `TestHandleRequest_ToolsCall_UnknownTool`
- `TestHandleRequest_UnsupportedMethod`
**Before:**
```go
func TestHandleRequest_ToolsCall_InvalidJSON(t *testing.T) {
assert := assert.New(t) // bound to outer t
// ...
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) { // inner t shadows outer t
result, err := server.HandleRequest(...)
assert.Error(err, ...) // uses outer t — wrong!
})
}
}
```
**After:**
```go
func TestHandleRequest_ToolsCall_InvalidJSON(t *testing.T) {
// ...
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert := assert.New(t) // bound to subtest t — correct
result, err := server.HandleRequest(...)
assert.Error(err, ...)
})
}
}
```
## Test Execution
All affected tests pass and failures are now correctly attributed to
their subtests:
```
--- PASS: TestHandleRequest_ToolsCall_InvalidJSON (0.00s)
--- PASS: TestHandleRequest_ToolsCall_InvalidJSON/invalid_JSON (0.00s)
--- PASS: TestHandleRequest_ToolsCall_InvalidJSON/missing_name_field (0.00s)
--- PASS: TestHandleRequest_ToolsCall_InvalidJSON/null_params (0.00s)
--- PASS: TestHandleRequest_ToolsCall_InvalidJSON/empty_object (0.00s)
--- PASS: TestHandleRequest_ToolsCall_InvalidJSON/array_instead_of_object (0.00s)
--- PASS: TestHandleRequest_ToolsCall_InvalidJSON/string_instead_of_object (0.00s)
--- PASS: TestHandleRequest_ToolsCall_UnknownTool (0.00s)
--- PASS: TestHandleRequest_ToolsCall_UnknownTool/unknown_tool (0.00s)
--- PASS: TestHandleRequest_ToolsCall_UnknownTool/misspelled_tool (0.00s)
--- PASS: TestHandleRequest_ToolsCall_UnknownTool/case_sensitive_tool_name (0.00s)
--- PASS: TestHandleRequest_ToolsCall_UnknownTool/empty_tool_name (0.00s)
--- PASS: TestHandleRequest_UnsupportedMethod (0.00s)
--- PASS: TestHandleRequest_UnsupportedMethod/resources/list (0.00s)
--- PASS: TestHandleRequest_UnsupportedMethod/prompts/list (0.00s)
--- PASS: TestHandleRequest_UnsupportedMethod/empty_method (0.00s)
--- PASS: TestHandleRequest_UnsupportedMethod/invalid_method (0.00s)
--- PASS: TestHandleRequest_UnsupportedMethod/case_sensitive_method (0.00s)
```
## Why These Changes?
This is a subtle but real correctness issue with testify's bound
asserter pattern. The `assert.New(t)` call captures the provided `t` at
construction time. When used inside a `t.Run` closure where the inner
function parameter also happens to be named `t`, the *outer* bound
asserter is still referencing the *outer* test context. Test failures
are silently misattributed to the parent test, making subtest-level
debugging impossible.
---
*Generated by Test Improver Workflow*
*Focuses on better patterns, increased coverage, and more stable tests*
> [!WARNING]
> <details>
> <summary>Firewall blocked 1 domain</summary>
>
> 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:
>
> ```yaml
> network:
> allowed:
> - defaults
> - "invalidhostthatdoesnotexist12345.com"
> ```
>
> See [Network
Configuration](https://github.github.com/gh-aw/reference/network/) for
more information.
>
> </details>
> Generated by [Test
Improver](https://github.com/github/gh-aw-mcpg/actions/runs/25892664198/agentic_workflow)
· ● 3.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, version:
1.0.40, model: claude-sonnet-4.6, id: 25892664198, workflow_id:
test-improver, run:
https://github.com/github/gh-aw-mcpg/actions/runs/25892664198 -->
<!-- gh-aw-workflow-id: test-improver -->1 file changed
Lines changed: 3 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
261 | 261 | | |
262 | 262 | | |
263 | 263 | | |
264 | | - | |
265 | | - | |
266 | 264 | | |
267 | 265 | | |
268 | 266 | | |
| |||
297 | 295 | | |
298 | 296 | | |
299 | 297 | | |
| 298 | + | |
300 | 299 | | |
301 | 300 | | |
302 | 301 | | |
| |||
307 | 306 | | |
308 | 307 | | |
309 | 308 | | |
310 | | - | |
311 | | - | |
312 | 309 | | |
313 | 310 | | |
314 | 311 | | |
| |||
335 | 332 | | |
336 | 333 | | |
337 | 334 | | |
| 335 | + | |
338 | 336 | | |
339 | 337 | | |
340 | 338 | | |
| |||
355 | 353 | | |
356 | 354 | | |
357 | 355 | | |
358 | | - | |
359 | | - | |
360 | 356 | | |
361 | 357 | | |
362 | 358 | | |
| |||
387 | 383 | | |
388 | 384 | | |
389 | 385 | | |
| 386 | + | |
390 | 387 | | |
391 | 388 | | |
392 | 389 | | |
| |||
0 commit comments