[test-improver] Improve tests for mcptest harness

[github-actions]

Test Improvements: harness_test.go

File Analyzed

• Test File: internal/testutil/mcptest/harness_test.go
• Package: internal/testutil/mcptest
• Lines of Code: 275 → 197 (−78 lines of boilerplate)

Improvements Made

1. Better Testify Assertions

Replaced all manual if-block error checking with idiomatic testify calls:

• ✅ require.NoError(t, err) instead of if err := ...; err != nil { t.Fatalf(...) }
• ✅ require.Len(t, tools, 1) instead of if len(tools) != 1 { t.Errorf(...) }
• ✅ assert.Equal(t, "test_echo", tools[0].Name) instead of nested if tools[0].Name != ...
• ✅ assert.False(t, result.IsError) instead of if result.IsError { t.Error(...) }
• ✅ require.True(t, ok, "Expected *sdk.TextContent") instead of if !ok { t.Error(...) }
• ✅ assert.ElementsMatch(t, []string{"echo1", "echo2", "add"}, toolNames) instead of manual map-lookup loop
• ✅ Added assert import alongside existing require

2. Increased Coverage

• ✅ Added missing assertion on add tool result: assert.Equal(t, "8", textContent.Text) — the original test called the add tool but never verified the return value
• ✅ require.Len on result.Content before accessing result.Content[0], preventing a potential index panic

3. Cleaner & More Stable Tests

• ✅ Removed redundant if len(x) > 0 { ... } guards that were only needed because t.Errorf doesn't stop execution (now require.Len stops on failure)
• ✅ Removed t.Logf("✓ ...") status messages that added noise without testing anything
• ✅ Cleaner linear test flow without deeply nested conditionals
• ✅ Consistent err := declaration pattern across all test functions

Why These Changes?

harness_test.go already imported require from testify but performed most assertions via manual if-blocks with t.Errorf/t.Fatalf. This pattern lets tests continue running after a failure, leading to confusing cascaded errors (e.g., an index-out-of-bounds panic after a length mismatch). Converting to testify stops tests at the right boundary, gives clearer failure messages, and removes substantial boilerplate.

The missing assertion on the add tool result was a genuine coverage gap — the test confirmed the tool call succeeded but never checked the computed value.

---

Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests

Generated by Test Improver · ◷

[Copilot]

Pull request overview

This PR refactors the internal/testutil/mcptest harness tests to reduce boilerplate and make failures clearer by standardizing on testify assertions.

Changes:

• Replaced manual if/t.Errorf/t.Fatalf checks with require/assert helpers.
• Added additional result validation (notably for the add tool) and safer length checks before indexing.
• Simplified test flow by removing redundant guards and logging noise.

Comments suppressed due to low confidence (1)

internal/testutil/mcptest/harness_test.go:120

• Same as above: if the tool returns IsError=true, prefer a require assertion so later content/type assertions don’t run and obscure the primary failure.

	require.NoError(t, err, "Failed to call add tool")
	assert.False(t, result.IsError)
	require.Len(t, result.Content, 1, "Expected exactly 1 content item")
	textContent, ok := result.Content[0].(*sdk.TextContent)

---

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

[Copilot]

result.IsError indicates the tool reported an error; using assert.False allows the test to continue and can produce additional, less relevant failures. Use a require assertion here so the test stops immediately when the tool call reports an error.

This issue also appears on line 117 of the same file.

Suggested change

	assert.False(t, result.IsError)
	require.False(t, result.IsError)