From 41410ad25a41a3aec312c4eb0dde3c38c4fcd473 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 25 Apr 2025 13:16:21 +0200 Subject: [PATCH 1/5] Build image only once in e2e tests --- e2e/e2e_test.go | 60 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 18 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 3d8c45dc..a4293858 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "os" "os/exec" + "sync" "testing" "time" @@ -16,16 +17,48 @@ import ( "github.com/stretchr/testify/require" ) -func TestE2E(t *testing.T) { - e2eServerToken := os.Getenv("GITHUB_MCP_SERVER_E2E_TOKEN") - if e2eServerToken == "" { - t.Fatalf("GITHUB_MCP_SERVER_E2E_TOKEN environment variable is not set") - } +var ( + // Shared variables and sync.Once instances to ensure one-time execution + getTokenOnce sync.Once + e2eToken string - // Build the Docker image for the MCP server. - buildDockerImage(t) + buildOnce sync.Once + buildError error +) - t.Setenv("GITHUB_PERSONAL_ACCESS_TOKEN", e2eServerToken) // The MCP Client merges the existing environment. +// getE2EToken ensures the environment variable is checked only once and returns the token +func getE2EToken(t *testing.T) string { + getTokenOnce.Do(func() { + e2eToken = os.Getenv("GITHUB_MCP_SERVER_E2E_TOKEN") + if e2eToken == "" { + t.Fatalf("GITHUB_MCP_SERVER_E2E_TOKEN environment variable is not set") + } + }) + return e2eToken +} + +// ensureDockerImageBuilt makes sure the Docker image is built only once across all tests +func ensureDockerImageBuilt(t *testing.T) { + buildOnce.Do(func() { + t.Log("Building Docker image for e2e tests...") + cmd := exec.Command("docker", "build", "-t", "github/e2e-github-mcp-server", ".") + cmd.Dir = ".." // Run this in the context of the root, where the Dockerfile is located. + output, err := cmd.CombinedOutput() + buildError = err + if err != nil { + t.Logf("Docker build output: %s", string(output)) + } + }) + + // Check if the build was successful + require.NoError(t, buildError, "expected to build Docker image successfully") +} + +func TestE2E(t *testing.T) { + token := getE2EToken(t) + ensureDockerImageBuilt(t) + + t.Setenv("GITHUB_PERSONAL_ACCESS_TOKEN", token) // The MCP Client merges the existing environment. args := []string{ "docker", "run", @@ -81,7 +114,7 @@ func TestE2E(t *testing.T) { // Then the login in the response should match the login obtained via the same // token using the GitHub API. - client := github.NewClient(nil).WithAuthToken(e2eServerToken) + client := github.NewClient(nil).WithAuthToken(token) user, _, err := client.Users.Get(context.Background(), "") require.NoError(t, err, "expected to get user successfully") require.Equal(t, trimmedContent.Login, *user.Login, "expected login to match") @@ -89,12 +122,3 @@ func TestE2E(t *testing.T) { require.NoError(t, client.Close(), "expected to close client successfully") } - -func buildDockerImage(t *testing.T) { - t.Log("Building Docker image for e2e tests...") - - cmd := exec.Command("docker", "build", "-t", "github/e2e-github-mcp-server", ".") - cmd.Dir = ".." // Run this in the context of the root, where the Dockerfile is located. - output, err := cmd.CombinedOutput() - require.NoError(t, err, "expected to build Docker image successfully, output: %s", string(output)) -} From 7118ff9b301c6a129f0e047823ac40fd3b873445 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 25 Apr 2025 13:33:38 +0200 Subject: [PATCH 2/5] Use functional options in e2e as prep for next test --- e2e/e2e_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 72 insertions(+), 12 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index a4293858..3aa300af 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -20,7 +20,7 @@ import ( var ( // Shared variables and sync.Once instances to ensure one-time execution getTokenOnce sync.Once - e2eToken string + token string buildOnce sync.Once buildError error @@ -29,12 +29,12 @@ var ( // getE2EToken ensures the environment variable is checked only once and returns the token func getE2EToken(t *testing.T) string { getTokenOnce.Do(func() { - e2eToken = os.Getenv("GITHUB_MCP_SERVER_E2E_TOKEN") - if e2eToken == "" { + token = os.Getenv("GITHUB_MCP_SERVER_E2E_TOKEN") + if token == "" { t.Fatalf("GITHUB_MCP_SERVER_E2E_TOKEN environment variable is not set") } }) - return e2eToken + return token } // ensureDockerImageBuilt makes sure the Docker image is built only once across all tests @@ -54,11 +54,55 @@ func ensureDockerImageBuilt(t *testing.T) { require.NoError(t, buildError, "expected to build Docker image successfully") } -func TestE2E(t *testing.T) { +// ClientOpts holds configuration options for the MCP client setup +type ClientOpts struct { + // Environment variables to set before starting the client + EnvVars map[string]string + // Whether to initialize the client after creation + ShouldInitialize bool +} + +// ClientOption defines a function type for configuring ClientOpts +type ClientOption func(*ClientOpts) + +// WithEnvVars returns an option that adds environment variables to the client options +func WithEnvVars(envVars map[string]string) ClientOption { + return func(opts *ClientOpts) { + opts.EnvVars = envVars + } +} + +// WithInitialize returns an option that configures the client to be initialized +func WithInitialize() ClientOption { + return func(opts *ClientOpts) { + opts.ShouldInitialize = true + } +} + +// setupMCPClient sets up the test environment and returns an initialized MCP client +// It handles token retrieval, Docker image building, and applying the provided options +func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { + // Get token and ensure Docker image is built token := getE2EToken(t) ensureDockerImageBuilt(t) - t.Setenv("GITHUB_PERSONAL_ACCESS_TOKEN", token) // The MCP Client merges the existing environment. + // Create and configure options + opts := &ClientOpts{ + EnvVars: make(map[string]string), + } + + // Apply all options to configure the opts struct + for _, option := range options { + option(opts) + } + + // Set the GitHub token and other environment variables + t.Setenv("GITHUB_PERSONAL_ACCESS_TOKEN", token) + for key, value := range opts.EnvVars { + t.Setenv(key, value) + } + + // Prepare Docker arguments args := []string{ "docker", "run", @@ -66,13 +110,23 @@ func TestE2E(t *testing.T) { "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", - "github/e2e-github-mcp-server", } + + // Add all environment variables to the Docker arguments + for key := range opts.EnvVars { + args = append(args, "-e", key) + } + + // Add the image name + args = append(args, "github/e2e-github-mcp-server") + + // Create the client t.Log("Starting Stdio MCP client...") client, err := mcpClient.NewStdioMCPClient(args[0], []string{}, args[1:]...) require.NoError(t, err, "expected to create client successfully") - t.Run("Initialize", func(t *testing.T) { + // Initialize the client if configured to do so + if opts.ShouldInitialize { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -84,10 +138,16 @@ func TestE2E(t *testing.T) { } result, err := client.Initialize(ctx, request) - require.NoError(t, err, "expected to initialize successfully") + require.NoError(t, err, "failed to initialize client") + require.Equal(t, "github-mcp-server", result.ServerInfo.Name, "unexpected server name") + } - require.Equal(t, "github-mcp-server", result.ServerInfo.Name) - }) + return client +} + +func TestE2E(t *testing.T) { + // Setup the MCP client with initialization + client := setupMCPClient(t, WithInitialize()) t.Run("CallTool get_me", func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -114,7 +174,7 @@ func TestE2E(t *testing.T) { // Then the login in the response should match the login obtained via the same // token using the GitHub API. - client := github.NewClient(nil).WithAuthToken(token) + client := github.NewClient(nil).WithAuthToken(getE2EToken(t)) user, _, err := client.Users.Get(context.Background(), "") require.NoError(t, err, "expected to get user successfully") require.Equal(t, trimmedContent.Login, *user.Login, "expected login to match") From 95361188f9dc54b7a8a2ca84c25ce8b39acb3664 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 25 Apr 2025 13:38:05 +0200 Subject: [PATCH 3/5] Always initialize in e2e tests --- e2e/e2e_test.go | 90 +++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 52 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 3aa300af..5d5c2032 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -58,8 +58,6 @@ func ensureDockerImageBuilt(t *testing.T) { type ClientOpts struct { // Environment variables to set before starting the client EnvVars map[string]string - // Whether to initialize the client after creation - ShouldInitialize bool } // ClientOption defines a function type for configuring ClientOpts @@ -72,13 +70,6 @@ func WithEnvVars(envVars map[string]string) ClientOption { } } -// WithInitialize returns an option that configures the client to be initialized -func WithInitialize() ClientOption { - return func(opts *ClientOpts) { - opts.ShouldInitialize = true - } -} - // setupMCPClient sets up the test environment and returns an initialized MCP client // It handles token retrieval, Docker image building, and applying the provided options func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { @@ -125,60 +116,55 @@ func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { client, err := mcpClient.NewStdioMCPClient(args[0], []string{}, args[1:]...) require.NoError(t, err, "expected to create client successfully") - // Initialize the client if configured to do so - if opts.ShouldInitialize { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - - request := mcp.InitializeRequest{} - request.Params.ProtocolVersion = "2025-03-26" - request.Params.ClientInfo = mcp.Implementation{ - Name: "e2e-test-client", - Version: "0.0.1", - } + // Initialize the client + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() - result, err := client.Initialize(ctx, request) - require.NoError(t, err, "failed to initialize client") - require.Equal(t, "github-mcp-server", result.ServerInfo.Name, "unexpected server name") + request := mcp.InitializeRequest{} + request.Params.ProtocolVersion = "2025-03-26" + request.Params.ClientInfo = mcp.Implementation{ + Name: "e2e-test-client", + Version: "0.0.1", } + result, err := client.Initialize(ctx, request) + require.NoError(t, err, "failed to initialize client") + require.Equal(t, "github-mcp-server", result.ServerInfo.Name, "unexpected server name") + return client } -func TestE2E(t *testing.T) { - // Setup the MCP client with initialization - client := setupMCPClient(t, WithInitialize()) +func TestGetMe(t *testing.T) { + mcpClient := setupMCPClient(t) - t.Run("CallTool get_me", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() - // When we call the "get_me" tool - request := mcp.CallToolRequest{} - request.Params.Name = "get_me" + // When we call the "get_me" tool + request := mcp.CallToolRequest{} + request.Params.Name = "get_me" - response, err := client.CallTool(ctx, request) - require.NoError(t, err, "expected to call 'get_me' tool successfully") + response, err := mcpClient.CallTool(ctx, request) + require.NoError(t, err, "expected to call 'get_me' tool successfully") - require.False(t, response.IsError, "expected result not to be an error") - require.Len(t, response.Content, 1, "expected content to have one item") + require.False(t, response.IsError, "expected result not to be an error") + require.Len(t, response.Content, 1, "expected content to have one item") - textContent, ok := response.Content[0].(mcp.TextContent) - require.True(t, ok, "expected content to be of type TextContent") + textContent, ok := response.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") - var trimmedContent struct { - Login string `json:"login"` - } - err = json.Unmarshal([]byte(textContent.Text), &trimmedContent) - require.NoError(t, err, "expected to unmarshal text content successfully") - - // Then the login in the response should match the login obtained via the same - // token using the GitHub API. - client := github.NewClient(nil).WithAuthToken(getE2EToken(t)) - user, _, err := client.Users.Get(context.Background(), "") - require.NoError(t, err, "expected to get user successfully") - require.Equal(t, trimmedContent.Login, *user.Login, "expected login to match") - }) + var trimmedContent struct { + Login string `json:"login"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedContent) + require.NoError(t, err, "expected to unmarshal text content successfully") + + // Then the login in the response should match the login obtained via the same + // token using the GitHub API. + ghClient := github.NewClient(nil).WithAuthToken(getE2EToken(t)) + user, _, err := ghClient.Users.Get(context.Background(), "") + require.NoError(t, err, "expected to get user successfully") + require.Equal(t, trimmedContent.Login, *user.Login, "expected login to match") - require.NoError(t, client.Close(), "expected to close client successfully") + require.NoError(t, mcpClient.Close(), "expected to close client successfully") } From 0207ce663f2fd522b0cf98da149a6122c4e309a0 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 25 Apr 2025 13:58:00 +0200 Subject: [PATCH 4/5] Ensure toolsets are configurable via env var --- cmd/github-mcp-server/main.go | 10 +++++++++- e2e/e2e_test.go | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 5ca0e21c..cf459f47 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -45,7 +45,15 @@ var ( stdlog.Fatal("Failed to initialize logger:", err) } - enabledToolsets := viper.GetStringSlice("toolsets") + // If you're wondering why we're not using viper.GetStringSlice("toolsets"), + // it's because viper doesn't handle comma-separated values correctly for env + // vars when using GetStringSlice. + // https://github.com/spf13/viper/issues/380 + var enabledToolsets []string + err = viper.UnmarshalKey("toolsets", &enabledToolsets) + if err != nil { + stdlog.Fatal("Failed to unmarshal toolsets:", err) + } logCommands := viper.GetBool("enable-command-logging") cfg := runConfig{ diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 5d5c2032..a3f5df6f 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "os" "os/exec" + "slices" "sync" "testing" "time" @@ -115,6 +116,9 @@ func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { t.Log("Starting Stdio MCP client...") client, err := mcpClient.NewStdioMCPClient(args[0], []string{}, args[1:]...) require.NoError(t, err, "expected to create client successfully") + t.Cleanup(func() { + require.NoError(t, client.Close(), "expected to close client successfully") + }) // Initialize the client ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -166,5 +170,33 @@ func TestGetMe(t *testing.T) { require.NoError(t, err, "expected to get user successfully") require.Equal(t, trimmedContent.Login, *user.Login, "expected login to match") - require.NoError(t, mcpClient.Close(), "expected to close client successfully") +} + +func TestToolsets(t *testing.T) { + mcpClient := setupMCPClient( + t, + WithEnvVars(map[string]string{ + "GITHUB_TOOLSETS": "repos,issues", + }), + ) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + request := mcp.ListToolsRequest{} + response, err := mcpClient.ListTools(ctx, request) + require.NoError(t, err, "expected to list tools successfully") + + // We could enumerate the tools here, but we'll need to expose that information + // declaratively in the MCP server, so for the moment let's just check the existence + // of an issue and repo tool, and the non-existence of a pull_request tool. + var toolsContains = func(expectedName string) bool { + return slices.ContainsFunc(response.Tools, func(tool mcp.Tool) bool { + return tool.Name == expectedName + }) + } + + require.True(t, toolsContains("get_issue"), "expected to find 'get_issue' tool") + require.True(t, toolsContains("list_branches"), "expected to find 'list_branches' tool") + require.False(t, toolsContains("get_pull_request"), "expected not to find 'get_pull_request' tool") } From fcf96940d1071f29af319489254408c4c141cc28 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 25 Apr 2025 14:04:30 +0200 Subject: [PATCH 5/5] Support e2e test parallelisation --- e2e/e2e_test.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index a3f5df6f..757dd5c2 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -5,6 +5,7 @@ package e2e_test import ( "context" "encoding/json" + "fmt" "os" "os/exec" "slices" @@ -88,12 +89,6 @@ func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { option(opts) } - // Set the GitHub token and other environment variables - t.Setenv("GITHUB_PERSONAL_ACCESS_TOKEN", token) - for key, value := range opts.EnvVars { - t.Setenv(key, value) - } - // Prepare Docker arguments args := []string{ "docker", @@ -101,7 +96,7 @@ func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { "-i", "--rm", "-e", - "GITHUB_PERSONAL_ACCESS_TOKEN", + "GITHUB_PERSONAL_ACCESS_TOKEN", // Personal access token is all required } // Add all environment variables to the Docker arguments @@ -112,9 +107,16 @@ func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { // Add the image name args = append(args, "github/e2e-github-mcp-server") + // Construct the env vars for the MCP Client to execute docker with + dockerEnvVars := make([]string, 0, len(opts.EnvVars)+1) + dockerEnvVars = append(dockerEnvVars, fmt.Sprintf("GITHUB_PERSONAL_ACCESS_TOKEN=%s", token)) + for key, value := range opts.EnvVars { + dockerEnvVars = append(dockerEnvVars, fmt.Sprintf("%s=%s", key, value)) + } + // Create the client t.Log("Starting Stdio MCP client...") - client, err := mcpClient.NewStdioMCPClient(args[0], []string{}, args[1:]...) + client, err := mcpClient.NewStdioMCPClient(args[0], dockerEnvVars, args[1:]...) require.NoError(t, err, "expected to create client successfully") t.Cleanup(func() { require.NoError(t, client.Close(), "expected to close client successfully") @@ -139,6 +141,8 @@ func setupMCPClient(t *testing.T, options ...ClientOption) *mcpClient.Client { } func TestGetMe(t *testing.T) { + t.Parallel() + mcpClient := setupMCPClient(t) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -173,6 +177,8 @@ func TestGetMe(t *testing.T) { } func TestToolsets(t *testing.T) { + t.Parallel() + mcpClient := setupMCPClient( t, WithEnvVars(map[string]string{