Skip to content

Fix cross-platform process creation in tests: use cmd /c more on Windows #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
edburns merged 2 commits into from
Apr 11, 2026
+14 −4
Merged

Fix cross-platform process creation in tests: use cmd /c more on Windows #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62ff965
Initial plan
Copilot Apr 11, 2026
20acdbf
Fix cross-platform process creation in tests: use cmd /c more on Windows
Copilot Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/test/java/com/github/copilot/sdk/CliServerManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,18 @@ void connectToServerTcpMode() throws Exception {
}
}

private static Process startBlockingProcess() throws IOException {
boolean isWindows = System.getProperty("os.name").toLowerCase().contains("windows");
return (isWindows ? new ProcessBuilder("cmd", "/c", "more") : new ProcessBuilder("cat")).start();
}
Comment on lines +73 to +76
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This startBlockingProcess() helper is identical to the one added in JsonRpcClientTest. Consider extracting it into a shared test utility to avoid duplication and keep the OS-specific command logic consistent across tests.

Copilot uses AI. Check for mistakes.

@Test
void connectToServerStdioMode() throws Exception {
var options = new CopilotClientOptions();
var manager = new CliServerManager(options);

// Create a dummy process for stdio mode
Process process = new ProcessBuilder("cat").start();
Process process = startBlockingProcess();
try {
JsonRpcClient client = manager.connectToServer(process, null, null);
assertNotNull(client);
Expand Down
11 changes: 8 additions & 3 deletions src/test/java/com/github/copilot/sdk/JsonRpcClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,17 +133,22 @@ void testIsConnectedWithSocketClosed() throws Exception {
pair.serverSocket.close();
}

private static Process startBlockingProcess() throws IOException {
boolean isWindows = System.getProperty("os.name").toLowerCase().contains("windows");
return (isWindows ? new ProcessBuilder("cmd", "/c", "more") : new ProcessBuilder("cat")).start();
}
Comment on lines +136 to +139
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The startBlockingProcess() helper is duplicated here and in CliServerManagerTest. Consider extracting it into a shared test utility (e.g., a package-private helper in src/test/java/com/github/copilot/sdk/) so future tweaks to the blocking command/OS detection only need to be made in one place.

Copilot uses AI. Check for mistakes.

@Test
void testIsConnectedWithProcess() throws Exception {
Process proc = new ProcessBuilder("cat").start();
Process proc = startBlockingProcess();
try (var client = JsonRpcClient.fromProcess(proc)) {
assertTrue(client.isConnected());
}
}

@Test
void testIsConnectedWithProcessDead() throws Exception {
Process proc = new ProcessBuilder("cat").start();
Process proc = startBlockingProcess();
var client = JsonRpcClient.fromProcess(proc);
proc.destroy();
proc.waitFor(5, TimeUnit.SECONDS);
Expand All @@ -155,7 +160,7 @@ void testIsConnectedWithProcessDead() throws Exception {

@Test
void testGetProcessReturnsProcess() throws Exception {
Process proc = new ProcessBuilder("cat").start();
Process proc = startBlockingProcess();
try (var client = JsonRpcClient.fromProcess(proc)) {
assertSame(proc, client.getProcess());
}
Expand Down
Loading