fix: skip additional Windows-specific tests #465
Conversation
- fish-installer: skip uninstall permission test (chmod on directory) - powershell-installer: skip "skip configuration when script line exists" test (Windows has dual profile paths so the second profile gets configured)
📝 WalkthroughWalkthroughTests updated to avoid Windows-specific failures: fish and PowerShell tests are conditionally skipped on Windows; bash tests now simulate failures by creating a blocking file to trigger ENOTDIR/permission errors instead of using invalid home paths. No production code changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
Instead of platform-specific invalid paths (Z:\ or /root), create a temporary file and use it as homeDir. This guarantees deterministic ENOTDIR failures when trying to create subdirectories on all platforms.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/core/completions/installers/bash-installer.test.ts (2)
378-388: Consider updating test name to reflect ENOTDIR scenario.The implementation correctly uses the blocking file pattern for cross-platform testing. However, the test name says "write permission errors" when it's actually testing ENOTDIR errors (attempting to write to a path where a parent component is a file, not a directory). The comments correctly describe the behavior.
💡 Suggested test name update
- it('should handle write permission errors gracefully', async () => { + it('should handle ENOTDIR errors gracefully', async () => { // Create a temporary file and use its path as homeDir // This guarantees ENOTDIR when trying to write .bashrc (cross-platform)
465-470: Consider using blocking file approach for cross-platform consistency.This test still uses the Unix-specific path
/root/invalid/path, which is inconsistent with the PR's goal of making error tests cross-platform using the ENOTDIR approach (as applied in lines 141-152 and 378-388).♻️ Align with cross-platform ENOTDIR pattern
it('should handle permission errors gracefully', async () => { - const invalidInstaller = new BashInstaller('/root/invalid/path'); + // Create a temporary file and use its path as homeDir + // This guarantees ENOTDIR when trying to access .bashrc (cross-platform) + const blockingFile = path.join(testHomeDir, 'blocking-file-remove'); + await fs.writeFile(blockingFile, 'blocking content'); + const invalidInstaller = new BashInstaller(blockingFile); const result = await invalidInstaller.removeBashrcConfig(); expect(result).toBe(true); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/core/completions/installers/bash-installer.test.ts
🔇 Additional comments (1)
test/core/completions/installers/bash-installer.test.ts (1)
141-152: LGTM! Cross-platform error handling approach is sound.The blocking file technique reliably triggers ENOTDIR errors across all platforms when the installer attempts to create subdirectories under a file path. This is a solid improvement over platform-specific invalid paths.
Summary
Skip two additional Windows-specific tests that were still failing after #464.
Changes
fish-installer: skip uninstall permission test (chmod on directory)powershell-installer: skip "skip configuration when script line exists" test (Windows has dual profile paths, so the second profile gets configured)🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.