[quality] add BATS tests for ublue-motd double-execution guard (PR #795 coverage) - #846
Conversation
📝 WalkthroughWalkthroughAdds a Bats test suite with an isolated sandbox and mocked ChangesMOTD guard test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
⛔ HOLD — do not merge This PR was filed by the quality agent (hold-gated mode). It requires human review before merging. Note: Please apply the following labels manually if not already set:
This PR adds 14 BATS tests for the double-execution guard logic introduced by #795, closing issue #845. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_ublue_motd_guard.bats (1)
48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrevent path quoting issues in
bash -c.Injecting path variables directly into a
bash -ccommand string using single quotes works for spaces, but can cause a syntax error if the repository clone path happens to contain a single quote character (e.g.,/home/user/my'repos/...). Passing file paths as positional arguments to the sub-shell is a safer convention.
tests/test_ublue_motd_guard.bats#L48-L48: Refactor torun bash -c 'source "$1"; echo "${UBLUE_MOTD_SHOWN:-unset}"' _ "${MOTD_PROFILE}"tests/test_ublue_motd_guard.bats#L55-L58: Refactor torun bash -c 'export UBLUE_MOTD_SHOWN=1; source "$1"' _ "${MOTD_PROFILE}"🤖 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/test_ublue_motd_guard.bats` at line 48, Update both bash -c invocations in tests/test_ublue_motd_guard.bats: at lines 48-48, pass MOTD_PROFILE as a positional argument and source it via "$1"; at lines 55-58, use the same positional-argument pattern while preserving the UBLUE_MOTD_SHOWN export. Avoid interpolating MOTD_PROFILE directly into the shell command string.
🤖 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 `@tests/test_ublue_motd_guard.bats`:
- Around line 1-11: Wire tests/test_ublue_motd_guard.bats into the repository’s
standard test execution by adding it to the appropriate test target in Justfile
and the unit-test workflow in .github/workflows/unit-tests.yml. Ensure both
local just test runs and CI execute this Bats test.
---
Nitpick comments:
In `@tests/test_ublue_motd_guard.bats`:
- Line 48: Update both bash -c invocations in tests/test_ublue_motd_guard.bats:
at lines 48-48, pass MOTD_PROFILE as a positional argument and source it via
"$1"; at lines 55-58, use the same positional-argument pattern while preserving
the UBLUE_MOTD_SHOWN export. Avoid interpolating MOTD_PROFILE directly into the
shell command string.
🪄 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: b65b4b68-ad04-41c4-a5f7-59366be17685
📒 Files selected for processing (1)
tests/test_ublue_motd_guard.bats
| #!/usr/bin/env bats | ||
| # Tests for the double-execution guard in system_files/shared/etc/profile.d/ublue-motd.sh | ||
| # and the env.sh existence guard in system_files/shared/usr/bin/ublue-motd. | ||
| # | ||
| # These tests validate the changes introduced by PR #795 (fix(motd): prevent | ||
| # double-execution and missing env.sh errors). | ||
| # | ||
| # Run: bats tests/test_ublue_motd_guard.bats | ||
|
|
||
| bats_require_minimum_version 1.5.0 | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Execute all tests using `just test` as instructed by coding guidelines.
just testRepository: projectbluefin/common
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'tests/test_ublue_motd_guard.bats' 'Justfile' 'AGENTS.md' | cat
printf '\n== test file ==\n'
cat -n tests/test_ublue_motd_guard.bats
printf '\n== just-related targets ==\n'
rg -n '^\s*(test|tests|bats|pytest)\b|just test|bats ' Justfile tests -S || trueRepository: projectbluefin/common
Length of output: 7765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== just test references ==\n'
rg -n '\bjust test\b|\btests/test_ublue_motd_guard\.bats\b|\btest_ublue_motd_guard\b' .github/workflows docs Justfile tests -S || true
printf '\n== workflow files ==\n'
git ls-files '.github/workflows/*' | catRepository: projectbluefin/common
Length of output: 1196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== unit-tests workflow ==\n'
sed -n '1,220p' .github/workflows/unit-tests.yml
printf '\n== run-testsuite workflow ==\n'
sed -n '1,220p' .github/workflows/run-testsuite.yml
printf '\n== testing docs ==\n'
sed -n '1,120p' docs/TESTING.mdRepository: projectbluefin/common
Length of output: 9694
Add this Bats test to the main test target
tests/test_ublue_motd_guard.bats isn’t wired into Justfile or .github/workflows/unit-tests.yml, so it won’t run through the repo’s standard just test/CI path yet.
🤖 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/test_ublue_motd_guard.bats` around lines 1 - 11, Wire
tests/test_ublue_motd_guard.bats into the repository’s standard test execution
by adding it to the appropriate test target in Justfile and the unit-test
workflow in .github/workflows/unit-tests.yml. Ensure both local just test runs
and CI execute this Bats test.
Source: Coding guidelines
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
Thanks for adding coverage, but this test file can't pass on its current base. It targets the guards from PR #795, which is still open (unmerged), so the source files these tests exercise don't exist on main:
system_files/shared/etc/profile.d/ublue-motd.sh— not present onmain.@test "ublue-motd.sh: first invocation runs ublue-motd"doesrun bash "${MOTD_PROFILE}"on a nonexistent path, which returns 127, so[ "${status}" -eq 0 ]fails.system_files/shared/usr/bin/ublue-motd— not present onmain; theenv.shguard tests can't run.fish_greeting.fishonmainis justfunction fish_greeting; umotd; end— it contains noUBLUE_MOTD_SHOWNand noset -gx UBLUE_MOTD_SHOWN 1, so@test "fish_greeting: contains UBLUE_MOTD_SHOWN guard"and the sibling static assertions fail.
To land this, either rebase it onto PR #795's branch (or wait until #795 merges and re-target main), so the code under test actually ships alongside the tests. As written against main the suite is red. Happy to re-review once the base includes the guard implementation.
castrojo
added
4-review
3 participants
Test Improvement
Adds
tests/test_ublue_motd_guard.bats(14 tests) covering the double-executionguard and env.sh existence guard introduced by PR #795.
New test coverage
ublue-motd.sh(profile.d entry)ublue-motdand exportsUBLUE_MOTD_SHOWN=1UBLUE_MOTD_SHOWN=1set) does NOT runublue-motdagain~/.config/no-show-user-motdexistsusr/bin/ublue-motdbinaryenv.shis missing (graceful skip — was a hard abort before fix(motd): prevent double-execution and missing env.sh errors #795)env.shnormally when it existsfish_greeting.fishUBLUE_MOTD_SHOWNguard (static assertions)UBLUE_MOTD_SHOWN=1before returningno-show-user-motdopt-out fileRelated Issue
Closes #845
Filed by quality agent (hold-gated mode). Human review required.
Summary by CodeRabbit