fix(test): add roborev to shell coverage spec - #1751
Conversation
… list Closes the shell-test CI failure caused by the roborev activate.sh not being registered in coverage_spec.sh after #1745.
|
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughNew test specifications are added to validate the Roborev service activation script and its configuration module. The activate script tests confirm proper shebang, directory creation, and permission handling. The default.nix tests verify service conditional gating, daemon presence, and parameter passing. Coverage tracking is updated to include the new activation script. ChangesRoborev Service Test Coverage
🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Mesa DescriptionTL;DRAdds What changed?
Description generated by Mesa. Update settings |
| @@ -0,0 +1,35 @@ | |||
| Describe 'home-manager/services/roborev/activate.sh' | |||
There was a problem hiding this comment.
Missing standard spec header: every other spec/activate_*_spec.sh file (activate_qmd_spec.sh, activate_paperclip_openclaw_spec.sh, activate_k3s_spec.sh, activate_neovim_spec.sh, activate_tailscale_spec.sh) begins with:
#!/usr/bin/env bash
# shellcheck disable=SC2016,SC2329This file starts directly with Describe, which is inconsistent with the established convention and may surface SC2016/SC2329 warnings on the inline single-quoted bash -c "grep '...' '$SCRIPT'" patterns when shellcheck runs over the spec directory.
| Describe 'home-manager/services/roborev/activate.sh' | |
| #!/usr/bin/env bash | |
| # shellcheck disable=SC2016,SC2329 | |
| Describe 'home-manager/services/roborev/activate.sh' |
There was a problem hiding this comment.
Code Review
This pull request introduces a new test suite for the roborev service and updates the coverage manifest. Feedback recommends optimizing the ShellSpec tests by removing redundant shell wrappers, utilizing more efficient search commands, and ensuring the new test file is properly integrated into the repository's coverage verification process.
| home-manager/services/openclaw/activate.sh | ||
| home-manager/services/paperclip/activate.sh | ||
| home-manager/services/qmd/activate.sh | ||
| home-manager/services/roborev/activate.sh |
There was a problem hiding this comment.
While the script is correctly added to the covered_scripts list here, it is missing a corresponding It block in the first Describe section of this file (around line 206) to verify that the spec file itself exists. Following the established pattern in this repository ensures that new spec files are tracked and not accidentally deleted.
| SCRIPT="$PWD/home-manager/services/roborev/activate.sh" | ||
|
|
||
| It 'uses bash shebang' | ||
| When run bash -c "head -1 '$SCRIPT'" |
There was a problem hiding this comment.
The use of bash -c inside When run is unnecessary for simple commands like head or grep. ShellSpec can execute these directly, which is cleaner and more efficient. Additionally, using head -n 1 is more standard than head -1. This pattern should be applied to lines 10, 15, 22, and 27 as well.
| When run bash -c "head -1 '$SCRIPT'" | |
| When run head -n 1 "$SCRIPT" |
| End | ||
|
|
||
| It 'passes data dir to activate script' | ||
| When run cat "$PWD/home-manager/services/roborev/default.nix" |
There was a problem hiding this comment.
Using cat to read an entire file just to check for a specific string is less efficient than using grep directly. It is better to use grep to search for the pattern, which is also more consistent with the other tests in this file.
| When run cat "$PWD/home-manager/services/roborev/default.nix" | |
| When run grep '"${./activate.sh}" "${dataDir}"' "$PWD/home-manager/services/roborev/default.nix" |
References
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions.
Summary
home-manager/services/roborev/activate.shto thecovered_scriptslist inspec/coverage_spec.shspec/activate_roborev_spec.shwith tests for the activate script anddefault.nixThe shell-test CI job has been failing since #1745 (roborev service) because the new
activate.shwas not registered in the coverage spec.Test plan
shell-testCI job passesspec/activate_roborev_spec.shtests pass🤖 Generated with Claude Code
Summary by cubic
Registers
home-manager/services/roborev/activate.shin the shell coverage list and adds tests for the activate script anddefault.nix. Fixes the failingshell-testCI job introduced with the roborev service.Written for commit 6ee15c5. Summary will update on new commits.