fix(test): resolve Windows test failures and isolate environments#962
fix(test): resolve Windows test failures and isolate environments#962decode2 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTests were updated for Windows-specific paths, binary names, environment variables, and symlink behavior across the CLI, component, TUI, and update flows. The install script helper now reads environment values with ChangesWindows compatibility updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/install_script_test.go`:
- Around line 116-120: The test helper in install_script_test.go is writing to a
fixed relative filename, which can collide with leftover files and is not
isolated for parallel runs. Update the temp script creation used around
os.WriteFile and defer os.Remove to use a unique path from t.TempDir() instead
of the hardcoded script_test.sh name, keeping the rest of the test flow the
same.
In `@scripts/install.sh`:
- Line 314: The current env-var lookup in the install script uses eval in a way
that trips set -u when the referenced variable is unset. Update the current
value expansion in the relevant install logic to use an explicit default so
unset variables resolve to empty instead of aborting, while keeping the existing
empty-check behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86918576-ab1d-4d5c-acfd-3b150dca151e
📒 Files selected for processing (10)
internal/cli/run_engram_download_test.gointernal/components/engram/download_test.gointernal/components/gga/config_test.gointernal/components/permissions/inject_test.gointernal/components/uninstall/cleaners_test.gointernal/tui/model_test.gointernal/update/check_test.gointernal/update/install_script_test.gointernal/update/upgrade/executor_test.goscripts/install.sh
| tmpFile := "script_test.sh" | ||
| if err := os.WriteFile(tmpFile, []byte(cmdStr), 0o755); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer os.Remove(tmpFile) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use a unique temp path instead of a fixed script_test.sh in the package dir.
Writing to a hardcoded relative filename in the current working directory risks colliding with a leftover file (e.g. if a prior run crashed before the defer os.Remove) and is not isolated if the package ever runs tests in parallel. Prefer t.TempDir() so cleanup and uniqueness are guaranteed.
♻️ Proposed fix
- tmpFile := "script_test.sh"
- if err := os.WriteFile(tmpFile, []byte(cmdStr), 0o755); err != nil {
- t.Fatal(err)
- }
- defer os.Remove(tmpFile)
+ tmpFile := filepath.Join(t.TempDir(), "script_test.sh")
+ if err := os.WriteFile(tmpFile, []byte(cmdStr), 0o755); err != nil {
+ t.Fatal(err)
+ }📝 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.
| tmpFile := "script_test.sh" | |
| if err := os.WriteFile(tmpFile, []byte(cmdStr), 0o755); err != nil { | |
| t.Fatal(err) | |
| } | |
| defer os.Remove(tmpFile) | |
| tmpFile := filepath.Join(t.TempDir(), "script_test.sh") | |
| if err := os.WriteFile(tmpFile, []byte(cmdStr), 0o755); err != nil { | |
| t.Fatal(err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/update/install_script_test.go` around lines 116 - 120, The test
helper in install_script_test.go is writing to a fixed relative filename, which
can collide with leftover files and is not isolated for parallel runs. Update
the temp script creation used around os.WriteFile and defer os.Remove to use a
unique path from t.TempDir() instead of the hardcoded script_test.sh name,
keeping the rest of the test flow the same.
|
@Alan-TheGentleman I've addressed the CodeRabbit review comments: isolated the temporary bash test script using a unique name containing the test's identity (avoiding absolute paths that fail in WSL bash), and updated the env-var eval fallback in scripts/install.sh to utilize default expansion to prevent set -u failures. |
1 participant
Summary
Fixes various Windows test failures across the codebase to ensure complete Windows compatibility and full test isolation.
Problem
Several tests failed on Windows due to:
APPDATA) andUSERPROFILEnot being isolated in tests./) hardcoded in expectations instead of using platform-specific pathing..exe) missing in tool setup binary assertions.wsl: Failed to start...).Solution
APPDATAviat.Setenvinconfig_test.goandexecutor_test.goto prevent pollution.HOMEandUSERPROFILEin TUI model picker tests (model_test.go).filepath.FromSlashforTestEngramGoInstallFromMain_UsesGoEnvForBinDir..exeto expected binary names when running on Windows in CLI engram tests.cleaners_test.goif the user lacks elevated privileges on Windows.${!name:-}) with a standardevalsyntax inscripts/install.sh.install_script_test.goas a file run directly, and strippedwsl:warnings from command outputs.exec.Commandcalls in check tests withmockCmdto support Windows.Closes #960
Summary by CodeRabbit