-
Notifications
You must be signed in to change notification settings - Fork 5
Trying to remove the $ failure from Shell Challenge Tests
#161
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.
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
Trying to remove the $ failure from Shell Challenge Tests
#161
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe GitHub Actions workflow for testing was updated to run the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 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
CodeRabbit Configuration File (
|
e280dea to
818957c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
internal/test_helpers/zsh/your_shell.sh (1)
2-10: Unconditionalrm -rf /etc/zsh*is risky and may silently fail
If the CI image ever runs as a non-root user these commands will fail, and if it does run as root they delete system-wide files outside the repo context. Either case can mask test issues or create side-effects.Guard the removals (e.g.,
if [ -w /etc/zsh ]; then … fi) or limit cleanup strictly to the test fixture directory.
🧹 Nitpick comments (1)
internal/test_helpers/zsh/your_shell.sh (1)
11-11: Use an absoluteZDOTDIRandexecto avoid brittle paths & subshell overhead
The relative path will break if the script is invoked from anywhere other than the repo root, and spawning an extra subshell is unnecessary.-ZDOTDIR='./internal/test_helpers/zsh/zsh_config' zsh -i +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +ZDOTDIR="$SCRIPT_DIR/zsh_config" exec zsh -i
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)internal/test_helpers/zsh/your_shell.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
| - name: Run tests against all shells on alpine | ||
| run: | | ||
| sudo make test_zsh | ||
| script -q -c "make test_zsh" /dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test Command Privileges and Output Redirection Issues
The make test_zsh command now runs without sudo privileges, which will cause test failures if elevated permissions are required (as implied by its previous use). Additionally, the script command redirects all test output to /dev/null, preventing visibility of results and making debugging impossible in CI.
Locations (1)
|
Can't believe that worked |
Summary by CodeRabbit