Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 18, 2025

Missing integration tests for MCP server tools left JSON output format unvalidated. This adds comprehensive test coverage ensuring each tool returns valid, parseable JSON.

Changes

New test file: pkg/cli/mcp_server_json_integration_test.go

  • Helper functions:

    • isValidJSON() - JSON structure validation
    • extractJSONFromOutput() - Strips console formatting (Unicode chars) from output
    • setupMCPServerTest() - Common test harness with --cmd flag to avoid gh extension requirement
  • Integration tests:

    • Individual tests for status, compile, audit, logs tools validating JSON structure and expected fields
    • jq filter functionality test
    • Comprehensive test covering all 7 MCP server tools with appropriate validation per tool type

Key handling:

  • Console-formatted output (e.g., ✓ .github/workflows/test.md\n[{...}]) extracted correctly
  • Error cases (missing credentials, invalid run IDs) handled gracefully without false failures
  • JSON field validation ensures output contracts are maintained

Example

// Extract JSON from console-formatted output
jsonOutput := extractJSONFromOutput(textContent.Text)
if !isValidJSON(jsonOutput) {
    t.Errorf("Tool did not return valid JSON")
}

var statusData []map[string]any
json.Unmarshal([]byte(jsonOutput), &statusData)

// Validate expected fields
expectedFields := []string{"workflow", "engine_id", "compiled", "status"}
for _, field := range expectedFields {
    if _, ok := statusData[0][field]; !ok {
        t.Errorf("Expected field '%s' not found", field)
    }
}
Original prompt

Add MCP integration tests for the mcp-server commands. Make sure each tool returns valid JSON.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI self-assigned this Nov 18, 2025
…tools

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Add MCP integration tests for server commands Add MCP integration tests with JSON validation for all server tools Nov 18, 2025
Copilot AI requested a review from pelikhan November 18, 2025 08:26
Copilot finished work on behalf of pelikhan November 18, 2025 08:26
@pelikhan pelikhan marked this pull request as ready for review November 18, 2025 12:25
Copilot AI review requested due to automatic review settings November 18, 2025 12:25
Copilot finished reviewing on behalf of pelikhan November 18, 2025 12:27
Copy link
Contributor

Copilot AI left a comment

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 adds comprehensive integration tests for MCP server tools to validate JSON output format. The new test file ensures each MCP server tool returns valid, parseable JSON with expected field structures.

Key Changes:

  • Adds mcp_server_json_integration_test.go with JSON validation tests for all 7 MCP server tools
  • Implements helper functions for JSON extraction and validation from console-formatted output
  • Includes graceful error handling for tools that may fail in test environments without credentials

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


// The audit tool may return an error message for invalid run IDs
// We check if the output contains "Error:" which indicates an error message
if len(textContent.Text) >= 6 && textContent.Text[0:6] == "Error:" {
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use strings.HasPrefix(textContent.Text, \"Error:\") instead of manual bounds checking and substring comparison. This is the idiomatic Go pattern used consistently throughout the codebase (seen in 100+ locations) and is more readable and safer.

Copilot uses AI. Check for mistakes.
}

// The logs tool may return an error message in test environment
if len(textContent.Text) >= 6 && textContent.Text[0:6] == "Error:" {
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use strings.HasPrefix(textContent.Text, \"Error:\") instead of manual bounds checking and substring comparison. This is the idiomatic Go pattern used consistently throughout the codebase (seen in 100+ locations) and is more readable and safer.

Copilot uses AI. Check for mistakes.
// If tool is expected to return JSON, validate it
if tc.expectJSON {
// Skip JSON validation if output starts with "Error:"
if len(textContent.Text) >= 6 && textContent.Text[0:6] == "Error:" {
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use strings.HasPrefix(textContent.Text, \"Error:\") instead of manual bounds checking and substring comparison. This is the idiomatic Go pattern used consistently throughout the codebase (seen in 100+ locations) and is more readable and safer.

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +16
import (
"context"
"encoding/json"
"os"
"os/exec"
"path/filepath"
"testing"
"time"

"github.com/githubnext/gh-aw/pkg/testutil"
"github.com/modelcontextprotocol/go-sdk/mcp"
)
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing strings package import needed for the suggested strings.HasPrefix() usage to replace manual substring checks.

Copilot uses AI. Check for mistakes.
@pelikhan pelikhan merged commit 5605d47 into main Nov 18, 2025
143 of 144 checks passed
@pelikhan pelikhan deleted the copilot/add-mcp-integration-tests branch November 18, 2025 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants