From 60310e9478cfc58534d2ed242f21d4a08036d463 Mon Sep 17 00:00:00 2001 From: HoberMin <102784200+HoberMin@users.noreply.github.com> Date: Mon, 7 Apr 2025 23:39:02 +0900 Subject: [PATCH 1/3] feat: Add error handling tests for InMemoryTransport --- src/inMemory.test.ts | 98 +++++++++++++++++++++++++++++++++++++++++++- src/inMemory.ts | 4 +- 2 files changed, 99 insertions(+), 3 deletions(-) diff --git a/src/inMemory.test.ts b/src/inMemory.test.ts index baf43446c..0b0d5fe50 100644 --- a/src/inMemory.test.ts +++ b/src/inMemory.test.ts @@ -96,10 +96,43 @@ describe("InMemoryTransport", () => { }); test("should throw error when sending after close", async () => { - await clientTransport.close(); + const [client, server] = InMemoryTransport.createLinkedPair(); + let clientError: Error | undefined; + let serverError: Error | undefined; + + client.onerror = (err) => { + clientError = err; + }; + + server.onerror = (err) => { + serverError = err; + }; + + await client.close(); + + // Attempt to send message from client await expect( - clientTransport.send({ jsonrpc: "2.0", method: "test", id: 1 }), + client.send({ + jsonrpc: "2.0", + method: "test", + id: 1, + }), ).rejects.toThrow("Not connected"); + + // Attempt to send message from server + await expect( + server.send({ + jsonrpc: "2.0", + method: "test", + id: 2, + }), + ).rejects.toThrow("Not connected"); + + // Verify that both sides received errors + expect(clientError).toBeDefined(); + expect(clientError?.message).toBe("Not connected"); + expect(serverError).toBeDefined(); + expect(serverError?.message).toBe("Not connected"); }); test("should queue messages sent before start", async () => { @@ -118,4 +151,65 @@ describe("InMemoryTransport", () => { await serverTransport.start(); expect(receivedMessage).toEqual(message); }); + + describe("error handling", () => { + test("should trigger onerror when sending without connection", async () => { + const transport = new InMemoryTransport(); + let error: Error | undefined; + + transport.onerror = (err) => { + error = err; + }; + + await expect( + transport.send({ + jsonrpc: "2.0", + method: "test", + id: 1, + }), + ).rejects.toThrow("Not connected"); + + expect(error).toBeDefined(); + expect(error?.message).toBe("Not connected"); + }); + + test("should trigger onerror when sending after close", async () => { + const [client, server] = InMemoryTransport.createLinkedPair(); + let clientError: Error | undefined; + let serverError: Error | undefined; + + client.onerror = (err) => { + clientError = err; + }; + + server.onerror = (err) => { + serverError = err; + }; + + await client.close(); + + // Attempt to send message from client + await expect( + client.send({ + jsonrpc: "2.0", + method: "test", + id: 1, + }), + ).rejects.toThrow("Not connected"); + + // Attempt to send message from server + await expect( + server.send({ + jsonrpc: "2.0", + method: "test", + id: 2, + }), + ).rejects.toThrow("Not connected"); + + // Verify that both sides received errors + expect(clientError?.message).toBe("Not connected"); + expect(serverError).toBeDefined(); + expect(serverError?.message).toBe("Not connected"); + }); + }); }); diff --git a/src/inMemory.ts b/src/inMemory.ts index 5dd6e81e0..056a4718d 100644 --- a/src/inMemory.ts +++ b/src/inMemory.ts @@ -51,7 +51,9 @@ export class InMemoryTransport implements Transport { */ async send(message: JSONRPCMessage, options?: { relatedRequestId?: RequestId, authInfo?: AuthInfo }): Promise { if (!this._otherTransport) { - throw new Error("Not connected"); + const error = new Error("Not connected"); + this.onerror?.(error); + throw error; } if (this._otherTransport.onmessage) { From a7dc1258501cde8cc4a20fb7415c82b18e3bb78b Mon Sep 17 00:00:00 2001 From: HoberMin <102784200+HoberMin@users.noreply.github.com> Date: Tue, 8 Apr 2025 09:27:19 +0900 Subject: [PATCH 2/3] feat: improve stdio test Windows compatibility and refactor command logic --- src/client/stdio.test.ts | 6 ++---- src/client/stdio.ts | 9 +++++++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/client/stdio.test.ts b/src/client/stdio.test.ts index b21324469..8c3786eb0 100644 --- a/src/client/stdio.test.ts +++ b/src/client/stdio.test.ts @@ -1,9 +1,7 @@ import { JSONRPCMessage } from "../types.js"; -import { StdioClientTransport, StdioServerParameters } from "./stdio.js"; +import { StdioClientTransport, getDefaultServerParameters } from "./stdio.js"; -const serverParameters: StdioServerParameters = { - command: "/usr/bin/tee", -}; +const serverParameters = getDefaultServerParameters(); test("should start then close cleanly", async () => { const client = new StdioClientTransport(serverParameters); diff --git a/src/client/stdio.ts b/src/client/stdio.ts index 62292ce10..a34e8f196 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -39,6 +39,15 @@ export type StdioServerParameters = { cwd?: string; }; +// Configure default server parameters based on OS +// Uses 'more' command for Windows and 'tee' command for Unix/Linux +export const getDefaultServerParameters = (): StdioServerParameters => { + if (process.platform === "win32") { + return { command: "more" }; + } + return { command: "/usr/bin/tee" }; +}; + /** * Environment variables to inherit by default, if an environment is not explicitly given. */ From 88ede3783e65c6168d5f9237e136e52a116b1a1b Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 8 Jul 2025 15:15:50 +0100 Subject: [PATCH 3/3] Address review feedback: move getDefaultServerParameters to test file - Moved getDefaultServerParameters from stdio.ts to stdio.test.ts since it's only used in tests - Reverted unrelated changes to inMemory.ts and inMemory.test.ts - Kept the core Windows compatibility fix for stdio tests Co-authored-by: HoberMin <99346635+HoberMin@users.noreply.github.com> --- src/client/stdio.test.ts | 11 ++++- src/client/stdio.ts | 9 ---- src/inMemory.test.ts | 98 +--------------------------------------- src/inMemory.ts | 4 +- 4 files changed, 13 insertions(+), 109 deletions(-) diff --git a/src/client/stdio.test.ts b/src/client/stdio.test.ts index 8c3786eb0..2e4d92c25 100644 --- a/src/client/stdio.test.ts +++ b/src/client/stdio.test.ts @@ -1,5 +1,14 @@ import { JSONRPCMessage } from "../types.js"; -import { StdioClientTransport, getDefaultServerParameters } from "./stdio.js"; +import { StdioClientTransport, StdioServerParameters } from "./stdio.js"; + +// Configure default server parameters based on OS +// Uses 'more' command for Windows and 'tee' command for Unix/Linux +const getDefaultServerParameters = (): StdioServerParameters => { + if (process.platform === "win32") { + return { command: "more" }; + } + return { command: "/usr/bin/tee" }; +}; const serverParameters = getDefaultServerParameters(); diff --git a/src/client/stdio.ts b/src/client/stdio.ts index a34e8f196..62292ce10 100644 --- a/src/client/stdio.ts +++ b/src/client/stdio.ts @@ -39,15 +39,6 @@ export type StdioServerParameters = { cwd?: string; }; -// Configure default server parameters based on OS -// Uses 'more' command for Windows and 'tee' command for Unix/Linux -export const getDefaultServerParameters = (): StdioServerParameters => { - if (process.platform === "win32") { - return { command: "more" }; - } - return { command: "/usr/bin/tee" }; -}; - /** * Environment variables to inherit by default, if an environment is not explicitly given. */ diff --git a/src/inMemory.test.ts b/src/inMemory.test.ts index 0b0d5fe50..baf43446c 100644 --- a/src/inMemory.test.ts +++ b/src/inMemory.test.ts @@ -96,43 +96,10 @@ describe("InMemoryTransport", () => { }); test("should throw error when sending after close", async () => { - const [client, server] = InMemoryTransport.createLinkedPair(); - let clientError: Error | undefined; - let serverError: Error | undefined; - - client.onerror = (err) => { - clientError = err; - }; - - server.onerror = (err) => { - serverError = err; - }; - - await client.close(); - - // Attempt to send message from client - await expect( - client.send({ - jsonrpc: "2.0", - method: "test", - id: 1, - }), - ).rejects.toThrow("Not connected"); - - // Attempt to send message from server + await clientTransport.close(); await expect( - server.send({ - jsonrpc: "2.0", - method: "test", - id: 2, - }), + clientTransport.send({ jsonrpc: "2.0", method: "test", id: 1 }), ).rejects.toThrow("Not connected"); - - // Verify that both sides received errors - expect(clientError).toBeDefined(); - expect(clientError?.message).toBe("Not connected"); - expect(serverError).toBeDefined(); - expect(serverError?.message).toBe("Not connected"); }); test("should queue messages sent before start", async () => { @@ -151,65 +118,4 @@ describe("InMemoryTransport", () => { await serverTransport.start(); expect(receivedMessage).toEqual(message); }); - - describe("error handling", () => { - test("should trigger onerror when sending without connection", async () => { - const transport = new InMemoryTransport(); - let error: Error | undefined; - - transport.onerror = (err) => { - error = err; - }; - - await expect( - transport.send({ - jsonrpc: "2.0", - method: "test", - id: 1, - }), - ).rejects.toThrow("Not connected"); - - expect(error).toBeDefined(); - expect(error?.message).toBe("Not connected"); - }); - - test("should trigger onerror when sending after close", async () => { - const [client, server] = InMemoryTransport.createLinkedPair(); - let clientError: Error | undefined; - let serverError: Error | undefined; - - client.onerror = (err) => { - clientError = err; - }; - - server.onerror = (err) => { - serverError = err; - }; - - await client.close(); - - // Attempt to send message from client - await expect( - client.send({ - jsonrpc: "2.0", - method: "test", - id: 1, - }), - ).rejects.toThrow("Not connected"); - - // Attempt to send message from server - await expect( - server.send({ - jsonrpc: "2.0", - method: "test", - id: 2, - }), - ).rejects.toThrow("Not connected"); - - // Verify that both sides received errors - expect(clientError?.message).toBe("Not connected"); - expect(serverError).toBeDefined(); - expect(serverError?.message).toBe("Not connected"); - }); - }); }); diff --git a/src/inMemory.ts b/src/inMemory.ts index 056a4718d..5dd6e81e0 100644 --- a/src/inMemory.ts +++ b/src/inMemory.ts @@ -51,9 +51,7 @@ export class InMemoryTransport implements Transport { */ async send(message: JSONRPCMessage, options?: { relatedRequestId?: RequestId, authInfo?: AuthInfo }): Promise { if (!this._otherTransport) { - const error = new Error("Not connected"); - this.onerror?.(error); - throw error; + throw new Error("Not connected"); } if (this._otherTransport.onmessage) {