Test/1396 harden managed state connect tests (#1508)
* 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
97b1a7fbff
commit
c60a128df3
8 changed files with 124 additions and 58 deletions
|
|
@ -3,6 +3,7 @@ import type { Prompt } from "@modelcontextprotocol/sdk/types.js";
|
||||||
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
||||||
import { ManagedPromptsState } from "@inspector/core/mcp/state/managedPromptsState";
|
import { ManagedPromptsState } from "@inspector/core/mcp/state/managedPromptsState";
|
||||||
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
||||||
|
import { waitForChangeEvent } from "./waitForChangeEvent";
|
||||||
|
|
||||||
function prompt(name: string): Prompt {
|
function prompt(name: string): Prompt {
|
||||||
return { name };
|
return { name };
|
||||||
|
|
@ -20,19 +21,11 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = {
|
||||||
};
|
};
|
||||||
|
|
||||||
function waitForPromptsChange(state: ManagedPromptsState): Promise<Prompt[]> {
|
function waitForPromptsChange(state: ManagedPromptsState): Promise<Prompt[]> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "promptsChange");
|
||||||
state.addEventListener("promptsChange", (e) => resolve(e.detail), {
|
|
||||||
once: true,
|
|
||||||
});
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function waitForListChanged(state: ManagedPromptsState): Promise<boolean> {
|
function waitForListChanged(state: ManagedPromptsState): Promise<boolean> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "listChangedChange");
|
||||||
state.addEventListener("listChangedChange", (e) => resolve(e.detail), {
|
|
||||||
once: true,
|
|
||||||
});
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
describe("ManagedPromptsState", () => {
|
describe("ManagedPromptsState", () => {
|
||||||
|
|
@ -84,10 +77,9 @@ describe("ManagedPromptsState", () => {
|
||||||
promptless.setStatus("connected");
|
promptless.setStatus("connected");
|
||||||
const promptlessState = new ManagedPromptsState(promptless, 0);
|
const promptlessState = new ManagedPromptsState(promptless, 0);
|
||||||
|
|
||||||
|
const changePromise = waitForPromptsChange(promptlessState);
|
||||||
promptless.dispatchTypedEvent("connect");
|
promptless.dispatchTypedEvent("connect");
|
||||||
// Yield so the async refresh chained off connect runs.
|
await changePromise;
|
||||||
await Promise.resolve();
|
|
||||||
await Promise.resolve();
|
|
||||||
expect(promptless.listPrompts).not.toHaveBeenCalled();
|
expect(promptless.listPrompts).not.toHaveBeenCalled();
|
||||||
expect(promptlessState.getPrompts()).toEqual([]);
|
expect(promptlessState.getPrompts()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach } from "vitest";
|
||||||
import type { Task } from "@modelcontextprotocol/sdk/types.js";
|
import type { Task } from "@modelcontextprotocol/sdk/types.js";
|
||||||
import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedRequestorTasksState";
|
import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedRequestorTasksState";
|
||||||
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
||||||
|
import { waitForChangeEvent } from "./waitForChangeEvent";
|
||||||
|
|
||||||
function task(taskId: string, status: Task["status"] = "working"): Task {
|
function task(taskId: string, status: Task["status"] = "working"): Task {
|
||||||
return {
|
return {
|
||||||
|
|
@ -14,11 +15,7 @@ function task(taskId: string, status: Task["status"] = "working"): Task {
|
||||||
}
|
}
|
||||||
|
|
||||||
function waitForChange(state: ManagedRequestorTasksState): Promise<Task[]> {
|
function waitForChange(state: ManagedRequestorTasksState): Promise<Task[]> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "tasksChange");
|
||||||
state.addEventListener("tasksChange", (e) => resolve(e.detail), {
|
|
||||||
once: true,
|
|
||||||
});
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
describe("ManagedRequestorTasksState", () => {
|
describe("ManagedRequestorTasksState", () => {
|
||||||
|
|
@ -73,10 +70,9 @@ describe("ManagedRequestorTasksState", () => {
|
||||||
taskless.setStatus("connected");
|
taskless.setStatus("connected");
|
||||||
const tasklessState = new ManagedRequestorTasksState(taskless);
|
const tasklessState = new ManagedRequestorTasksState(taskless);
|
||||||
|
|
||||||
|
const changePromise = waitForChange(tasklessState);
|
||||||
taskless.dispatchTypedEvent("connect");
|
taskless.dispatchTypedEvent("connect");
|
||||||
// Yield once so the async refresh chained off connect runs.
|
await changePromise;
|
||||||
await Promise.resolve();
|
|
||||||
await Promise.resolve();
|
|
||||||
expect(taskless.listRequestorTasks).not.toHaveBeenCalled();
|
expect(taskless.listRequestorTasks).not.toHaveBeenCalled();
|
||||||
expect(tasklessState.getTasks()).toEqual([]);
|
expect(tasklessState.getTasks()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,7 @@ import type { ResourceTemplate } from "@modelcontextprotocol/sdk/types.js";
|
||||||
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
||||||
import { ManagedResourceTemplatesState } from "@inspector/core/mcp/state/managedResourceTemplatesState";
|
import { ManagedResourceTemplatesState } from "@inspector/core/mcp/state/managedResourceTemplatesState";
|
||||||
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
||||||
|
import { waitForChangeEvent } from "./waitForChangeEvent";
|
||||||
|
|
||||||
function template(name: string): ResourceTemplate {
|
function template(name: string): ResourceTemplate {
|
||||||
return { uriTemplate: `tpl://{${name}}`, name };
|
return { uriTemplate: `tpl://{${name}}`, name };
|
||||||
|
|
@ -22,13 +23,7 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = {
|
||||||
function waitForChange(
|
function waitForChange(
|
||||||
state: ManagedResourceTemplatesState,
|
state: ManagedResourceTemplatesState,
|
||||||
): Promise<ResourceTemplate[]> {
|
): Promise<ResourceTemplate[]> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "resourceTemplatesChange");
|
||||||
state.addEventListener(
|
|
||||||
"resourceTemplatesChange",
|
|
||||||
(e) => resolve(e.detail),
|
|
||||||
{ once: true },
|
|
||||||
);
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
describe("ManagedResourceTemplatesState", () => {
|
describe("ManagedResourceTemplatesState", () => {
|
||||||
|
|
@ -90,10 +85,9 @@ describe("ManagedResourceTemplatesState", () => {
|
||||||
0,
|
0,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
const changePromise = waitForChange(resourcelessState);
|
||||||
resourceless.dispatchTypedEvent("connect");
|
resourceless.dispatchTypedEvent("connect");
|
||||||
// Yield so the async refresh chained off connect runs.
|
await changePromise;
|
||||||
await Promise.resolve();
|
|
||||||
await Promise.resolve();
|
|
||||||
expect(resourceless.listResourceTemplates).not.toHaveBeenCalled();
|
expect(resourceless.listResourceTemplates).not.toHaveBeenCalled();
|
||||||
expect(resourcelessState.getResourceTemplates()).toEqual([]);
|
expect(resourcelessState.getResourceTemplates()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,7 @@ import type { Resource } from "@modelcontextprotocol/sdk/types.js";
|
||||||
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
||||||
import { ManagedResourcesState } from "@inspector/core/mcp/state/managedResourcesState";
|
import { ManagedResourcesState } from "@inspector/core/mcp/state/managedResourcesState";
|
||||||
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
||||||
|
import { waitForChangeEvent } from "./waitForChangeEvent";
|
||||||
|
|
||||||
function resource(uri: string): Resource {
|
function resource(uri: string): Resource {
|
||||||
return { uri, name: uri };
|
return { uri, name: uri };
|
||||||
|
|
@ -22,19 +23,11 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = {
|
||||||
function waitForResourcesChange(
|
function waitForResourcesChange(
|
||||||
state: ManagedResourcesState,
|
state: ManagedResourcesState,
|
||||||
): Promise<Resource[]> {
|
): Promise<Resource[]> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "resourcesChange");
|
||||||
state.addEventListener("resourcesChange", (e) => resolve(e.detail), {
|
|
||||||
once: true,
|
|
||||||
});
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function waitForListChanged(state: ManagedResourcesState): Promise<boolean> {
|
function waitForListChanged(state: ManagedResourcesState): Promise<boolean> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "listChangedChange");
|
||||||
state.addEventListener("listChangedChange", (e) => resolve(e.detail), {
|
|
||||||
once: true,
|
|
||||||
});
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
describe("ManagedResourcesState", () => {
|
describe("ManagedResourcesState", () => {
|
||||||
|
|
@ -89,10 +82,9 @@ describe("ManagedResourcesState", () => {
|
||||||
resourceless.setStatus("connected");
|
resourceless.setStatus("connected");
|
||||||
const resourcelessState = new ManagedResourcesState(resourceless, 0);
|
const resourcelessState = new ManagedResourcesState(resourceless, 0);
|
||||||
|
|
||||||
|
const changePromise = waitForResourcesChange(resourcelessState);
|
||||||
resourceless.dispatchTypedEvent("connect");
|
resourceless.dispatchTypedEvent("connect");
|
||||||
// Yield so the async refresh chained off connect runs.
|
await changePromise;
|
||||||
await Promise.resolve();
|
|
||||||
await Promise.resolve();
|
|
||||||
expect(resourceless.listResources).not.toHaveBeenCalled();
|
expect(resourceless.listResources).not.toHaveBeenCalled();
|
||||||
expect(resourcelessState.getResources()).toEqual([]);
|
expect(resourcelessState.getResources()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,7 @@ import type { Tool } from "@modelcontextprotocol/sdk/types.js";
|
||||||
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
|
||||||
import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState";
|
import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState";
|
||||||
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
|
||||||
|
import { waitForChangeEvent } from "./waitForChangeEvent";
|
||||||
|
|
||||||
function tool(name: string): Tool {
|
function tool(name: string): Tool {
|
||||||
return { name, inputSchema: { type: "object" } };
|
return { name, inputSchema: { type: "object" } };
|
||||||
|
|
@ -20,19 +21,11 @@ const AUTO_REFRESH_SETTINGS: InspectorServerSettings = {
|
||||||
};
|
};
|
||||||
|
|
||||||
function waitForToolsChange(state: ManagedToolsState): Promise<Tool[]> {
|
function waitForToolsChange(state: ManagedToolsState): Promise<Tool[]> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "toolsChange");
|
||||||
state.addEventListener("toolsChange", (e) => resolve(e.detail), {
|
|
||||||
once: true,
|
|
||||||
});
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function waitForListChanged(state: ManagedToolsState): Promise<boolean> {
|
function waitForListChanged(state: ManagedToolsState): Promise<boolean> {
|
||||||
return new Promise((resolve) => {
|
return waitForChangeEvent(state, "listChangedChange");
|
||||||
state.addEventListener("listChangedChange", (e) => resolve(e.detail), {
|
|
||||||
once: true,
|
|
||||||
});
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
describe("ManagedToolsState", () => {
|
describe("ManagedToolsState", () => {
|
||||||
|
|
@ -84,10 +77,9 @@ describe("ManagedToolsState", () => {
|
||||||
toolless.setStatus("connected");
|
toolless.setStatus("connected");
|
||||||
const toollessState = new ManagedToolsState(toolless, 0);
|
const toollessState = new ManagedToolsState(toolless, 0);
|
||||||
|
|
||||||
|
const changePromise = waitForToolsChange(toollessState);
|
||||||
toolless.dispatchTypedEvent("connect");
|
toolless.dispatchTypedEvent("connect");
|
||||||
// Yield so the async refresh chained off connect runs.
|
await changePromise;
|
||||||
await Promise.resolve();
|
|
||||||
await Promise.resolve();
|
|
||||||
expect(toolless.listTools).not.toHaveBeenCalled();
|
expect(toolless.listTools).not.toHaveBeenCalled();
|
||||||
expect(toollessState.getTools()).toEqual([]);
|
expect(toollessState.getTools()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -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<TestEventMap>();
|
||||||
|
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<TestEventMap>();
|
||||||
|
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<TestEventMap>();
|
||||||
|
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<TestEventMap>();
|
||||||
|
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));
|
||||||
|
});
|
||||||
|
});
|
||||||
52
clients/web/src/test/core/mcp/state/waitForChangeEvent.ts
Normal file
52
clients/web/src/test/core/mcp/state/waitForChangeEvent.ts
Normal file
|
|
@ -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<EventMap>,
|
||||||
|
type: K,
|
||||||
|
timeoutMs: number = DEFAULT_TIMEOUT_MS,
|
||||||
|
): Promise<EventMap[K]> {
|
||||||
|
return new Promise<EventMap[K]>((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<EventMap, K>) => {
|
||||||
|
clearTimeout(timer);
|
||||||
|
resolve(event.detail);
|
||||||
|
},
|
||||||
|
{ once: true, signal: controller.signal },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
@ -6,7 +6,8 @@
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Typed event class that extends CustomEvent with type-safe detail.
|
* 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<
|
export class TypedEventGeneric<
|
||||||
EventMap extends object,
|
EventMap extends object,
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue