From c60a128df3f4822fe1e014dc934775b013569afa Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 24 Jun 2026 07:46:09 -0700 Subject: [PATCH] Test/1396 harden managed state connect tests (#1508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(state): wait for connect refresh before asserting list RPC gate (#1396) Replace fixed double-microtask flushes in Managed*State connect-path tests with *Change event waits so the capability gate is asserted after refresh completes. * test(state): fail fast with a readable message if a connect refresh never dispatches its change event (#1396) The hardened connect-path gate tests now depend on the gated (capability-less / empty-list) refresh still emitting its *Change event. A future optimization that skipped that redundant dispatch would make these awaits hang to the vitest per-test timeout with no clue why. Extract the per-file waitFor*Change helpers into a shared, generically-typed waitForChangeEvent() that races the listener against a short timeout and rejects naming the event that never fired — turning a future hang into a fast, readable failure. This also removes the duplicated listener/Promise boilerplate across the five state test files. No production code changes. Co-Authored-By: Claude Opus 4.8 (1M context) * test(state): cover void-detail event resolution in waitForChangeEvent Add a case asserting the helper still resolves for a signal-only (void-detail) event — CustomEvent normalizes an absent detail to null, so the listener fires and resolves rather than hanging. Closes the coverage gap noted in review. Co-Authored-By: Claude Opus 4.8 (1M context) * docs(core): correct TypedEventGeneric void-detail comment to null CustomEvent normalizes an absent detail to null, so listeners receive null (not undefined) for void events — as the new waitForChangeEvent void case establishes. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: cliffhall Co-authored-by: Claude Opus 4.8 (1M context) --- .../mcp/state/managedPromptsState.test.ts | 18 ++----- .../state/managedRequestorTasksState.test.ts | 12 ++--- .../managedResourceTemplatesState.test.ts | 14 ++--- .../mcp/state/managedResourcesState.test.ts | 18 ++----- .../core/mcp/state/managedToolsState.test.ts | 18 ++----- .../core/mcp/state/waitForChangeEvent.test.ts | 47 +++++++++++++++++ .../test/core/mcp/state/waitForChangeEvent.ts | 52 +++++++++++++++++++ core/mcp/typedEventTarget.ts | 3 +- 8 files changed, 124 insertions(+), 58 deletions(-) create mode 100644 clients/web/src/test/core/mcp/state/waitForChangeEvent.test.ts create mode 100644 clients/web/src/test/core/mcp/state/waitForChangeEvent.ts diff --git a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts index 2cc964c2..79f9d4a4 100644 --- a/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedPromptsState.test.ts @@ -3,6 +3,7 @@ import type { Prompt } from "@modelcontextprotocol/sdk/types.js"; import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; import { ManagedPromptsState } from "@inspector/core/mcp/state/managedPromptsState"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; +import { waitForChangeEvent } from "./waitForChangeEvent"; function prompt(name: string): Prompt { return { name }; @@ -20,19 +21,11 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { }; function waitForPromptsChange(state: ManagedPromptsState): Promise { - return new Promise((resolve) => { - state.addEventListener("promptsChange", (e) => resolve(e.detail), { - once: true, - }); - }); + return waitForChangeEvent(state, "promptsChange"); } function waitForListChanged(state: ManagedPromptsState): Promise { - return new Promise((resolve) => { - state.addEventListener("listChangedChange", (e) => resolve(e.detail), { - once: true, - }); - }); + return waitForChangeEvent(state, "listChangedChange"); } describe("ManagedPromptsState", () => { @@ -84,10 +77,9 @@ describe("ManagedPromptsState", () => { promptless.setStatus("connected"); const promptlessState = new ManagedPromptsState(promptless, 0); + const changePromise = waitForPromptsChange(promptlessState); promptless.dispatchTypedEvent("connect"); - // Yield so the async refresh chained off connect runs. - await Promise.resolve(); - await Promise.resolve(); + await changePromise; expect(promptless.listPrompts).not.toHaveBeenCalled(); expect(promptlessState.getPrompts()).toEqual([]); }); diff --git a/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts b/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts index 32212b43..ca00ee9f 100644 --- a/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedRequestorTasksState.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach } from "vitest"; import type { Task } from "@modelcontextprotocol/sdk/types.js"; import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedRequestorTasksState"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; +import { waitForChangeEvent } from "./waitForChangeEvent"; function task(taskId: string, status: Task["status"] = "working"): Task { return { @@ -14,11 +15,7 @@ function task(taskId: string, status: Task["status"] = "working"): Task { } function waitForChange(state: ManagedRequestorTasksState): Promise { - return new Promise((resolve) => { - state.addEventListener("tasksChange", (e) => resolve(e.detail), { - once: true, - }); - }); + return waitForChangeEvent(state, "tasksChange"); } describe("ManagedRequestorTasksState", () => { @@ -73,10 +70,9 @@ describe("ManagedRequestorTasksState", () => { taskless.setStatus("connected"); const tasklessState = new ManagedRequestorTasksState(taskless); + const changePromise = waitForChange(tasklessState); taskless.dispatchTypedEvent("connect"); - // Yield once so the async refresh chained off connect runs. - await Promise.resolve(); - await Promise.resolve(); + await changePromise; expect(taskless.listRequestorTasks).not.toHaveBeenCalled(); expect(tasklessState.getTasks()).toEqual([]); }); diff --git a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts index 502e5eed..c0cf63b6 100644 --- a/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts @@ -3,6 +3,7 @@ import type { ResourceTemplate } from "@modelcontextprotocol/sdk/types.js"; import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; import { ManagedResourceTemplatesState } from "@inspector/core/mcp/state/managedResourceTemplatesState"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; +import { waitForChangeEvent } from "./waitForChangeEvent"; function template(name: string): ResourceTemplate { return { uriTemplate: `tpl://{${name}}`, name }; @@ -22,13 +23,7 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { function waitForChange( state: ManagedResourceTemplatesState, ): Promise { - return new Promise((resolve) => { - state.addEventListener( - "resourceTemplatesChange", - (e) => resolve(e.detail), - { once: true }, - ); - }); + return waitForChangeEvent(state, "resourceTemplatesChange"); } describe("ManagedResourceTemplatesState", () => { @@ -90,10 +85,9 @@ describe("ManagedResourceTemplatesState", () => { 0, ); + const changePromise = waitForChange(resourcelessState); resourceless.dispatchTypedEvent("connect"); - // Yield so the async refresh chained off connect runs. - await Promise.resolve(); - await Promise.resolve(); + await changePromise; expect(resourceless.listResourceTemplates).not.toHaveBeenCalled(); expect(resourcelessState.getResourceTemplates()).toEqual([]); }); diff --git a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts index ea510fb2..33d82545 100644 --- a/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedResourcesState.test.ts @@ -3,6 +3,7 @@ import type { Resource } from "@modelcontextprotocol/sdk/types.js"; import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; import { ManagedResourcesState } from "@inspector/core/mcp/state/managedResourcesState"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; +import { waitForChangeEvent } from "./waitForChangeEvent"; function resource(uri: string): Resource { return { uri, name: uri }; @@ -22,19 +23,11 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { function waitForResourcesChange( state: ManagedResourcesState, ): Promise { - return new Promise((resolve) => { - state.addEventListener("resourcesChange", (e) => resolve(e.detail), { - once: true, - }); - }); + return waitForChangeEvent(state, "resourcesChange"); } function waitForListChanged(state: ManagedResourcesState): Promise { - return new Promise((resolve) => { - state.addEventListener("listChangedChange", (e) => resolve(e.detail), { - once: true, - }); - }); + return waitForChangeEvent(state, "listChangedChange"); } describe("ManagedResourcesState", () => { @@ -89,10 +82,9 @@ describe("ManagedResourcesState", () => { resourceless.setStatus("connected"); const resourcelessState = new ManagedResourcesState(resourceless, 0); + const changePromise = waitForResourcesChange(resourcelessState); resourceless.dispatchTypedEvent("connect"); - // Yield so the async refresh chained off connect runs. - await Promise.resolve(); - await Promise.resolve(); + await changePromise; expect(resourceless.listResources).not.toHaveBeenCalled(); expect(resourcelessState.getResources()).toEqual([]); }); diff --git a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts index fd2e143c..dad8d80a 100644 --- a/clients/web/src/test/core/mcp/state/managedToolsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedToolsState.test.ts @@ -3,6 +3,7 @@ import type { Tool } from "@modelcontextprotocol/sdk/types.js"; import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; +import { waitForChangeEvent } from "./waitForChangeEvent"; function tool(name: string): Tool { return { name, inputSchema: { type: "object" } }; @@ -20,19 +21,11 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = { }; function waitForToolsChange(state: ManagedToolsState): Promise { - return new Promise((resolve) => { - state.addEventListener("toolsChange", (e) => resolve(e.detail), { - once: true, - }); - }); + return waitForChangeEvent(state, "toolsChange"); } function waitForListChanged(state: ManagedToolsState): Promise { - return new Promise((resolve) => { - state.addEventListener("listChangedChange", (e) => resolve(e.detail), { - once: true, - }); - }); + return waitForChangeEvent(state, "listChangedChange"); } describe("ManagedToolsState", () => { @@ -84,10 +77,9 @@ describe("ManagedToolsState", () => { toolless.setStatus("connected"); const toollessState = new ManagedToolsState(toolless, 0); + const changePromise = waitForToolsChange(toollessState); toolless.dispatchTypedEvent("connect"); - // Yield so the async refresh chained off connect runs. - await Promise.resolve(); - await Promise.resolve(); + await changePromise; expect(toolless.listTools).not.toHaveBeenCalled(); expect(toollessState.getTools()).toEqual([]); }); diff --git a/clients/web/src/test/core/mcp/state/waitForChangeEvent.test.ts b/clients/web/src/test/core/mcp/state/waitForChangeEvent.test.ts new file mode 100644 index 00000000..868d0a68 --- /dev/null +++ b/clients/web/src/test/core/mcp/state/waitForChangeEvent.test.ts @@ -0,0 +1,47 @@ +import { describe, it, expect } from "vitest"; +import { TypedEventTarget } from "@inspector/core/mcp/typedEventTarget"; +import { waitForChangeEvent } from "./waitForChangeEvent"; + +interface TestEventMap { + ping: number; + poke: void; +} + +describe("waitForChangeEvent", () => { + it("resolves with the event detail when the event fires", async () => { + const target = new TypedEventTarget(); + const pending = waitForChangeEvent(target, "ping", 1000); + target.dispatchTypedEvent("ping", 42); + await expect(pending).resolves.toBe(42); + }); + + it("resolves for a void-detail event", async () => { + // `connect` and other signal-only events carry no payload; the helper must + // still resolve rather than wait for a detail that never comes. CustomEvent + // normalizes an absent detail to null, so that's what the listener sees. + const target = new TypedEventTarget(); + const pending = waitForChangeEvent(target, "poke", 1000); + target.dispatchTypedEvent("poke"); + await expect(pending).resolves.toBeNull(); + }); + + it("rejects with a readable message naming the event on timeout", async () => { + // The point of the helper: a connect-path refresh that never dispatches its + // change event surfaces as a fast, named failure instead of hanging to the + // vitest per-test timeout. + const target = new TypedEventTarget(); + await expect(waitForChangeEvent(target, "ping", 25)).rejects.toThrow( + 'Timed out after 25ms waiting for "ping" event', + ); + }); + + it("does not reject after the event has already resolved", async () => { + const target = new TypedEventTarget(); + const pending = waitForChangeEvent(target, "ping", 25); + target.dispatchTypedEvent("ping", 7); + await expect(pending).resolves.toBe(7); + // Wait past the timeout window; the cleared timer must not reject a + // settled promise (an unhandled rejection would fail the run). + await new Promise((resolve) => setTimeout(resolve, 50)); + }); +}); diff --git a/clients/web/src/test/core/mcp/state/waitForChangeEvent.ts b/clients/web/src/test/core/mcp/state/waitForChangeEvent.ts new file mode 100644 index 00000000..8c6757a4 --- /dev/null +++ b/clients/web/src/test/core/mcp/state/waitForChangeEvent.ts @@ -0,0 +1,52 @@ +import type { + TypedEventTarget, + TypedEventGeneric, +} from "@inspector/core/mcp/typedEventTarget"; + +/** + * Default ceiling for a single change-event wait. Generous relative to the + * near-instant fake-client dispatches these tests drive, but well under the + * vitest per-test timeout so this rejection — not the runner's generic timeout + * — is what surfaces. + */ +const DEFAULT_TIMEOUT_MS = 2000; + +/** + * Resolve when `target` dispatches `type`, rejecting with a readable message if + * the event never arrives within `timeoutMs`. + * + * The timeout is the point of this helper. The connect-path gate tests assert + * that a capability-less / empty-list refresh still dispatches its change event + * (so the listener resolves once the connect-driven refresh has fully run). + * Should a future optimization stop dispatching that redundant event, awaiting + * it would otherwise hang to the vitest timeout with no clue why; here it fails + * fast, naming the event that never fired. + */ +export function waitForChangeEvent< + EventMap extends object, + K extends keyof EventMap, +>( + target: TypedEventTarget, + type: K, + timeoutMs: number = DEFAULT_TIMEOUT_MS, +): Promise { + return new Promise((resolve, reject) => { + const controller = new AbortController(); + const timer = setTimeout(() => { + controller.abort(); + reject( + new Error( + `Timed out after ${timeoutMs}ms waiting for "${String(type)}" event`, + ), + ); + }, timeoutMs); + target.addEventListener( + type, + (event: TypedEventGeneric) => { + clearTimeout(timer); + resolve(event.detail); + }, + { once: true, signal: controller.signal }, + ); + }); +} diff --git a/core/mcp/typedEventTarget.ts b/core/mcp/typedEventTarget.ts index 1ec77d9b..32923b91 100644 --- a/core/mcp/typedEventTarget.ts +++ b/core/mcp/typedEventTarget.ts @@ -6,7 +6,8 @@ /** * Typed event class that extends CustomEvent with type-safe detail. - * For void events, detail is undefined. + * For void events the detail is omitted; CustomEvent normalizes that to `null`, + * so listeners receive `null` (not `undefined`) for such events. */ export class TypedEventGeneric< EventMap extends object,