#1867 test: add a regression suite and a CI job to run it - #1871
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds automated structural checks for binds, dot metafiles, Lua syntax, and shell scripts, wires them into GitHub Actions, improves Hyprland bind deduplication and keypad mappings, and corrects several shell script interpreter and expansion forms. ChangesValidation and bind quality
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant tests/run.sh
participant Test cases
participant Repository
GitHub Actions->>tests/run.sh: execute test suite
tests/run.sh->>Test cases: discover and run test scripts
Test cases->>Repository: inspect configuration and scripts
Repository-->>Test cases: validation results
Test cases-->>tests/run.sh: pass or fail status
tests/run.sh-->>GitHub Actions: workflow status
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 6
🤖 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 @.github/workflows/test.yml:
- Line 17: Update the actions/checkout@v4 step to set persist-credentials to
false, and ensure the workflow job permissions remain limited to contents: read
unless writable token access is explicitly required.
In `@Configs/.local/share/hypr/lua/hyde/binds.lua`:
- Around line 57-65: Update the modifier normalization logic before sorting so
duplicate normalized entries are removed, ensuring inputs such as repeated CTRL
produce the same canonical binding as a single CTRL. Preserve alias
normalization, deterministic sorting, and appending the uppercased key in the
existing return flow.
In `@tests/python/check_dots.py`:
- Around line 75-77: Update the source_root validation in the table-checking
logic to first require non-null values to be strings, then resolve REPO_ROOT /
source_root and reject any path whose resolved location escapes REPO_ROOT,
including absolute paths and parent-directory traversal; retain the
missing-directory failure for valid in-repository paths.
In `@tests/run.sh`:
- Around line 21-35: Update the test discovery and execution loop around
case_path so only regular executable test_*.sh files are included by checking
both -f and -x, then invoke each selected case directly instead of through sh,
preserving its shebang behavior.
In `@tests/test_shell.sh`:
- Around line 21-26: Update the ShellCheck invocation in the report block to
capture its exit status while preserving the captured output, then fail when
either xargs/ShellCheck returns non-zero or report is non-empty. Keep printing
report lines before calling fail when findings are present, and ensure the
status check distinguishes command failure from ShellCheck findings.
- Around line 7-13: The script-parsing loop currently validates every file with
bash regardless of its declared interpreter. Update the loop around scripts and
file to read each script’s shebang and run sh -n for /bin/sh or /usr/bin/env sh
declarations, otherwise bash -n for Bash declarations, preserving the existing
failure message and count behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8182dfb2-90da-4987-b2b6-44057f5091b9
📒 Files selected for processing (15)
.github/workflows/test.ymlConfigs/.config/uwsm/env.d/01-gpu.shConfigs/.local/lib/hyde/shutils/ocr.shConfigs/.local/lib/hyde/wallpaper.kon.shConfigs/.local/share/hypr/lua/hyde/binds.luaConfigs/.local/share/hypr/lua/key_binds.luatests/README.mdtests/lib/common.shtests/lua/bind_harness.luatests/python/check_dots.pytests/run.shtests/test_binds.shtests/test_dots.shtests/test_lua_syntax.shtests/test_shell.sh
| for case_path in "$tests_dir"/test_*.sh; do | ||
| [ -f "$case_path" ] || continue | ||
|
|
||
| case_name=$(basename "$case_path" .sh) | ||
| if [ -n "$filter" ]; then | ||
| case "$case_name" in | ||
| *"$filter"*) ;; | ||
| *) continue ;; | ||
| esac | ||
| fi | ||
|
|
||
| total=$((total + 1)) | ||
| printf '%s\n' "$case_name" | ||
|
|
||
| if sh "$case_path"; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor the documented executable-test contract.
The runner executes every regular test_*.sh via sh, including non-executable files and ignoring an executable test’s shebang. Filter on -x and execute the case directly.
Proposed fix
for case_path in "$tests_dir"/test_*.sh; do
- [ -f "$case_path" ] || continue
+ [ -f "$case_path" ] && [ -x "$case_path" ] || continue
...
- if sh "$case_path"; then
+ if "$case_path"; then📝 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.
| for case_path in "$tests_dir"/test_*.sh; do | |
| [ -f "$case_path" ] || continue | |
| case_name=$(basename "$case_path" .sh) | |
| if [ -n "$filter" ]; then | |
| case "$case_name" in | |
| *"$filter"*) ;; | |
| *) continue ;; | |
| esac | |
| fi | |
| total=$((total + 1)) | |
| printf '%s\n' "$case_name" | |
| if sh "$case_path"; then | |
| for case_path in "$tests_dir"/test_*.sh; do | |
| [ -f "$case_path" ] && [ -x "$case_path" ] || continue | |
| case_name=$(basename "$case_path" .sh) | |
| if [ -n "$filter" ]; then | |
| case "$case_name" in | |
| *"$filter"*) ;; | |
| *) continue ;; | |
| esac | |
| fi | |
| total=$((total + 1)) | |
| printf '%s\n' "$case_name" | |
| if "$case_path"; then |
🤖 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 `@tests/run.sh` around lines 21 - 35, Update the test discovery and execution
loop around case_path so only regular executable test_*.sh files are included by
checking both -f and -x, then invoke each selected case directly instead of
through sh, preserving its shebang behavior.
There was a problem hiding this comment.
Done. An executable case runs directly under its own shebang; a non-executable one still runs under sh so a forgotten chmod does not silently drop coverage.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tests/run.sh ---'
nl -ba tests/run.sh | sed -n '1,100p'Length of output: 472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,100p' tests/run.shLength of output: 1689
@RAprogramm, verified: executable cases now run directly under their own shebang, while non-executable regular files fall back to sh as intended. That is a reasonable coverage-preserving alternative to the original executable-only proposal.
One small follow-up remains: tests/run.sh still documents a case as “any executable tests/test_*.sh,” which no longer matches the implementation. Please update that wording (and the corresponding tests/README.md wording, if present) to describe the fallback behavior.
|
please resolve |
|
Resolved — rebased onto dev after #1870 landed, conflict is gone. While doing that CI caught something unrelated to the suite: checkout fails on this repo because the tree carries two gitlinks with no Neither is referenced anywhere — Also addressed the bot: workflow permissions and CI is green. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
I usually avoid adding tests ( Before I have a lot of time reading any lapses). I thought I can remember what I wrote. But now I think adding test is fine for me and for new users. |
|
Thanks for merging, and for the honest read on tests. The suite is deliberately cheap to live with: no framework, a case is one executable file in It already runs on the other open PRs and passes there, so nothing extra is needed on your side. |
|
Appreciated! |
2 participants
Pull Request
Description
Fixes #1867
Stacked on #1870. The suite fails against
devas it stands, because thedefects it detects are the ones that pull request removes. Merge #1870 first.
What lands
No framework is vendored. A case is an executable
tests/test_*.shthatprints its own diagnostics and exits non-zero, so adding a case is adding a
file.
What is checked
test_binds.shcode:NNform the Lua parser rejects; every bind carries a description; everyhyde-shellcommand a bind runs resolves, either to a file in the tree or to a subcommand hyde-shell declares in its own headertest_dots.shScripts/dotsparses, declarespathsandtarget_root, uses a known action, points at a source directory that exists, and lists only known package managerstest_lua_syntax.shtest_shell.shThe keybind case never talks to a compositor.
hlis replaced by a recorder,so the real
key_binds.lua,hyde/binds.luaandhyde/dispatcher.luaareexecuted and inspected without dispatching anything.
Effect on the tree
Three error-severity shellcheck findings existed and are fixed here, since a
linter that lands red is a linter nobody runs:
Configs/.config/uwsm/env.d/01-gpu.sh#!/usr/bin/env shas its sibling00-hyde.shConfigs/.local/lib/hyde/shutils/ocr.sh[@]inside a string argument (SC2145), which splits the notification body across arguments. Now[*]Configs/.local/lib/hyde/wallpaper.kon.sh[@]inside[[ ]](SC2199), so the substring test compared against an implicit concatenation. Now[*]Verification
Against this branch:
Against
devas it stands, the same suite reports 23 keybind defects and thethree shellcheck findings, so the checks bite rather than pass vacuously:
Known gap
Fourteen metafile entries point at source paths that no longer exist in the
tree, most of them the legacy Hyprland
.conffiles that the Lua releaseremoved. The path-level existence check is deliberately left out of this pull
request so it does not carry a judgement about what should happen to those
entries. It is filed separately.
Type of change
Checklist
tests/README.mddocuments how to run the suite and what each case covers.The changelog entry covers only the two shellcheck fixes that a user can
notice; the suite itself is not user-facing.
Additional context
The suite is dependency-light on purpose: a POSIX runner, one Lua harness and
one Python checker, no framework to vendor and no compositor to talk to. A
case whose tool is missing reports itself as skipped instead of failing, so it
stays usable on a contributor's machine and runs in full in CI.
pre-commit runpasses on the files this branch touches, and markdownlintpasses on the markdown it adds. The repository has 244 files with pre-existing
end-of-file and trailing-whitespace findings; folding those in would bury the
change, so they are left alone.
Summary by CodeRabbit