Support debugging e2e tests #380
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors server startup logic into a shared internal package and adds an in-process debug mode for end-to-end tests.
- Extract
NewMCPServerandRunStdioServerintointernal/ghmcpand wire them into the CLI - Update e2e tests to optionally run the MCP server in‐process via
GITHUB_MCP_SERVER_E2E_DEBUG - Document the new debug mode in
e2e/README.md
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/ghmcp/server.go | New shared server construction & stdio runner |
| e2e/e2e_test.go | Refactor test client setup to support Docker vs. in-process debug |
| e2e/README.md | Add instructions for the GITHUB_MCP_SERVER_E2E_DEBUG flag |
| cmd/github-mcp-server/main.go | Switch CLI commands to use internal/ghmcp and RunE for errors |
Comments suppressed due to low confidence (2)
internal/ghmcp/server.go:101
- [nitpick] The variable name
contextshadows the importedcontextpackage. Consider renaming toctxToolsetor similar to avoid confusion.
context := github.InitContextToolset(getClient, cfg.Translator)
internal/ghmcp/server.go:47
- Newly extracted functions like
NewMCPServerandRunStdioServerlack direct unit tests. Consider adding unit tests to cover config parsing, hook registration, and shutdown behavior.
func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
| enabledToolsets = github.DefaultTools | ||
| } | ||
|
|
||
| ghServer, err := ghmcp.NewMCPServer(ghmcp.MCPServerConfig{ |
There was a problem hiding this comment.
The MCPServerConfig only sets Token, EnabledToolsets, and Translator, leaving Version, Host, DynamicToolsets, and ReadOnly at zero values. This can change client behavior—please populate all required fields.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Yeh they aren't required, that's why.
This commit cleanly separates config parsing, stdio server execution and mcp server construction. Aside from significant clarity improvements, it allows for direct construction of the mcp server in e2e tests to allow for breakpoint debugging.
williammartin
force-pushed
the
wm/debug-e2e-tests
branch
from
9bdd601 to
635b831
Compare
e2e/e2e_test.go
Outdated
| 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.enabledToolsets)+1) |
There was a problem hiding this comment.
If I'm not mistaken, as you are joining opts.enabledToolsets below, you might not want to set this slice size, and it could be just 2?
There was a problem hiding this comment.
Thanks, this was part of the previous implementation where we supported WithEnvVars, so it could have been n. Good spot, thanks, will adjust before merge.
williammartin
force-pushed
the
wm/debug-e2e-tests
branch
from
635b831 to
bd1df79
Compare
|
Bypassing review requirement as the only thing I changed after @juruen approved was to fix https://github.com/github/github-mcp-server/pull/380/files#r2077653340 |
Description
While working on other issues, I often found myself experiencing failures that were not easy to debug given the black box nature of the e2e tests. Now we can use
GITHUB_MCP_SERVER_E2E_DEBUG=trueto run an inprocess server version of the server, allowing for breakpointing. Open to renaming the env var to indicate exactly what it does, rather than debug. See image:Doing this also drove out some nice cleanups, separating config parsing, mcp server construction and stdio server execution. I have not written unit tests for these extracted functions as they were not previously tested and currently get most of their coverage from the e2e tests. I believe that later we may want to look into further testing of these to support further refactors, but that is out of scope for this PR.