Add end-to-end test suite - #2167
Conversation
Sorry, something went wrong.
⚠️ No Changeset foundLatest commit: 39f8e3f Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset. Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Sorry, something went wrong.
npm i https://pkg.pr.new/@modelcontextprotocol/sdk@2167 commit: 39f8e3f |
Sorry, something went wrong.
There was a problem hiding this comment.
I didn't find any bugs in this change, but it's a large addition (~35 files) introducing a whole conformance-style e2e test framework — requirements manifest, knownFailures conventions, transport matrix runner, CI step — so the design and maintenance approach deserve a maintainer's review.
Extended reasoning...Overview
This PR is test-only plus light tooling changes: a new test/e2e/ directory (requirements manifest of ~320 entries, per-area scenario files, a matrix runner producing 762 cells, wire-format sniffer, hosting helpers, a stdio fixture), an eslint override scoped to test/e2e/, package.json script changes (test now excludes test/e2e, new test:e2e), and one added CI step running the e2e suite on Node 24. No src/ files are modified.
Security risks
None of substance. The suite uses mock OAuth servers, locally-generated throwaway keys, and ephemeral 127.0.0.1 listeners; nothing is exposed externally and no production auth/crypto code is changed. The only pipeline change is an additional CI test step, which cannot weaken existing checks.
Level of scrutiny
Although the blast radius on shipped code is essentially zero, the PR establishes long-lived testing conventions: a requirements manifest that doubles as a spec-conformance registry, knownFailures entries that codify current SDK deviations as expected-fail (effectively a backlog of behavior the team is implicitly endorsing as 'known gaps'), a change to what npm test runs by default, and a new CI step. Those are design and maintenance-burden decisions a maintainer should explicitly accept rather than have a bot approve. The sheer size (~10k+ lines) also makes high-confidence automated sign-off inappropriate.
Other factors
The bug hunting system reported no bugs, the suite is self-contained (no shared fixtures leaking into existing tests), and the existing npm test scope is preserved. The author reports 762/762 cells green locally and CI runs the suite, so correctness risk is mostly self-verifying; the open questions are about whether the maintainers want this framework and its conventions in-tree, which only a human can decide.
Sorry, something went wrong.
There was a problem hiding this comment.
I didn't find any bugs in this PR. It's test-only (no src/ changes), but it's a large addition (~30 new test files) that introduces a new conformance-suite architecture (requirements manifest, verifies() linkage, knownFailures expected-fail pattern), plus a new CI job and a change to the npm test script — worth a maintainer's look at the approach and scope rather than a rubber stamp.
Extended reasoning...Overview
This PR adds a conformance-style end-to-end test suite under test/e2e/: a requirements manifest (requirements.ts), per-area scenario test files that cite requirement ids via a verifies() helper, coverage gates (coverage.test.ts), an in-process wire-format sniffer that validates every JSON-RPC message against the SDK's spec-anchored Zod schemas, hosting helpers (per-session/stateless/resumable Streamable HTTP, Express), and a stdio fixture. Outside the test directory the changes are small: a new test-e2e CI job (Node 24), a scoped eslint override for test/e2e/**, and a package.json change splitting npm test (now excludes test/e2e) from a new test:e2e script.
Security risks
None to production code — src/ is untouched and nothing here ships in the published package. The auth-related tests use mock authorization servers, locally generated throwaway keys, and ephemeral 127.0.0.1 listeners only. The CI addition runs the suite in an isolated job with no new secrets or publish-path changes.
Level of scrutiny
Low risk to runtime behavior, but high in volume and in process/architecture decisions: the PR establishes a requirements-manifest testing methodology, encodes a list of knownFailures that doubles as a backlog of asserted SDK spec deviations, changes what npm test runs, and adds a CI job. Those are maintainer-level choices about how the project tests and tracks conformance, not something an automated approval should decide. The diff is also large enough (~688K chars, truncated in automated review) that line-by-line verification of every scenario was not feasible.
Other factors
The bug hunting system reported no bugs, and the portions reviewed (helpers, wire sniffer, hosting helpers, several scenario files, CI/package.json/eslint changes) look careful and self-consistent. One minor note: the PR description references a matrix.test.ts runner, but the implementation uses the verifies() registration in each scenario file plus coverage.test.ts gates instead — the description appears slightly out of date relative to the code. There are no prior human reviews on the thread; the only timeline activity is bot noise (changeset, pkg-pr-new).
Sorry, something went wrong.
Cover behaviors real SDK consumers depend on: the Streamable HTTP client transport error and session surface, reconnection failure reporting, raw and custom transport usage, post-connect callback wrapping, stdio pre-start handling, the auth() helper result values, typed OAuth errors, bearer-header behavior without tokens, the legacy SSE 401 surface, tool metadata round-tripping, the three-argument callTool form, and server session lifecycle callbacks.
wire() can now connect client and server over the HTTP+SSE transport via a real loopback listener, and stateful requirements run on it as a fifth column. Progress delivery over real-socket SSE is recorded as a known failure: the batched response is processed before the progress notifications, which are then dropped as unknown-token.
There was a problem hiding this comment.
I didn't find any bugs in this change, but it's a large test-only addition (~36 files introducing a new conformance-style e2e suite, plus CI and package.json script changes), so the overall structure and CI integration are worth a maintainer's look.
Extended reasoning...Overview
This PR adds a new test/e2e/ conformance suite: a requirements manifest (requirements.ts), per-area scenario test files linked to requirements via a verifies() helper, manifest-consistency gates (coverage.test.ts), a wire-format sniffer that validates every JSON-RPC message against the SDK's Zod schemas, and hosting helpers (in-process StreamableHTTP hosting, Express helpers, an in-process stdio pipe). Outside the test directory it touches three things: a new test-e2e CI job (Node 18 and 24), an eslint override for _-prefixed unused vars in test/e2e/, and package.json scripts (test now excludes test/e2e, new test:e2e).
Security risks
None of substance — no production/source code under src/ is modified, and no secrets or credentials are introduced (all tokens, keys, and OAuth flows in the tests are mock/in-memory). The CI change runs only npm ci and npm run test:e2e with no new secrets or permissions.
Level of scrutiny
Although the change is test-only and cannot affect shipped SDK behavior, it is large (hundreds of new requirements/tests) and introduces structural decisions a maintainer should own: the requirements-manifest/verifies() architecture, the knownFailures convention encoding what the team considers known SDK deviations from the spec, the change to what npm test runs, and the additional CI job/matrix. Those are design/process decisions rather than mechanical changes, so this doesn't meet the bar for auto-approval despite no bugs being found.
Other factors
A few minor consistency nits a human may want to confirm: the PR description and test/e2e/CLAUDE.md reference a matrix.test.ts runner and paths like test/e2e/tools.test.ts, but the actual layout uses verifies() registration in test/e2e/scenarios/*.test.ts with no matrix.test.ts in the file list; helpers/verifies.ts also carries a comment about being 'staged in the harness' / a 'restructure codemod' that reads like a leftover from another workflow. There is no changeset, which seems appropriate for a test-only change. The timeline only has bot comments so far; no human reviewer has weighed in yet.
Sorry, something went wrong.
Adds a conformance-style end-to-end test suite for the SDK's public surface.
Motivation and Context
The SDK has solid unit coverage but no suite that pins consumer-observable behavior end-to-end against the spec: what a client/server actually does over each transport, what goes over the wire, and how the documented hosting patterns behave. This PR adds test/e2e/:
How Has This Been Tested?
Breaking Changes
None. Test-only, plus package.json scripts (test now excludes test/e2e, new test:e2e) and a CI step.
Types of changes
Checklist
Additional context