- Notifications
You must be signed in to change notification settings - Fork899
Support debugging e2e tests#380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this 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 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 theGITHUB_MCP_SERVER_E2E_DEBUG flag |
| cmd/github-mcp-server/main.go | Switch CLI commands to useinternal/ghmcp andRunE 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TheMCPServerConfig only setsToken,EnabledToolsets, andTranslator, leavingVersion,Host,DynamicToolsets, andReadOnly 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeh they aren't required, that's why.
This commit cleanly separates config parsing, stdio server execution andmcp server construction. Aside from significant clarity improvements, itallows for direct construction of the mcp server in e2e tests to allowfor breakpoint debugging.
9bdd601 to635b831CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks great! Just a small comment. 🚀
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I'm not mistaken, as you arejoiningopts.enabledToolsets below, you might not want to set this slice size, and it could be just2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, this was part of the previous implementation where we supportedWithEnvVars, so it could have beenn. Good spot, thanks, will adjust before merge.
635b831 tobd1df79CompareBypassing review requirement as the only thing I changed after@juruen approved was to fixhttps://github.com/github/github-mcp-server/pull/380/files#r2077653340 |
0ca07aa intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.