Improve stdio test Windows compatibility and refactor command logic #284
Conversation
…lt-tool-json fix: When tool type cannot be determined, use DynamicJsonForm
There was a problem hiding this comment.
Hi @HoberMin thank you for putting up this PR!
The core change looks good to me and after trying it out on Windows, I can confirm it fixes stdio tests there, which is great:
| Without #284 | With #284 |
|---|---|
 |
 |
2 Small asks before we can merge:
- Could we please move
getDefaultServerParametersinto the test file directly? It's unused outside of tests, so we shouldn't expose it. - There seem to be some unrelated changes to
inMemoryand associated tests; could you confirm that these are indeed unrelated and submit a separate PR for these changes, explaining the rationale in the description? Happy to re-review when that is ready.
| // 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" }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Can we move this to src/client/stdio.test.ts? It doesn't seem to be used outside of tests anywhere.
| const error = new Error("Not connected"); | ||
| this.onerror?.(error); | ||
| throw error; |
There was a problem hiding this comment.
Ditto regarding the inMemory test change - this seems unrelated so should ideally be in a different PR.
| @@ -69,10 +69,43 @@ describe("InMemoryTransport", () => { | |||
| }); | |||
|
|
|||
| test("should throw error when sending after close", async () => { | |||
| await clientTransport.close(); | |||
| const [client, server] = InMemoryTransport.createLinkedPair(); | |||
There was a problem hiding this comment.
The changes in this file seem unrelated to anything in the PR description - were these accidentally included? If not, could you provide some color why they are needed and relevant to the windows compatibility change?
Could we make this a separate PR to isolate the windows change from this unrelated one? Happy to re-review when that is ready.
Description
This PR improves the Windows compatibility of stdio tests and refactors the OS-specific command logic. The main changes include:
getDefaultServerParametersfunction for cross-platform supportmorecommand for Windows instead ofteeMotivation and Context
The stdio tests were not properly handling Windows environments, and the OS-specific logic was mixed in the test file. This PR improves code organization and cross-platform compatibility.
Type of changes
How Has This Been Tested?
Checklist: