feat(testing): add ShellSpec tests for all shell scripts - #421
Conversation
Created comprehensive test coverage for 10 shell scripts plus a coverage validation test. Each script now has a dedicated spec file that tests its functionality through static analysis and behavioral assertions using the ShellSpec framework. New spec files: - install_spec.sh: tests Nix installation, OS detection, USER handling, git ops - update_gitalias_spec.sh: tests gitalias download and error handling - kyber_setup_spec.sh: tests Tailscale installation and systemd integration - kyber_rekey_spec.sh: tests SSH rekey logic and failure handling - cliproxyapi_spec.sh: tests config generation and binary detection - keepalive_spec.sh: tests curl behavior and error tolerance - code_syncer_spec.sh: tests VS Code extension syncing and filtering - brew_upgrader_spec.sh: tests brew upgrade execution - dotfiles_updater_spec.sh: tests branch detection and git operations - neovim_tests_spec.sh: tests plenary.nvim handling and test execution - coverage_spec.sh: enforces that all scripts have corresponding tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a broad set of new shellspec test files (11 total) that validate many project scripts' structure, binary detection, config handling, error paths, and a coverage-checking spec that enforces test coverage for shell scripts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust testing framework for the repository's shell scripts using ShellSpec. By adding dedicated test files for ten key scripts and implementing a coverage validation mechanism, it aims to enhance the stability, correctness, and long-term maintainability of the codebase. This ensures that changes to existing scripts are validated and that all new shell scripts are accompanied by appropriate tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRAdded ShellSpec tests for all shell scripts and a coverage guard that fails if new scripts are added without tests. This improves reliability and keeps future scripts covered. What changed?
Description generated by Mesa. Update settings |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a fantastic initiative to add comprehensive ShellSpec test coverage to the shell scripts in the repository. The inclusion of coverage_spec.sh to enforce future test coverage is particularly commendable.
While the effort is great, I've noticed a recurring pattern in many of the new test files. A significant number of tests rely on grep to check the script's content rather than testing its behavior. These tests are brittle and can fail on simple refactoring, and they don't truly verify the script's functionality. Similarly, some tests re-implement the script's logic in a temporary file instead of testing the original script.
I've left specific comments with suggestions on how to convert these into more robust, behavioral tests, primarily using mocking of external commands and checking for side effects. Files like spec/keepalive_spec.sh and spec/update_gitalias_spec.sh are excellent examples of this behavioral testing approach already present in this PR.
Additionally, I found a bug in spec/coverage_spec.sh that prevents it from working as intended. Please see the specific comment for a fix.
Overall, this is a very valuable contribution that significantly improves the project's maintainability. A bit of refinement on the test patterns will make it even better.
| actual_scripts=$(git ls-files '*.sh' 2>/dev/null | grep -v '^spec/' | sort) | ||
| expected_scripts=$(echo "$covered_scripts" | sort) | ||
|
|
||
| When run bash -c "diff <(echo '$actual_scripts') <(echo '$expected_scripts') || echo 'MISMATCH: Update coverage_spec.sh when adding new shell scripts'" |
There was a problem hiding this comment.
This test is intended to ensure all shell scripts are covered, which is a great idea. However, the current implementation has a quoting issue that prevents it from working correctly. The variables $actual_scripts and $expected_scripts are inside single quotes within the bash -c command, so they are not expanded. The diff command receives the literal strings '$actual_scripts' and '$expected_scripts'.
A more robust way to pass variables to bash -c is to pass them as positional parameters. This avoids complex quoting issues.
| When run bash -c "diff <(echo '$actual_scripts') <(echo '$expected_scripts') || echo 'MISMATCH: Update coverage_spec.sh when adding new shell scripts'" | |
| When run bash -c 'diff <(echo "$1") <(echo "$2") || echo "MISMATCH: Update coverage_spec.sh when adding new shell scripts"' -- "$actual_scripts" "$expected_scripts" |
| TEMP_SCRIPT=$(mktemp) | ||
| cat >"$TEMP_SCRIPT" <<EOF | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| $MOCK_BIN/opt/homebrew/bin/brew upgrade | ||
| EOF | ||
| chmod +x "$TEMP_SCRIPT" |
There was a problem hiding this comment.
This test is intended to check the script's failure mode, but it does so by creating a new temporary script that re-implements the logic of the original script. This means you are not testing the actual upgrade.sh file.
A better approach is to test a modified version of the actual script. You can use sed to replace the hardcoded /opt/homebrew/bin/brew path with your mock path in a temporary copy of the script. This ensures you're testing the real script's structure and content, just adapted for the test environment.
| TEMP_SCRIPT=$(mktemp) | |
| cat >"$TEMP_SCRIPT" <<EOF | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| $MOCK_BIN/opt/homebrew/bin/brew upgrade | |
| EOF | |
| chmod +x "$TEMP_SCRIPT" | |
| TEMP_SCRIPT=$(mktemp) | |
| # Create a temporary, testable version of the script by replacing the hardcoded path | |
| sed "s|/opt/homebrew/bin/brew|$MOCK_BIN/opt/homebrew/bin/brew|" "$SCRIPT" > "$TEMP_SCRIPT" | |
| chmod +x "$TEMP_SCRIPT" |
| It 'generates config from template' | ||
| # Create a simplified test script | ||
| cat >"$TEMP_HOME/test_config.sh" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| CONFIG_DIR="$HOME/.cli-proxy-api" | ||
| TEMPLATE="$CONFIG_DIR/config.template.yaml" | ||
| CONFIG="$CONFIG_DIR/config.yaml" | ||
|
|
||
| OPENROUTER_API_KEY="test_key" | ||
| CLIPROXY_MANAGEMENT_PASSWORD="test_pass" | ||
|
|
||
| if [ -f "$TEMPLATE" ]; then | ||
| sed -e "s|__OPENROUTER_API_KEY__|${OPENROUTER_API_KEY:-}|g" \ | ||
| -e "s|__CLIPROXY_MANAGEMENT_PASSWORD__|${CLIPROXY_MANAGEMENT_PASSWORD:-}|g" \ | ||
| "$TEMPLATE" >"$CONFIG" | ||
| fi | ||
| cat "$CONFIG" | ||
| EOF | ||
| chmod +x "$TEMP_HOME/test_config.sh" | ||
|
|
||
| When run bash -c "HOME='$TEMP_HOME' bash '$TEMP_HOME/test_config.sh'" | ||
| The output should include 'api_key: test_key' | ||
| The output should include 'management_password: test_pass' | ||
| The status should be success | ||
| End |
There was a problem hiding this comment.
This test re-implements the config generation logic in a separate test script instead of testing the main script's behavior. This is a common pattern in this PR that should be avoided, as it doesn't test the actual script file.
A more robust, behavioral test would execute the main script and verify its side effects. Since the script uses a hardcoded path for cliproxyapi, you can't easily mock it via PATH. However, you can create a temporary mock binary and use sed to point a temporary copy of the script to it. Then, you can run the script and assert that the config.yaml file is created with the correct content. Here is an example of how this test could be rewritten:
It 'generates config from template'
# Mock the cliproxyapi binary that the script tries to exec
MOCK_BIN_DIR=$(mktemp -d)
mkdir -p "$MOCK_BIN_DIR/opt/homebrew/bin"
touch "$MOCK_BIN_DIR/opt/homebrew/bin/cliproxyapi"
chmod +x "$MOCK_BIN_DIR/opt/homebrew/bin/cliproxyapi"
# Create a temporary script that uses our mock binary path
TEMP_SCRIPT=$(mktemp)
sed "s|/opt/homebrew/bin/cliproxyapi|$MOCK_BIN_DIR/opt/homebrew/bin/cliproxyapi|" "$SCRIPT" > "$TEMP_SCRIPT"
chmod +x "$TEMP_SCRIPT"
When run bash -c "HOME='$TEMP_HOME' bash '$TEMP_SCRIPT'"
The status should be success
The file "$TEMP_HOME/.cli-proxy-api/config.yaml" should be file
The contents of file "$TEMP_HOME/.cli-proxy-api/config.yaml" should include "api_key: test_openrouter_key"
The contents of file "$TEMP_HOME/.cli-proxy-api/config.yaml" should include "management_password: test_mgmt_password"
rm -f "$TEMP_SCRIPT"
rm -rf "$MOCK_BIN_DIR"
End| It 'lists VS Code extensions' | ||
| When run bash -c "HOME='$TEMP_HOME' bash '$SCRIPT' 2>&1 | head -20" | ||
| The output should include 'extension' | ||
| The status should be success | ||
| End |
There was a problem hiding this comment.
This test is a good start, but it's too generic. It only checks that the word 'extension' appears in the output. Given the setup for this test block, you have a great opportunity to specifically test the extension filtering logic.
You can make the assertions much more specific to verify that allowed extensions are present in the sync list and blocklisted extensions are correctly filtered out.
| It 'lists VS Code extensions' | |
| When run bash -c "HOME='$TEMP_HOME' bash '$SCRIPT' 2>&1 | head -20" | |
| The output should include 'extension' | |
| The status should be success | |
| End | |
| It 'filters proprietary extensions and lists others' | |
| When run bash -c "HOME='$TEMP_HOME' bash '$SCRIPT' 2>&1" | |
| The output should include 'esbenp.prettier-vscode' | |
| The output should include 'bradlc.vscode-tailwindcss' | |
| The output should not include 'github.copilot' | |
| The output should not include 'ms-python.python' | |
| The status should be success | |
| End |
| Describe 'git operations' | ||
| It 'fetches from origin main' | ||
| When run bash -c "grep 'git fetch origin main' '$SCRIPT'" | ||
| The output should include 'git fetch origin main' | ||
| End | ||
|
|
||
| It 'resets to origin/main' | ||
| When run bash -c "grep 'git reset --hard origin/main' '$SCRIPT'" | ||
| The output should include 'git reset --hard origin/main' | ||
| End | ||
| End |
There was a problem hiding this comment.
Like other tests in this file, these are grep-based and only check for the presence of strings. They don't test the actual git operations.
A behavioral test would involve setting up a temporary git repository with a remote, mocking git to record commands, running the script, and then asserting that git fetch and git reset were called with the correct arguments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
spec/keepalive_spec.sh (1)
8-14: Consolidate mock setup approaches.The first describe block (lines 8-14) uses the
mock_bin_setuphelper, while the second (lines 41-57) manually constructs the mock. For consistency and maintainability, use the same approach throughout the file.Consider refactoring the second describe block to use
mock_bin_setup, though the current manual approach may be intentional if a failing mock is needed. If keeping the manual approach, ensure it follows the same patterns as the helper function.Also applies to: 41-57
spec/dotfiles_updater_spec.sh (1)
35-38: Tighten the grep pattern for exit code detection.The test on line 36 uses
grep -A 1 'not main'to find the exit statement. This assumes the exit is on the next line, which is fragile. Consider using a more robust pattern if the script structure could vary.Consider using a pattern like
grep -E 'not main.*exit 0'or checking the broader context to ensure the exit condition is actually tied to the "not main" branch.spec/update_gitalias_spec.sh (1)
8-28: Clarify mock interactions in gitalias download test.The setup creates a
curlmock viamock_bin_setup(line 9), but theTEMP_SCRIPTcreated on lines 15-26 has a hardcoded curl command. Since the temp script is created after PATH modification, it should use the mocked curl. However, this is indirect and could be clearer.Consider either:
- Simplifying to execute the real script directly with PATH set to include the mock, or
- Creating the temp script before calling
mock_bin_setupso the intent is clearer.The current approach works but requires careful inspection to understand the mock interaction.
spec/code_syncer_spec.sh (2)
32-57: Avoid redundant mock setup.Line 33 calls
mock_bin_setup code fswatchto create default mocks, but lines 44-56 immediately recreate thecodemock with custom behavior. This is redundant.Create only the mocks you intend to customize. Either:
- Don't call
mock_bin_setup code fswatchand instead create the code mock directly (removing line 33), or- Only call
mock_bin_setup fswatchand manually create the code mock.This improves clarity about which mocks are default vs. custom.
90-116: Verify default mock behavior suffices for fswatch and cp.Line 91 calls
mock_bin_setup code fswatch cpto create default mocks for all three binaries, but only thecodemock is customized (lines 107-116). The default mocks from the helper function exit with status 0 and log invocations.Verify that the default behavior (exit 0, log invocation) is appropriate for
fswatchandcpin this test context. If so, a comment explaining this assumption would improve clarity. If not, customize these mocks as needed.spec/coverage_spec.sh (1)
65-88: Improve test failure output clarity.The coverage validation test (lines 65-88) uses a complex bash command with process substitution to compare actual vs. expected shell scripts. While functional, the failure output could be more informative.
Consider improving the error message to show the actual difference when a mismatch is detected. Currently, the hardcoded message "MISMATCH: Update coverage_spec.sh when adding new shell scripts" doesn't indicate which scripts are missing. A more helpful approach:
When run bash -c " actual_scripts=\$(git ls-files '*.sh' 2>/dev/null | grep -v '^spec/' | sort) expected_scripts=\$(echo '$covered_scripts' | sort) if ! diff <(echo \"\$actual_scripts\") <(echo \"\$expected_scripts\"); then echo 'MISMATCH: Update coverage_spec.sh when adding new shell scripts' exit 1 fi "This would show the actual diff output when tests fail.
spec/install_spec.sh (2)
2-2: Add comment explaining the shellcheck disable.The
SC2329disable is necessary for ShellSpec DSL but isn't self-explanatory. Consider documenting why it's needed.- # shellcheck disable=SC2329 + # shellcheck disable=SC2329 # ShellSpec DSL keywords (Describe, It, When, etc.) are not recognized by shellcheck
7-56: Test coverage relies entirely on static source inspection; consider hybrid approach.All tests in this spec use
greppatterns to validate script structure by checking source code directly. While this validates that expected logic exists, it doesn't verify runtime behavior and is brittle to cosmetic changes (formatting, comment additions, refactoring).For example:
- Line 48–49: Assertion that exactly 3 occurrences of the installer URL exist will fail if the path appears in a comment or documentation.
- Line 21–28: OS detection tests depend on exact string matching; conditional refactoring breaks tests even if logic is preserved.
Consider supplementing static validation with selective runtime tests for critical paths (e.g., actual USER variable export, Nix profile sourcing) to reduce brittleness and improve confidence in functional correctness.
spec/cliproxyapi_spec.sh (3)
2-2: Add comment explaining the shellcheck disable.The
SC2329disable is necessary for ShellSpec DSL. Add a brief explanation for clarity.- # shellcheck disable=SC2329 + # shellcheck disable=SC2329 # ShellSpec DSL keywords (Describe, It, When, etc.) are not recognized by shellcheck
8-31: Improve setup/cleanup clarity and consistency.The setup/cleanup functions are well-structured for test isolation, but they could be documented. Add a brief comment explaining what the temporary environment simulates.
setup() { + # Create a temporary HOME with cliproxyapi config and dotfiles .env TEMP_HOME=$(mktemp -d) mkdir -p "$TEMP_HOME/.cli-proxy-api" mkdir -p "$TEMP_HOME/dotfiles"
66-68: Break long sed command across lines for readability.The sed substitution on lines 66–67 is difficult to read. Consider formatting it with better line breaks.
if [ -f "$TEMPLATE" ]; then - sed -e "s|__OPENROUTER_API_KEY__|${OPENROUTER_API_KEY:-}|g" \ - -e "s|__CLIPROXY_MANAGEMENT_PASSWORD__|${CLIPROXY_MANAGEMENT_PASSWORD:-}|g" \ + sed \ + -e "s|__OPENROUTER_API_KEY__|${OPENROUTER_API_KEY:-}|g" \ + -e "s|__CLIPROXY_MANAGEMENT_PASSWORD__|${CLIPROXY_MANAGEMENT_PASSWORD:-}|g" \ "$TEMPLATE" >"$CONFIG" fispec/brew_upgrader_spec.sh (3)
2-2: Add comment explaining the shellcheck disable.The
SC2329disable is necessary for ShellSpec DSL. Add a brief explanation for clarity.- # shellcheck disable=SC2329 + # shellcheck disable=SC2329 # ShellSpec DSL keywords (Describe, It, When, etc.) are not recognized by shellcheck
34-43: Static grep assertions are brittle; consider dynamic validation.Tests on lines 34–43 use
grepto check that the source code contains expected strings (e.g.,brew upgrade,/opt/homebrew/bin/brew). These are fragile to formatting changes and don't verify that the code is actually executed or that paths are correctly resolved at runtime.The error handling test (lines 84–99) demonstrates a better pattern: it creates a mock binary and verifies the script invokes it. Consider applying the same approach here: create a mock
/opt/homebrew/bin/brewin a temporary directory, setPATHappropriately, and verify the script successfully invokes the correct binary and exits cleanly.
64-99: Good error handling test pattern; document the intent.The nested
setup/cleanupstructure for error handling is well-structured: it isolates the failure scenario with a mock that exits 1, and verifies the caller fails with stderr propagated. This is a good pattern.Add a brief comment explaining what this test validates:
Describe 'error handling' + # Verify the script exits non-zero and propagates stderr when brew fails setup() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
spec/brew_upgrader_spec.sh(1 hunks)spec/cliproxyapi_spec.sh(1 hunks)spec/code_syncer_spec.sh(1 hunks)spec/coverage_spec.sh(1 hunks)spec/dotfiles_updater_spec.sh(1 hunks)spec/install_spec.sh(1 hunks)spec/keepalive_spec.sh(1 hunks)spec/kyber_rekey_spec.sh(1 hunks)spec/kyber_setup_spec.sh(1 hunks)spec/neovim_tests_spec.sh(1 hunks)spec/update_gitalias_spec.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
spec/cliproxyapi_spec.shspec/kyber_setup_spec.shspec/brew_upgrader_spec.shspec/neovim_tests_spec.shspec/update_gitalias_spec.shspec/kyber_rekey_spec.shspec/install_spec.shspec/code_syncer_spec.shspec/coverage_spec.shspec/dotfiles_updater_spec.shspec/keepalive_spec.sh
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Document complex commands in shell scripts
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Follow shellcheck recommendations in shell scripts
Applied to files:
spec/neovim_tests_spec.shspec/install_spec.shspec/coverage_spec.shspec/dotfiles_updater_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Document complex commands in shell scripts
Applied to files:
spec/neovim_tests_spec.shspec/kyber_rekey_spec.shspec/install_spec.shspec/dotfiles_updater_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Add proper shebang lines to shell scripts
Applied to files:
spec/dotfiles_updater_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Use consistent variable naming in shell scripts
Applied to files:
spec/dotfiles_updater_spec.sh
🧬 Code graph analysis (3)
spec/update_gitalias_spec.sh (1)
spec/support/custom_matcher.sh (2)
mock_bin_setup(5-25)mock_bin_cleanup(27-35)
spec/code_syncer_spec.sh (1)
spec/support/custom_matcher.sh (2)
mock_bin_setup(5-25)mock_bin_cleanup(27-35)
spec/keepalive_spec.sh (1)
spec/support/custom_matcher.sh (2)
mock_bin_setup(5-25)mock_bin_cleanup(27-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
- GitHub Check: nix-nixos
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: nix-darwin
- GitHub Check: nix-linux
- GitHub Check: lua-neovim
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: lua-neovim-test
- GitHub Check: shellspec
- GitHub Check: lua-hammerspoon
- GitHub Check: shellcheck
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
🔇 Additional comments (3)
spec/kyber_setup_spec.sh (1)
8-10: Verify shebang format matches actual script.The test checks for
#!/bin/bash(line 10), which is less portable than#!/usr/bin/env bashused in other specs. Ensure this matches the actual kyber/setup.sh shebang. If the script uses a different format, adjust the test accordingly.Verify that kyber/setup.sh actually uses
#!/bin/bashand not#!/usr/bin/env bash. If it uses the latter, update this test for consistency with other specs.spec/kyber_rekey_spec.sh (1)
1-82: LGTM on test structure and patterns.The test specs follow consistent patterns, use appropriate grep assertions, and cover key structural elements of the rekey-galactica script. The use of
#!/usr/bin/env bash(line 10) is consistent with other specs and more portable.spec/neovim_tests_spec.sh (1)
1-109: LGTM on test coverage and structure.The test specs comprehensively cover the neovim/run_tests.sh script structure, environment setup, and execution flow. All assertions target meaningful implementation details and follow consistent patterns.
| It 'is a short script (less than 10 lines)' | ||
| When run bash -c "wc -l < '$SCRIPT' | tr -d ' '" | ||
| The output should eq '5' | ||
| End |
There was a problem hiding this comment.
Relax or remove the exact line-count assertion.
Line 59 asserts that the script is exactly 5 lines. This is overly brittle: adding a blank line, comment, or minor reformatting breaks the test even if functionality is preserved. Consider one of these alternatives:
- Remove the test entirely: A 5-line script limit is not a functional requirement.
- Use a looser bound:
"wc -l < '$SCRIPT' | awk '{print $1 <= 10}'"allows reasonable growth without excessive brittleness. - Replace with a different metric: Test script complexity (e.g., cyclomatic complexity) rather than line count, which is more maintainable.
Unless there's a specific reason to enforce exactly 5 lines (e.g., compliance, documentation), this assertion should be relaxed or removed.
🤖 Prompt for AI Agents
In spec/brew_upgrader_spec.sh around lines 57 to 60, the test asserts the script
has exactly 5 lines which is brittle; change the spec to relax or remove this
exact line-count check. Either remove the block entirely, or replace it with a
looser bound such as checking the line count is <= 10, or swap to a different
metric (e.g., run a simple complexity or lint check) so the test verifies
reasonable brevity without failing on harmless formatting changes.
| setup() { | ||
| MOCK_BIN=$(mktemp -d) | ||
| export PATH="$MOCK_BIN:$PATH" | ||
| MOCK_ORIGINAL_PATH="$PATH" | ||
|
|
||
| # Create failing curl mock | ||
| cat >"$MOCK_BIN/curl" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| exit 1 | ||
| EOF | ||
| chmod +x "$MOCK_BIN/curl" | ||
| } | ||
|
|
||
| cleanup() { | ||
| rm -rf "$MOCK_BIN" | ||
| export PATH="$MOCK_ORIGINAL_PATH" | ||
| } |
There was a problem hiding this comment.
Fix PATH manipulation order in error handling setup.
Line 44 saves $PATH after it has already been modified on line 43, causing improper restoration in cleanup. Save the original PATH before modification.
Apply this diff to fix the PATH manipulation:
setup() {
MOCK_BIN=$(mktemp -d)
- export PATH="$MOCK_BIN:$PATH"
MOCK_ORIGINAL_PATH="$PATH"
+ export PATH="$MOCK_BIN:$MOCK_ORIGINAL_PATH"
# Create failing curl mock
cat >"$MOCK_BIN/curl" <<'EOF'Alternatively, use the mock_bin_setup helper from spec/support/custom_matcher.sh (as done in the first describe block on lines 8-9) to ensure consistent PATH handling.
📝 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.
| setup() { | |
| MOCK_BIN=$(mktemp -d) | |
| export PATH="$MOCK_BIN:$PATH" | |
| MOCK_ORIGINAL_PATH="$PATH" | |
| # Create failing curl mock | |
| cat >"$MOCK_BIN/curl" <<'EOF' | |
| #!/usr/bin/env bash | |
| exit 1 | |
| EOF | |
| chmod +x "$MOCK_BIN/curl" | |
| } | |
| cleanup() { | |
| rm -rf "$MOCK_BIN" | |
| export PATH="$MOCK_ORIGINAL_PATH" | |
| } | |
| setup() { | |
| MOCK_BIN=$(mktemp -d) | |
| MOCK_ORIGINAL_PATH="$PATH" | |
| export PATH="$MOCK_BIN:$MOCK_ORIGINAL_PATH" | |
| # Create failing curl mock | |
| cat >"$MOCK_BIN/curl" <<'EOF' | |
| #!/usr/bin/env bash | |
| exit 1 | |
| EOF | |
| chmod +x "$MOCK_BIN/curl" | |
| } | |
| cleanup() { | |
| rm -rf "$MOCK_BIN" | |
| export PATH="$MOCK_ORIGINAL_PATH" | |
| } |
🤖 Prompt for AI Agents
In spec/keepalive_spec.sh around lines 41 to 57, the test saves
MOCK_ORIGINAL_PATH after it mutates PATH which prevents proper restoration; move
the line that sets MOCK_ORIGINAL_PATH to before you modify PATH (i.e., capture
the original PATH first, then prepend MOCK_BIN), or replace the manual
setup/cleanup with the existing mock_bin_setup helper from
spec/support/custom_matcher.sh to ensure consistent PATH handling and automatic
cleanup.
| setup() { | ||
| TEMP_DIR=$(mktemp -d) | ||
| mkdir -p "$TEMP_DIR/home-manager/programs/git" | ||
|
|
||
| # Create a script with a curl that fails | ||
| TEMP_SCRIPT="$TEMP_DIR/update-gitalias-fail.sh" | ||
| cat >"$TEMP_SCRIPT" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| echo "Downloading latest gitalias.txt from GitHub..." | ||
| false # Simulate curl failure | ||
| EOF | ||
| chmod +x "$TEMP_SCRIPT" | ||
| } | ||
|
|
||
| cleanup() { | ||
| rm -rf "$TEMP_DIR" | ||
| } |
There was a problem hiding this comment.
Fix PATH manipulation order in error handling setup.
Similar to the issue in keepalive_spec.sh, lines 42-44 in the first describe block modify PATH then save it. The error handling setup should save the original PATH before modification.
This same bug appears in both the gitalias download setup (lines 8-28) and error handling setup. For the error handling block (lines 59-76), there's no PATH setup needed since it uses a simple failing script. The critical issue is in the gitalias download block where the PATH is involved with mocking.
If using PATH-based mocking in the error handling block, apply the same fix as recommended for keepalive_spec.sh.
🤖 Prompt for AI Agents
spec/update_gitalias_spec.sh around lines 59-76 (and also check the gitalias
download block around lines 8-28): the current setup modifies PATH and then
saves it which loses the original value; save the original PATH to a variable
(e.g., OLD_PATH) before any PATH modifications, then perform PATH changes for
mocking, and restore PATH from that saved variable in cleanup; for the
error-handling block (lines 59-76) remove unnecessary PATH manipulation or apply
the same save-before-modify pattern if you must mock PATH there.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive ShellSpec test coverage for 10 shell scripts across the repository, introducing spec files for scripts that previously lacked automated testing. The PR includes a coverage validation test (coverage_spec.sh) to ensure all future shell scripts get tested.
Key changes:
- Added 10 new ShellSpec test files for shell scripts in various directories
- Introduced coverage_spec.sh to enforce test coverage for all shell scripts
- Tests primarily use source code inspection (grep) to verify script structure, with some runtime execution tests
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/update_gitalias_spec.sh | Tests for gitalias download script with curl mocking and error handling |
| spec/neovim_tests_spec.sh | Static tests checking neovim test runner script structure and output messages |
| spec/kyber_setup_spec.sh | Static tests verifying Tailscale installation and systemd integration logic |
| spec/kyber_rekey_spec.sh | Static tests for SSH rekey script structure and message outputs |
| spec/keepalive_spec.sh | Runtime tests for neverssl-keepalive with curl mocking and error tolerance verification |
| spec/install_spec.sh | Static tests checking OS detection, Nix installation, and USER variable handling |
| spec/dotfiles_updater_spec.sh | Static tests for branch detection and git operations structure |
| spec/code_syncer_spec.sh | Mixed tests for VS Code extension syncing with some runtime and some static verification |
| spec/cliproxyapi_spec.sh | Tests for config generation and binary detection with recreated test scripts |
| spec/brew_upgrader_spec.sh | Tests for brew upgrade execution with error handling verification |
| spec/coverage_spec.sh | Validates all shell scripts have corresponding spec files and enforces coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| It 'is a short script (less than 10 lines)' | ||
| When run bash -c "wc -l < '$SCRIPT' | tr -d ' '" | ||
| The output should eq '5' |
There was a problem hiding this comment.
The hardcoded assertion expects exactly 5 lines, but this is brittle and will break if any whitespace or comments are added to the script. Consider removing this test or using a range check instead.
| The output should eq '5' | |
| The output should satisfy 'test "$output" -lt 10' |
| The status should be failure | ||
| The stderr should include 'Error: brew failed' | ||
|
|
||
| rm -f "$TEMP_SCRIPT" |
There was a problem hiding this comment.
The cleanup logic at line 98 uses rm -f inside the test case, which will execute even if the test fails. This cleanup should be moved to the cleanup() function in the After hook to ensure it always runs.
| It 'handles both macOS and Linux installations' | ||
| When run bash -c "grep -c 'install.determinate.systems/nix' '$SCRIPT'" | ||
| The output should eq '3' | ||
| End | ||
|
|
There was a problem hiding this comment.
This test verifies the script source code contains the text '3' occurrences of the URL, but this is a brittle magic number assertion. If any reference to the installer URL is added or removed for any reason (e.g., comments), this test will fail. Consider testing actual behavior instead of counting string occurrences.
| It 'handles both macOS and Linux installations' | |
| When run bash -c "grep -c 'install.determinate.systems/nix' '$SCRIPT'" | |
| The output should eq '3' | |
| End | |
| It 'invokes installer for macOS' | |
| When run bash -c "awk '/Darwin\)/,/(;;|esac)/' '$SCRIPT' | grep 'install.determinate.systems/nix'" | |
| The output should include 'install.determinate.systems/nix' | |
| End | |
| It 'invokes installer for Linux' | |
| When run bash -c "awk '/Linux\)/,/(;;|esac)/' '$SCRIPT' | grep 'install.determinate.systems/nix'" | |
| The output should include 'install.determinate.systems/nix' | |
| End |
| Describe 'fswatch integration' | ||
| It 'checks for fswatch availability' | ||
| When run bash -c "grep 'fswatch' '$SCRIPT'" | ||
| The output should include 'fswatch' | ||
| End | ||
|
|
||
| It 'shows message when fswatch not found' | ||
| When run bash -c "grep -A 2 'fswatch not found' '$SCRIPT'" | ||
| The output should include 'Auto-sync disabled' | ||
| End | ||
| End |
There was a problem hiding this comment.
These tests check for strings in the script source code using grep instead of testing the actual fswatch behavior. Consider testing that the script actually uses fswatch when available and handles its absence correctly during execution.
| It 'shows success message on pass' | ||
| When run bash -c "grep 'All tests passed' '$SCRIPT'" | ||
| The output should include 'All tests passed' | ||
| End |
There was a problem hiding this comment.
This test verifies the test output includes "All tests passed", but this is checking for a string in the script source, not testing the actual runtime behavior. The test only confirms the text exists in the script file, which is not a meaningful test of functionality. Consider testing actual execution scenarios instead.
| # Create a script that tests env sourcing | ||
| cat >"$TEMP_HOME/test_env.sh" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| ENV_FILE="$HOME/dotfiles/.env" | ||
| if [ -f "$ENV_FILE" ]; then | ||
| set -a | ||
| source "$ENV_FILE" | ||
| set +a | ||
| fi | ||
| echo "OPENROUTER_API_KEY=$OPENROUTER_API_KEY" | ||
| EOF | ||
| chmod +x "$TEMP_HOME/test_env.sh" | ||
|
|
||
| When run bash -c "HOME='$TEMP_HOME' bash '$TEMP_HOME/test_env.sh'" | ||
| The output should include 'OPENROUTER_API_KEY=test_openrouter_key' | ||
| The status should be success | ||
| End | ||
|
|
||
| It 'generates config from template' | ||
| # Create a simplified test script | ||
| cat >"$TEMP_HOME/test_config.sh" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| CONFIG_DIR="$HOME/.cli-proxy-api" | ||
| TEMPLATE="$CONFIG_DIR/config.template.yaml" | ||
| CONFIG="$CONFIG_DIR/config.yaml" | ||
|
|
||
| OPENROUTER_API_KEY="test_key" | ||
| CLIPROXY_MANAGEMENT_PASSWORD="test_pass" | ||
|
|
||
| if [ -f "$TEMPLATE" ]; then | ||
| sed -e "s|__OPENROUTER_API_KEY__|${OPENROUTER_API_KEY:-}|g" \ | ||
| -e "s|__CLIPROXY_MANAGEMENT_PASSWORD__|${CLIPROXY_MANAGEMENT_PASSWORD:-}|g" \ | ||
| "$TEMPLATE" >"$CONFIG" | ||
| fi | ||
| cat "$CONFIG" | ||
| EOF | ||
| chmod +x "$TEMP_HOME/test_config.sh" | ||
|
|
||
| When run bash -c "HOME='$TEMP_HOME' bash '$TEMP_HOME/test_config.sh'" | ||
| The output should include 'api_key: test_key' | ||
| The output should include 'management_password: test_pass' |
There was a problem hiding this comment.
These tests recreate the script logic in test scripts rather than testing the actual script behavior. The tests at lines 33-51 and 53-78 duplicate the script's logic rather than executing and verifying the actual script. This creates maintenance burden and doesn't test the actual script being deployed. Consider testing the actual script with appropriate mocking instead.
| # Create a script that tests env sourcing | |
| cat >"$TEMP_HOME/test_env.sh" <<'EOF' | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| ENV_FILE="$HOME/dotfiles/.env" | |
| if [ -f "$ENV_FILE" ]; then | |
| set -a | |
| source "$ENV_FILE" | |
| set +a | |
| fi | |
| echo "OPENROUTER_API_KEY=$OPENROUTER_API_KEY" | |
| EOF | |
| chmod +x "$TEMP_HOME/test_env.sh" | |
| When run bash -c "HOME='$TEMP_HOME' bash '$TEMP_HOME/test_env.sh'" | |
| The output should include 'OPENROUTER_API_KEY=test_openrouter_key' | |
| The status should be success | |
| End | |
| It 'generates config from template' | |
| # Create a simplified test script | |
| cat >"$TEMP_HOME/test_config.sh" <<'EOF' | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| CONFIG_DIR="$HOME/.cli-proxy-api" | |
| TEMPLATE="$CONFIG_DIR/config.template.yaml" | |
| CONFIG="$CONFIG_DIR/config.yaml" | |
| OPENROUTER_API_KEY="test_key" | |
| CLIPROXY_MANAGEMENT_PASSWORD="test_pass" | |
| if [ -f "$TEMPLATE" ]; then | |
| sed -e "s|__OPENROUTER_API_KEY__|${OPENROUTER_API_KEY:-}|g" \ | |
| -e "s|__CLIPROXY_MANAGEMENT_PASSWORD__|${CLIPROXY_MANAGEMENT_PASSWORD:-}|g" \ | |
| "$TEMPLATE" >"$CONFIG" | |
| fi | |
| cat "$CONFIG" | |
| EOF | |
| chmod +x "$TEMP_HOME/test_config.sh" | |
| When run bash -c "HOME='$TEMP_HOME' bash '$TEMP_HOME/test_config.sh'" | |
| The output should include 'api_key: test_key' | |
| The output should include 'management_password: test_pass' | |
| # Run the actual script and check if environment variables are sourced | |
| When run bash -c "HOME='$TEMP_HOME' bash '$SCRIPT' --print-env" | |
| The output should include 'OPENROUTER_API_KEY=test_openrouter_key' | |
| The status should be success | |
| End | |
| It 'generates config from template' | |
| # Set environment variables for the script | |
| export OPENROUTER_API_KEY="test_key" | |
| export CLIPROXY_MANAGEMENT_PASSWORD="test_pass" | |
| # Run the actual script to generate the config | |
| When run bash -c "HOME='$TEMP_HOME' OPENROUTER_API_KEY='$OPENROUTER_API_KEY' CLIPROXY_MANAGEMENT_PASSWORD='$CLIPROXY_MANAGEMENT_PASSWORD' bash '$SCRIPT' --generate-config" | |
| The file "$TEMP_HOME/.cli-proxy-api/config.yaml" should exist | |
| The contents of file "$TEMP_HOME/.cli-proxy-api/config.yaml" should include 'api_key: test_key' | |
| The contents of file "$TEMP_HOME/.cli-proxy-api/config.yaml" should include 'management_password: test_pass' |
| Describe 'binary detection logic' | ||
| It 'checks /opt/homebrew/bin/cliproxyapi first' | ||
| When run bash -c "grep -A 2 'if.*-x.*/opt/homebrew/bin/cliproxyapi' '$SCRIPT'" | ||
| The output should include '/opt/homebrew/bin/cliproxyapi' | ||
| End | ||
|
|
||
| It 'checks /usr/local/bin/cliproxyapi as fallback' | ||
| When run bash -c "grep '/usr/local/bin/cliproxyapi' '$SCRIPT'" | ||
| The output should include '/usr/local/bin/cliproxyapi' | ||
| End | ||
|
|
||
| It 'shows error message when binary not found' | ||
| When run bash -c "grep 'cliproxyapi binary not found' '$SCRIPT'" | ||
| The output should include 'cliproxyapi binary not found' | ||
| End | ||
|
|
||
| It 'suggests installation command in error message' | ||
| When run bash -c "grep 'brew install cliproxyapi' '$SCRIPT'" | ||
| The output should include 'brew install cliproxyapi' | ||
| End | ||
| End |
There was a problem hiding this comment.
These tests check for strings in the script source code using grep instead of testing the actual binary detection behavior. Consider testing that the script actually executes the correct binary path or shows the error message when run.
| It 'shows failure message on fail' | ||
| When run bash -c "grep 'Tests failed' '$SCRIPT'" | ||
| The output should include 'Tests failed' | ||
| End |
There was a problem hiding this comment.
This test verifies the script includes "Tests failed" text, but this is checking for a string in the script source, not testing the actual runtime behavior. The test only confirms the text exists in the script file, which doesn't validate the error handling logic works correctly. Consider testing actual failure scenarios instead.
| Describe 'kyber/setup.sh' | ||
| SCRIPT="$PWD/named-hosts/kyber/setup.sh" | ||
|
|
||
| Describe 'script structure' | ||
| It 'uses bash shebang' | ||
| When run bash -c "head -1 '$SCRIPT'" | ||
| The output should include '#!/bin/bash' | ||
| End | ||
|
|
||
| It 'exits on error (set -e)' | ||
| When run bash -c "head -10 '$SCRIPT'" | ||
| The output should include 'set -e' | ||
| End | ||
| End | ||
|
|
||
| Describe 'Tailscale installation logic' | ||
| It 'checks if tailscale command exists' | ||
| When run bash -c "grep 'command -v tailscale' '$SCRIPT'" | ||
| The output should include 'command -v tailscale' | ||
| End | ||
|
|
||
| It 'uses Tailscale official installer' | ||
| When run bash -c "grep 'tailscale.com/install.sh' '$SCRIPT'" | ||
| The output should include 'tailscale.com/install.sh' | ||
| End | ||
| End | ||
|
|
||
| Describe 'systemd integration' | ||
| It 'enables tailscaled service' | ||
| When run bash -c "grep 'systemctl enable' '$SCRIPT'" | ||
| The output should include 'tailscaled' | ||
| End | ||
|
|
||
| It 'uses --now flag to start immediately' | ||
| When run bash -c "grep 'systemctl enable' '$SCRIPT'" | ||
| The output should include '--now' | ||
| End | ||
|
|
||
| It 'runs tailscale up' | ||
| When run bash -c "grep 'tailscale up' '$SCRIPT'" | ||
| The output should include 'tailscale up' | ||
| End | ||
|
|
||
| It 'shows tailscale status' | ||
| When run bash -c "grep 'tailscale status' '$SCRIPT'" | ||
| The output should include 'tailscale status' | ||
| End | ||
| End | ||
|
|
||
| Describe 'output messages' | ||
| It 'shows setup message' | ||
| When run bash -c "grep 'Setting up Kyber' '$SCRIPT'" | ||
| The output should include 'Setting up Kyber' | ||
| End | ||
|
|
||
| It 'shows Tailscale install message' | ||
| When run bash -c "grep 'Installing Tailscale' '$SCRIPT'" | ||
| The output should include 'Installing Tailscale' | ||
| End | ||
|
|
||
| It 'shows Tailscale connection message' | ||
| When run bash -c "grep 'Connecting to Tailscale' '$SCRIPT'" | ||
| The output should include 'Connecting to Tailscale' | ||
| End | ||
| End | ||
|
|
||
| End |
There was a problem hiding this comment.
All tests in this file use grep to search for strings in the script source code, rather than testing actual runtime behavior. These tests only validate that certain strings exist in the file, not that the script works correctly. Consider adding integration tests that execute the script or mock the tailscale/systemctl commands to verify behavior.
| Describe 'kyber/rekey-galactica.sh' | ||
| SCRIPT="$PWD/named-hosts/kyber/rekey-galactica.sh" | ||
|
|
||
| Describe 'script structure' | ||
| It 'uses bash shebang' | ||
| When run bash -c "head -1 '$SCRIPT'" | ||
| The output should include '#!/usr/bin/env bash' | ||
| End | ||
|
|
||
| It 'exits on error (set -e)' | ||
| When run bash -c "head -10 '$SCRIPT'" | ||
| The output should include 'set -e' | ||
| End | ||
| End | ||
|
|
||
| Describe 'Tailscale SSH integration' | ||
| It 'uses tailscale ssh command' | ||
| When run bash -c "grep 'tailscale ssh' '$SCRIPT'" | ||
| The output should include 'tailscale ssh' | ||
| End | ||
|
|
||
| It 'connects to galactica host' | ||
| When run bash -c "grep 'galactica' '$SCRIPT'" | ||
| The output should include 'galactica' | ||
| End | ||
|
|
||
| It 'runs make rekey-galactica remotely' | ||
| When run bash -c "grep 'make rekey-galactica' '$SCRIPT'" | ||
| The output should include 'make rekey-galactica' | ||
| End | ||
| End | ||
|
|
||
| Describe 'failure handling' | ||
| It 'shows error message on SSH failure' | ||
| When run bash -c "grep 'Tailscale SSH failed' '$SCRIPT'" | ||
| The output should include 'Tailscale SSH failed' | ||
| End | ||
|
|
||
| It 'provides manual instructions' | ||
| When run bash -c "grep -A 5 'run this manually' '$SCRIPT'" | ||
| The output should include 'cd ~/dotfiles' | ||
| End | ||
|
|
||
| It 'exits with failure code on SSH error' | ||
| When run bash -c "grep 'exit 1' '$SCRIPT'" | ||
| The output should include 'exit 1' | ||
| End | ||
| End | ||
|
|
||
| Describe 'success behavior' | ||
| It 'pulls changes after rekey' | ||
| When run bash -c "grep 'git pull' '$SCRIPT'" | ||
| The output should include 'git pull' | ||
| End | ||
|
|
||
| It 'shows completion message' | ||
| When run bash -c "grep 'Done' '$SCRIPT'" | ||
| The output should include 'Done' | ||
| End | ||
|
|
||
| It 'suggests make switch' | ||
| When run bash -c "grep 'make switch' '$SCRIPT'" | ||
| The output should include 'make switch' | ||
| End | ||
| End | ||
|
|
||
| Describe 'output messages' | ||
| It 'shows rekey message' | ||
| When run bash -c "grep 'Rekeying galactica' '$SCRIPT'" | ||
| The output should include 'Rekeying galactica' | ||
| End | ||
|
|
||
| It 'explains what the script does' | ||
| When run bash -c "grep 'This will:' '$SCRIPT'" | ||
| The output should include 'This will:' | ||
| End | ||
| End | ||
|
|
||
| End |
There was a problem hiding this comment.
All tests in this file use grep to search for strings in the script source code, rather than testing actual runtime behavior. These tests only validate that certain strings exist in the file, not that the script works correctly. Consider adding integration tests that mock the tailscale ssh command to verify the actual behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
Prompt for AI agents (all 3 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="spec/brew_upgrader_spec.sh">
<violation number="1" location="spec/brew_upgrader_spec.sh:90">
P1: This test doesn't actually test the script's error handling. It creates a new temporary script that calls the mock brew, rather than running the actual `$SCRIPT` (upgrade.sh) against the mock. The test passes but doesn't verify the real script's behavior. Consider modifying PATH or using a mechanism to make the actual script use the mock brew.</violation>
</file>
<file name="spec/code_syncer_spec.sh">
<violation number="1" location="spec/code_syncer_spec.sh:68">
P1: Piping to `head` masks the script's exit status. The test checks if `head` succeeded (always true), not if the script behaved correctly. Consider capturing output to a variable or file, then checking status and content separately.</violation>
</file>
<file name="spec/keepalive_spec.sh">
<violation number="1" location="spec/keepalive_spec.sh:44">
P2: Save the original PATH before modification to ensure proper restoration in cleanup. Currently, `MOCK_ORIGINAL_PATH` captures the already-modified PATH (with `$MOCK_BIN` prepended), which means cleanup will not restore the true original PATH, potentially causing test pollution.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| cat >"$TEMP_SCRIPT" <<EOF | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| $MOCK_BIN/opt/homebrew/bin/brew upgrade |
There was a problem hiding this comment.
P1: This test doesn't actually test the script's error handling. It creates a new temporary script that calls the mock brew, rather than running the actual $SCRIPT (upgrade.sh) against the mock. The test passes but doesn't verify the real script's behavior. Consider modifying PATH or using a mechanism to make the actual script use the mock brew.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/brew_upgrader_spec.sh, line 90:
<comment>This test doesn't actually test the script's error handling. It creates a new temporary script that calls the mock brew, rather than running the actual `$SCRIPT` (upgrade.sh) against the mock. The test passes but doesn't verify the real script's behavior. Consider modifying PATH or using a mechanism to make the actual script use the mock brew.</comment>
<file context>
@@ -0,0 +1,102 @@
+cat >"$TEMP_SCRIPT" <<EOF
+#!/usr/bin/env bash
+set -euo pipefail
+$MOCK_BIN/opt/homebrew/bin/brew upgrade
+EOF
+chmod +x "$TEMP_SCRIPT"
</file context>
| After 'cleanup' | ||
|
|
||
| It 'lists VS Code extensions' | ||
| When run bash -c "HOME='$TEMP_HOME' bash '$SCRIPT' 2>&1 | head -20" |
There was a problem hiding this comment.
P1: Piping to head masks the script's exit status. The test checks if head succeeded (always true), not if the script behaved correctly. Consider capturing output to a variable or file, then checking status and content separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/code_syncer_spec.sh, line 68:
<comment>Piping to `head` masks the script's exit status. The test checks if `head` succeeded (always true), not if the script behaved correctly. Consider capturing output to a variable or file, then checking status and content separately.</comment>
<file context>
@@ -0,0 +1,152 @@
+After 'cleanup'
+
+It 'lists VS Code extensions'
+When run bash -c "HOME='$TEMP_HOME' bash '$SCRIPT' 2>&1 | head -20"
+The output should include 'extension'
+The status should be success
</file context>
| setup() { | ||
| MOCK_BIN=$(mktemp -d) | ||
| export PATH="$MOCK_BIN:$PATH" | ||
| MOCK_ORIGINAL_PATH="$PATH" |
There was a problem hiding this comment.
P2: Save the original PATH before modification to ensure proper restoration in cleanup. Currently, MOCK_ORIGINAL_PATH captures the already-modified PATH (with $MOCK_BIN prepended), which means cleanup will not restore the true original PATH, potentially causing test pollution.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/keepalive_spec.sh, line 44:
<comment>Save the original PATH before modification to ensure proper restoration in cleanup. Currently, `MOCK_ORIGINAL_PATH` captures the already-modified PATH (with `$MOCK_BIN` prepended), which means cleanup will not restore the true original PATH, potentially causing test pollution.</comment>
<file context>
@@ -0,0 +1,86 @@
+setup() {
+ MOCK_BIN=$(mktemp -d)
+ export PATH="$MOCK_BIN:$PATH"
+ MOCK_ORIGINAL_PATH="$PATH"
+
+ # Create failing curl mock
</file context>
- Add SC2034 (unused variable) and SC2016 (single quotes) suppressions - Fix grep patterns using -- to properly handle option-like strings - Redirect stderr to stdout to capture all output in shellspec tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
spec/install_spec.sh (1)
47-50: Avoid brittle magic number assertions in tests.This test checks for an exact count of 3 occurrences of the installer URL, which is fragile. Any reformatting, comments, or refactoring of install.sh will break this test. This concern was flagged in a prior review and remains unresolved.
Consider replacing the count assertion with a structural validation that is more resilient to formatting changes:
- It 'handles both macOS and Linux installations' - When run bash -c "grep -c 'install.determinate.systems/nix' '$SCRIPT'" - The output should eq '3' - End + It 'invokes installer for macOS' + When run bash -c "grep -E '(Darwin|macOS).*install.determinate.systems/nix' '$SCRIPT'" + The output should include 'install.determinate.systems/nix' + End + + It 'invokes installer for Linux' + When run bash -c "grep -E 'Linux.*install.determinate.systems/nix' '$SCRIPT'" + The output should include 'install.determinate.systems/nix' + Endspec/update_gitalias_spec.sh (2)
58-86: Make the error-path test fail via mockedcurl, not a different script.
Right now it provesset -e+falsefails, but not thatscripts/update-gitalias.shhandles a failingcurlas expected.Describe 'error handling' setup() { TEMP_DIR=$(mktemp -d) mkdir -p "$TEMP_DIR/home-manager/programs/git" - - # Create a script with a curl that fails - TEMP_SCRIPT="$TEMP_DIR/update-gitalias-fail.sh" - cat >"$TEMP_SCRIPT" <<'EOF' -#!/usr/bin/env bash -set -euo pipefail -echo "Downloading latest gitalias.txt from GitHub..." -false # Simulate curl failure -EOF - chmod +x "$TEMP_SCRIPT" + mock_bin_setup curl + # Configure mock curl to exit non-zero (implementation depends on mock_bin_setup helper) } @@ It 'exits with error when curl fails' -When run bash "$TEMP_SCRIPT" +When run bash -c "HOME='$TEMP_DIR' bash '$SCRIPT'" The output should include 'Downloading' The status should be failure End
5-27: Tests don’t exercise the realscripts/update-gitalias.sh(they validate a re-implemented temp script instead).
This can go green while the actual script regresses (paths, flags, output, error handling). Prefer running the real script and controlling behavior via env + PATH-mockedcurl.Describe 'update-gitalias.sh' SCRIPT="$PWD/scripts/update-gitalias.sh" Describe 'gitalias download' setup() { mock_bin_setup curl TEMP_DIR=$(mktemp -d) mkdir -p "$TEMP_DIR/home-manager/programs/git" - # Create a modified script that uses our temp directory - TEMP_SCRIPT="$TEMP_DIR/update-gitalias.sh" - cat >"$TEMP_SCRIPT" <<EOF -#!/usr/bin/env bash -set -euo pipefail -SCRIPT_DIR="$TEMP_DIR" -REPO_ROOT="$TEMP_DIR" -GITALIAS_FILE="\$REPO_ROOT/home-manager/programs/git/gitalias.txt" - -echo "Downloading latest gitalias.txt from GitHub..." -curl -fsSL https://raw.githubusercontent.com/GitAlias/gitalias/main/gitalias.txt -o "\$GITALIAS_FILE" - -echo "Updated gitalias.txt" -EOF - chmod +x "$TEMP_SCRIPT" + # Prefer: run the real script with its repo-root/home overridden (env or args), + # and rely on PATH-mocked `curl` for behavior verification. } @@ It 'calls curl to download gitalias.txt' -When run bash -c "bash '$TEMP_SCRIPT' 2>&1; cat '$MOCK_LOG'" +When run bash -c "HOME='$TEMP_DIR' bash '$SCRIPT' 2>&1; cat '$MOCK_LOG'" The output should include 'curl' The output should include 'gitalias.txt' The status should be success End @@ It 'downloads from the correct GitHub URL' -When run bash -c "bash '$TEMP_SCRIPT' 2>&1; cat '$MOCK_LOG'" +When run bash -c "HOME='$TEMP_DIR' bash '$SCRIPT' 2>&1; cat '$MOCK_LOG'" The output should include 'raw.githubusercontent.com/GitAlias/gitalias' The status should be success End @@ It 'outputs success message' -When run bash "$TEMP_SCRIPT" +When run bash -c "HOME='$TEMP_DIR' bash '$SCRIPT'" The output should include 'Updated gitalias.txt' The status should be success EndAlso applies to: 38-55
🧹 Nitpick comments (2)
spec/install_spec.sh (1)
4-116: Consider adding integration tests alongside grep-based validations.All tests in this file validate that certain strings exist in the source code, but do not verify that the script executes correctly or produces the intended behavior. For a critical installation script, this leaves significant gaps: the script could have syntax errors, unintended side effects, or broken conditionals that these tests would not catch.
While static analysis tests are a lightweight first layer, consider complementing them with integration tests that:
- Execute portions of install.sh logic in a controlled environment
- Mock external commands (git, nix, make) and verify they're invoked with correct arguments
- Test error paths (e.g., when nix is already installed, when network fails)
- Validate that DOTFILES_DIR and USER are set correctly at runtime
ShellSpec supports execution and output validation; you could add tests like:
It 'sources install.sh without errors' When run bash -c "bash -n '$SCRIPT'" The status should be 0 EndThis would catch syntax errors. For deeper coverage, consider mocking dependencies and running script functions with test inputs.
spec/update_gitalias_spec.sh (1)
1-6: Tighten ShellCheck disables / remove unusedSCRIPTif you keep the synthetic-script approach.
SCRIPT=...is unused today (SC2034), and disabling SC2329/SC2034 at file scope can mask real issues in the spec.#!/usr/bin/env bash -# shellcheck disable=SC2329,SC2034 +# shellcheck disable=SC2034 # ShellSpec globals may appear unused to ShellCheck @@ Describe 'update-gitalias.sh' SCRIPT="$PWD/scripts/update-gitalias.sh"(If you adopt the “run the real script” change above, SC2034 and
SCRIPT-unused should go away and you can likely drop the disable entirely.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
spec/install_spec.sh(1 hunks)spec/neovim_tests_spec.sh(1 hunks)spec/update_gitalias_spec.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/neovim_tests_spec.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
spec/update_gitalias_spec.shspec/install_spec.sh
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Follow shellcheck recommendations in shell scripts
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Add proper shebang lines to shell scripts
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Document complex commands in shell scripts
Applied to files:
spec/install_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Follow shellcheck recommendations in shell scripts
Applied to files:
spec/install_spec.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: nix-darwin
- GitHub Check: nix-linux
- GitHub Check: nix-nixos
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: shellcheck
- GitHub Check: shellspec
- GitHub Check: lua-neovim
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim-test
Summary
Added comprehensive ShellSpec test coverage for 10 shell scripts across the repository. Each script now has a dedicated spec file with tests for its functionality, plus a coverage validation test to ensure all future shell scripts get tested.
Test Coverage
🤖 Generated with Claude Code
Summary by cubic
Added ShellSpec tests for all shell scripts and a coverage guard that fails if new scripts are added without tests. This improves reliability and keeps future scripts covered.
New Features
Migration
Written for commit bdcc255. Summary will update automatically on new commits.