Add GH Action for Testing on Windows #132
Conversation
WalkthroughThe changes introduce Windows-specific handling in build workflows, the Makefile, and tests. Workflow files now cache the Windows and non-Windows builds of the simple-responder binary. The Makefile adds a Windows build target. The proxy configuration uses a new shlex library with OS-specific command parsing, and related tests are split into Windows and POSIX files. Helper and process tests are updated for Windows compatibility. Dependency updates reflect the new shlex package. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow (CI)
participant Makefile
participant Cache
participant Build System
Workflow (CI)->>Cache: Restore simple-responder cache (keyed by OS and hash)
alt Cache miss
Workflow (CI)->>Makefile: Build simple-responder (OS-specific target)
Makefile->>Build System: Compile binary (simple-responder or .exe)
Workflow (CI)->>Cache: Save build artifact to cache
else Cache hit
Note right of Workflow (CI): Skip build step
end
Workflow (CI)->>Build System: Run tests
sequenceDiagram
participant SanitizeCommand
participant OS
participant shlex.Posix
participant shlex.Windows
SanitizeCommand->>OS: Check operating system
alt Windows
SanitizeCommand->>shlex.Windows: Split command string
else POSIX
SanitizeCommand->>shlex.Posix: Split command string
end
SanitizeCommand-->>Caller: Return argument slice
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
proxy/config_windows_test.go (2)
11-12: Consider adding an explicit test for single quotes behavior.The comment indicates single quotes aren't supported like in the POSIX version, but there's no explicit test case demonstrating this difference. Adding a test case for single-quoted strings would better document the expected Windows behavior.
func TestConfig_SanitizeCommand(t *testing.T) { // does not support single quoted strings like in config_posix_test.go + // Test single quotes are treated literally on Windows, not as string delimiters + singleQuoteArgs, singleQuoteErr := SanitizeCommand(`python -a 'single quotes'`) + assert.NoError(t, singleQuoteErr) + assert.Equal(t, []string{"python", "-a", "'single", "quotes'"}, singleQuoteArgs) + args, err := SanitizeCommand(`python model1.py \
26-26: Clarify the comment about lexer behavior with embedded quotes.The current comment notes confusion about the lexer's handling of embedded quotes. Consider updating this comment to explain the expected Windows behavior more clearly, as this will help future developers understand the intention.
- "--arg4", "'string in string'", // this is a little weird but the lexer says so...? + "--arg4", "'string in string'", // On Windows, shlex.Windows.Split preserves single quotes but removes double quotes when they're inside other quotes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.github/workflows/go-ci-windows.yml(2 hunks).github/workflows/go-ci.yml(1 hunks)Makefile(1 hunks)go.mod(1 hunks)proxy/config.go(2 hunks)proxy/config_posix_test.go(1 hunks)proxy/config_test.go(0 hunks)proxy/config_windows_test.go(1 hunks)proxy/helpers_test.go(1 hunks)proxy/process_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- proxy/config_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
proxy/config_windows_test.go (2)
proxy/config_posix_test.go (1)
TestConfig_SanitizeCommand(11-36)proxy/config.go (1)
SanitizeCommand(231-250)
proxy/config_posix_test.go (2)
proxy/config_windows_test.go (1)
TestConfig_SanitizeCommand(11-35)proxy/config.go (1)
SanitizeCommand(231-250)
🔇 Additional comments (18)
Makefile (1)
49-51: Good addition of Windows support.The new
simple-responder-windowstarget correctly builds the simple-responder binary for Windows. This aligns well with the PR's objective of improving Windows testing compatibility.proxy/process_test.go (2)
8-8: LGTM: Runtime package import added.The runtime package import is necessary for the platform-specific error handling later in the file.
436-440: Good platform-specific error handling.The conditional check for different error messages based on the runtime.GOOS value is an excellent approach to handle platform-specific behavior when forcibly terminating a process. Windows and POSIX systems report different errors in this scenario.
go.mod (1)
15-15:❓ Verification inconclusive
Verify compatibility of the new dependency.
The addition of the
github.com/billziss-gh/golibdependency appears to support cross-platform command parsing via its shlex package. However, this repository hasn't been updated in a while (last commit in 2019).
🏁 Script executed:
#!/bin/bash # Check the imported library's activity status echo "Checking github.com/billziss-gh/golib repository info:" echo "====================================================" curl -s https://api.github.com/repos/billziss-gh/golib | \ jq '{name: .name, description: .description, stars: .stargazers_count, forks: .forks_count, open_issues: .open_issues_count, updated_at: .updated_at, pushed_at: .pushed_at}' echo -e "\nChecking where shlex is used in the codebase:" echo "====================================================" rg -A 3 "billziss-gh/golib/shlex" --type goLength of output: 1053
Confirm golib v0.2.0 is appropriate for shlex parsing
Our check shows that
github.com/billziss-gh/golibhas low community traction (9 stars, 2 forks), no open issues, and its last code push was in March 2021. Theshlexpackage from this module is imported in your code:
- proxy/config.go:
import "github.com/billziss-gh/golib/shlex"Please ensure v0.2.0 meets your cross-platform parsing requirements. In particular:
- Vendor or pin the module to avoid unexpected upstream changes.
- Add integration tests covering all target shells/OSes.
- If long-term maintenance is a concern, evaluate alternative shlex implementations with more active support.
.github/workflows/go-ci.yml (2)
25-32: Good caching implementation.Adding a cache for the simple-responder build based on the hash of its source file is an excellent optimization that will speed up CI workflows by avoiding redundant builds.
37-44: LGTM: Conditional cache save step.The conditional save step that only runs when the cache was missed is well-implemented. The comment on line 38 explains the purpose clearly.
proxy/helpers_test.go (1)
48-53: Good OS-specific path handlingThe change correctly handles different path formats for Windows vs. other operating systems. This is essential for cross-platform testing and aligns with the Windows-specific build target in the Makefile.
.github/workflows/go-ci-windows.yml (4)
5-5: Updated branch trigger to "main"The workflow now triggers on the main branch instead of windows-tests, ensuring Windows tests run on main branch commits and PRs. This is an important improvement for maintaining Windows compatibility.
Also applies to: 8-8
25-32: Good caching implementationAdding caching for the simple-responder binary will significantly improve workflow performance by avoiding unnecessary rebuilds. The cache key based on the source file hash ensures rebuilds happen only when the source changes.
34-37: Improved Windows-specific buildThe build step now:
- Only runs when the cache is missed
- Uses the Windows-specific Makefile target
This ensures efficient and correct building of the Windows executable.
39-46: Proper cache savingThis step correctly saves the build artifacts to the cache, maintaining consistency with the restore step and only running when necessary. The conditional execution ensures we don't waste time on redundant operations.
proxy/config.go (2)
7-7: Changed dependency for better Windows compatibilitySwitching from google/shlex to billziss-gh/golib/shlex and adding runtime import to support OS-specific command parsing. This change is crucial for the PR's goal of improving Windows compatibility.
Also applies to: 12-12
237-242:❓ Verification inconclusive
OS-specific command parsing
This change properly handles command parsing differences between Windows and POSIX systems. Windows uses different command-line parsing rules, and this implementation respects those differences.
As mentioned in the PR description, this change might break existing Windows path configurations.
Verify if the Windows command splitting behavior correctly handles your Windows paths:
🏁 Script executed:
#!/bin/bash # Check if any existing Windows path configurations in YAML files might break with the new parsing echo "Searching for command configuration in YAML files that might contain Windows paths..." rg -A 5 "cmd:.*[A-Za-z]:[\\\/]" --glob "*.yml" --glob "*.yaml"Length of output: 244
To ensure we catch any Windows-style paths (not just under
cmd:), let’s scan all YAML files for drive-letter paths:
🏁 Script executed:
#!/bin/bash echo "Searching for any Windows-style drive paths in YAML files..." rg '[A-Za-z]:\\' --glob '*.yml' --glob '*.yaml'Length of output: 180
OS-specific command parsing validated – no Windows-style paths found in YAML
I scanned all YAML files for Windows drive-letter paths (e.g.
C:\…) and didn’t detect any instances that would be affected by the newshlex.Windows.Splitlogic. It appears safe to merge, but please double-check if you have any custom Windows path configurations in:
- Other config formats (JSON, TOML, etc.)
- Inline scripts or documentation examples
If you do have Windows paths elsewhere, ensure they’re compatible with the new parsing behavior.
proxy/config_posix_test.go (2)
1-2: Appropriate build constraintThe build constraint ensures this test only runs on non-Windows platforms, preventing false failures on Windows where command parsing rules differ.
11-36: Comprehensive command parsing testThis test thoroughly validates POSIX-specific command parsing behavior, including:
- Multi-line commands
- Double and single quotes
- Nested quotes
- Empty command handling
This is well-paired with the corresponding Windows test and ensures the command parsing works correctly across platforms.
One observation: the Windows test mentioned in the relevant code snippets doesn't support single quotes in the same way. This difference is expected and correctly handled in the separate test files.
proxy/config_windows_test.go (3)
1-2: Appropriate use of build constraint.The build tag correctly ensures this test file only runs on Windows platforms, which is essential for OS-specific testing.
13-19: LGTM. Command string formatting is appropriate for Windows testing.The test input correctly uses double quotes for string delimitation, which matches Windows command-line behavior.
30-33: LGTM. Empty command test properly handles the error case.The test correctly verifies that empty commands return an error and nil arguments, matching the implementation in SanitizeCommand.
| //go:build windows | ||
|
|
||
| package proxy | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestConfig_SanitizeCommand(t *testing.T) { | ||
| // does not support single quoted strings like in config_posix_test.go | ||
| args, err := SanitizeCommand(`python model1.py \ | ||
| -a "double quotes" \ | ||
| -s | ||
| --arg3 123 \ | ||
| --arg4 '"string in string"' | ||
| -c "'single quoted'" | ||
| `) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, []string{ | ||
| "python", "model1.py", | ||
| "-a", "double quotes", | ||
| "-s", | ||
| "--arg3", "123", | ||
| "--arg4", "'string in string'", // this is a little weird but the lexer says so...? | ||
| "-c", `'single quoted'`, | ||
| }, args) | ||
|
|
||
| // Test an empty command | ||
| args, err = SanitizeCommand("") | ||
| assert.Error(t, err) | ||
| assert.Nil(t, args) | ||
|
|
||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding Windows-specific path tests.
Since this PR aims to improve Windows path compatibility, consider adding tests specifically for Windows path handling with backslashes, which often cause issues due to their use as escape characters.
func TestConfig_SanitizeCommand(t *testing.T) {
// existing tests...
+ // Test Windows path handling
+ pathArgs, pathErr := SanitizeCommand(`python "C:\Program Files\script.py" --input "D:\Data Files\input.txt"`)
+ assert.NoError(t, pathErr)
+ assert.Equal(t, []string{
+ "python", "C:\\Program Files\\script.py",
+ "--input", "D:\\Data Files\\input.txt",
+ }, pathArgs)
}Let's verify how the current Windows command parsing handles backslashes in paths:
🏁 Script executed:
#!/bin/bash
# Check how shlex library is imported and used
grep -r "import \"github.com/mattn/go-shellwords\"" --include="*.go" .
grep -r "shlex" --include="*.go" .Length of output: 269
Add Windows-specific backslash path tests in proxy/config_windows_test.go
To ensure SanitizeCommand correctly handles Windows paths (which include backslashes), add a test case in TestConfig_SanitizeCommand that exercises typical C:\… and D:\… paths.
• File: proxy/config_windows_test.go
Location: inside the existing func TestConfig_SanitizeCommand(t *testing.T) { … } block
func TestConfig_SanitizeCommand(t *testing.T) {
// existing tests…
+ // Windows path handling with backslashes
+ pathArgs, pathErr := SanitizeCommand(`python "C:\Program Files\script.py" --input "D:\Data Files\input.txt"`)
+ assert.NoError(t, pathErr)
+ assert.Equal(t, []string{
+ "python", `C:\Program Files\script.py`,
+ "--input", `D:\Data Files\input.txt`,
+ }, pathArgs)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //go:build windows | |
| package proxy | |
| import ( | |
| "testing" | |
| "github.com/stretchr/testify/assert" | |
| ) | |
| func TestConfig_SanitizeCommand(t *testing.T) { | |
| // does not support single quoted strings like in config_posix_test.go | |
| args, err := SanitizeCommand(`python model1.py \ | |
| -a "double quotes" \ | |
| -s | |
| --arg3 123 \ | |
| --arg4 '"string in string"' | |
| -c "'single quoted'" | |
| `) | |
| assert.NoError(t, err) | |
| assert.Equal(t, []string{ | |
| "python", "model1.py", | |
| "-a", "double quotes", | |
| "-s", | |
| "--arg3", "123", | |
| "--arg4", "'string in string'", // this is a little weird but the lexer says so...? | |
| "-c", `'single quoted'`, | |
| }, args) | |
| // Test an empty command | |
| args, err = SanitizeCommand("") | |
| assert.Error(t, err) | |
| assert.Nil(t, args) | |
| } | |
| func TestConfig_SanitizeCommand(t *testing.T) { | |
| // does not support single quoted strings like in config_posix_test.go | |
| args, err := SanitizeCommand(`python model1.py \ | |
| -a "double quotes" \ | |
| -s | |
| --arg3 123 \ | |
| --arg4 '"string in string"' | |
| -c "'single quoted'" | |
| `) | |
| assert.NoError(t, err) | |
| assert.Equal(t, []string{ | |
| "python", "model1.py", | |
| "-a", "double quotes", | |
| "-s", | |
| "--arg3", "123", | |
| "--arg4", "'string in string'", // this is a little weird but the lexer says so...? | |
| "-c", `'single quoted'`, | |
| }, args) | |
| // Test an empty command | |
| args, err = SanitizeCommand("") | |
| assert.Error(t, err) | |
| assert.Nil(t, args) | |
| // Windows path handling with backslashes | |
| pathArgs, pathErr := SanitizeCommand(`python "C:\Program Files\script.py" --input "D:\Data Files\input.txt"`) | |
| assert.NoError(t, pathErr) | |
| assert.Equal(t, []string{ | |
| "python", `C:\Program Files\script.py`, | |
| "--input", `D:\Data Files\input.txt`, | |
| }, pathArgs) | |
| } |
🤖 Prompt for AI Agents
In proxy/config_windows_test.go within the TestConfig_SanitizeCommand function
(lines 1-35), add a new test case that includes Windows-style paths with
backslashes, such as "C:\\path\\to\\file" and "D:\\another\\path", to verify
that SanitizeCommand correctly parses these paths without misinterpreting
backslashes as escape characters. This will improve test coverage for
Windows-specific path handling and ensure the function behaves as expected on
Windows systems.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests