Skip to content

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

Merged
edburns merged 2 commits intomainfrom
copilot/fix-code-for-review-comments
Apr 11, 2026
Merged

Fix cross-platform process creation in tests: use cmd /c more on Windows#67
edburns merged 2 commits intomainfrom
copilot/fix-code-for-review-comments

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 11, 2026

new ProcessBuilder("cat").start() is Unix-only. On Windows, cat doesn't exist, and using cmd /c type NUL as a substitute exits immediately rather than blocking on stdin — making any test that relies on process.isAlive() inherently flaky.

Before the change?

  • JsonRpcClientTest and CliServerManagerTest used new ProcessBuilder("cat").start() to create a long-lived process for testing, which only works on Unix.

After the change?

  • A startBlockingProcess() helper is added to both test classes. It selects the right command at runtime — cmd /c more on Windows (blocks on stdin until destroyed) and cat on Unix — ensuring consistent cross-platform behaviour.
  • All 4 occurrences of new ProcessBuilder("cat").start() across the two test files are replaced:
    • JsonRpcClientTesttestIsConnectedWithProcess, testIsConnectedWithProcessDead, testGetProcessReturnsProcess
    • CliServerManagerTestconnectToServerStdioMode
  • No other ProcessBuilder usages in the test suite required this fix (remaining usages invoke the real Copilot CLI or the npx tsx proxy server).
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();
}

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • mvn spotless:apply has been run to format the code
  • mvn clean verify passes locally

Does this introduce a breaking change?

  • Yes
  • No

Copilot AI changed the title [WIP] Fix code for all comments in review thread Fix cross-platform process creation in tests: use cmd /c more on Windows Apr 11, 2026
Copilot AI requested a review from edburns April 11, 2026 15:08
@edburns
Copy link
Copy Markdown
Collaborator

edburns commented Apr 11, 2026

@copilot did you get all the occurrences?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 11, 2026

@copilot did you get all the occurrences?

Yes — all occurrences are covered. The only ProcessBuilder("cat") calls in the test suite were in JsonRpcClientTest (3 uses) and CliServerManagerTest (1 use), all of which are now replaced with the cross-platform startBlockingProcess() helper. The remaining ProcessBuilder usages in other tests (CopilotClientTest, MetadataApiTest, E2ETestContext) all invoke the real copilot CLI and don't need this fix.

@edburns edburns marked this pull request as ready for review April 11, 2026 15:22
Copilot AI review requested due to automatic review settings April 11, 2026 15:22
@edburns edburns merged commit 15b3712 into main Apr 11, 2026
11 checks passed
@edburns edburns deleted the copilot/fix-code-for-review-comments branch April 11, 2026 15:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates unit tests to create a long-lived dummy stdio process in a cross-platform way (Windows vs Unix), improving reliability of process.isAlive()-based assertions in the SDK’s process-backed JSON-RPC client and CLI server manager tests.

Changes:

  • Added a startBlockingProcess() helper in JsonRpcClientTest to use cmd /c more on Windows and cat elsewhere.
  • Added the same helper in CliServerManagerTest and replaced the stdio dummy process creation.
  • Replaced all occurrences of new ProcessBuilder("cat").start() in the affected tests.
Show a summary per file
File Description
src/test/java/com/github/copilot/sdk/JsonRpcClientTest.java Uses an OS-specific blocking process helper to make process-backed connection tests work on Windows.
src/test/java/com/github/copilot/sdk/CliServerManagerTest.java Uses the same blocking process helper for stdio-mode connection testing on Windows.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment on lines +136 to +139
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();
}
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.
Comment on lines +73 to +76
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();
}
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants