Uh oh!
There was an error while loading. Please reload this page.
Extract mcptest: the wire-test harness both instances duplicated - #2
Conversation
In-memory MCP client/server connect, ListTools/CallText helpers, and the golden-snapshot compare with -update — moved from basecamp-mcp-server (internal/server/mcp_test.go, internal/tools/snapshot_test.go) and hey-mcp-server (internal/server/server_test.go), where the harness existed near-verbatim by duplication. Polish where the instances trivially disagreed: helpers take testing.TB, Connect closes both sessions via t.Cleanup, and the snapshot switch is the -update flag (hey's spelling) rather than an env var.
There was a problem hiding this comment.
Pull request overview
Extracts the duplicated MCP wire-test harness into the shared mcptest package.
Changes:
- Adds in-memory client/server connection and tool-call helpers.
- Adds golden snapshot comparison and rewriting.
- Adds self-tests and MCP SDK dependencies.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
mcptest/mcptest.go | Implements wire-test helpers. |
mcptest/snapshot.go | Implements snapshot comparison and rewriting. |
mcptest/mcptest_test.go | Tests connection, calls, errors, and snapshots. |
mcptest/testdata/snapshot_fixture.txt | Provides the golden fixture. |
go.mod | Adds required dependencies. |
go.sum | Records dependency checksums. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
The SDK server pages tool listings by ServerOptions.PageSize, so taking only the first ListTools response drops tools on servers larger than one page. Iterate with ClientSession.Tools, which follows NextCursor; a one-tool page size in the test forces the multi-page path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1bf1d97d9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Extraction PR 2 of 4 per the program board's plan of record (internal): the wire-test harness, moved from where both product instances proved it by duplication.
What moves
mcptest.Connect— connects a real MCP client to a server over in-memory transports, so tests assert against the wire surface (tool listings, call results,isError) rather than internal state. From basecamp-mcp-server'sconnectInMemory(internal/server/mcp_test.go) and hey-mcp-server'sconnect(internal/server/server_test.go).mcptest.ListTools/mcptest.CallText— the listing and text-call helpers, verbatim from hey-mcp-server, shape-identical in basecamp-mcp-server.mcptest.Snapshot— golden-file compare with a rewrite switch, the mechanism both instances carry (hey's catalog snapshot, basecamp's tool snapshots).Polish (only where the instances trivially disagree)
testing.TB(both instances used*testing.T).Connectcloses both sessions viat.Cleanup(basecamp's behavior; hey closed only the client).-updateflag (hey's spelling) rather than basecamp'sBASECAMP_UPDATE_SNAPSHOTSenv var.No redesign otherwise. Self-tests cover the round-trip, in-band error surfacing, and snapshot compare/rewrite.
Adoption PRs in both instances follow once
gatewayandcatalogland, per the sequence.