Skip to content

Add userwide config and some rudimentary testing (via bats)#44

Merged
yarikoptic merged 3 commits intomainfrom
enh-userwide-config
Mar 1, 2026
Merged

Add userwide config and some rudimentary testing (via bats)#44
yarikoptic merged 3 commits intomainfrom
enh-userwide-config

Conversation

@yarikoptic
Copy link
Member

planing to merge if all looks green and see whta copilot says

yarikoptic and others added 2 commits February 26, 2026 21:03
Source ${XDG_CONFIG_HOME:-~/.config}/yolo/config before the per-project
.git/yolo/config. Arrays (YOLO_PODMAN_VOLUMES, YOLO_PODMAN_OPTIONS,
YOLO_CLAUDE_ARGS) are merged from both configs; scalar flags in
project config override user-wide values. --no-config suppresses both.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
26 tests covering:
- expand_volume function (shorthand, options, full form, partial form)
- CLI flags (--help, --no-config, --anonymized-paths, --nvidia,
  --entrypoint, --worktree validation)
- Separator (--) and argument routing
- Config loading and merging (user-only, project-only, array merge,
  scalar override, --no-config suppression, XDG_CONFIG_HOME override,
  YOLO_PODMAN_OPTIONS, YOLO_CLAUDE_ARGS)
- Environment variable passthrough (CLAUDE_CONFIG_DIR, GIT_CONFIG_GLOBAL,
  CLAUDE_CODE_OAUTH_TOKEN)
- Container name generation
- Config template output

Uses bats-core with bats-support and bats-assert as git submodules.
Mock podman captures args to a file for assertion. CI runs on both
ubuntu-latest and macos-latest.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@yarikoptic yarikoptic requested a review from Copilot February 28, 2026 18:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds user-wide configuration support to yolo and introduces a BATS-based test suite with CI coverage to validate CLI/config behaviors.

Changes:

  • Add user-wide config loading via ${XDG_CONFIG_HOME:-~/.config}/yolo/config and merge it with per-project config.
  • Introduce BATS tests + helper utilities (with bats-support/bats-assert) to cover volume expansion, flag routing, and config behavior.
  • Add GitHub Actions CI workflow to run ShellCheck, BATS tests, and basic setup/integration checks.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
bin/yolo Adds user-wide config loading and array merging behavior; updates help/template text accordingly.
config.example Documents user-wide config location and merge/override semantics.
tests/yolo.bats Adds end-to-end and function-level BATS tests for flags, config loading, and argument routing.
tests/test_helper/common.bash Adds shared BATS helpers (mock podman, config writers, yolo runner, function loader).
tests/test_helper/bats-support Adds bats-support as a git submodule for testing utilities.
tests/test_helper/bats-assert Adds bats-assert as a git submodule for assertions.
.gitmodules Registers the two BATS helper submodules.
.github/workflows/ci.yml Adds CI jobs for ShellCheck, BATS tests, and setup/integration smoke checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +23 to +26
@test "expand_volume: full form host:container:options passed through" {
load_yolo_functions
run expand_volume "~/data:/data:Z"
assert_output "~/data:/data:Z"
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test codifies behavior that will likely break real usage: when expand_volume returns ~/data:/data:Z unchanged, podman -v will receive a literal ~ (tilde expansion won’t happen inside the string), so the host path typically won’t exist. Consider updating expand_volume to expand ~ on the host side even for host:container(:options) forms, and update this expectation accordingly (and similarly for the host:container case).

Suggested change
@test "expand_volume: full form host:container:options passed through" {
load_yolo_functions
run expand_volume "~/data:/data:Z"
assert_output "~/data:/data:Z"
@test "expand_volume: full form host:container:options expands host ~" {
load_yolo_functions
export HOME="$TEST_HOME"
run expand_volume "~/data:/data:Z"
assert_output "$TEST_HOME/data:/data:Z"

Copilot uses AI. Check for mistakes.
Comment on lines 41 to 48
# and `set -e`, then evals it. This is more robust than per-function sed
# extraction which depends on matching closing braces at column 0.
load_yolo_functions() {
eval "$(sed -n '2,/^# Parse arguments/{
/^set -e$/d
/^# Parse arguments/d
p
}' "$YOLO_BIN")"
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using eval to import code from bin/yolo makes the tests tightly coupled to an exact marker comment (# Parse arguments) and can be brittle to future refactors. A more maintainable pattern is to structure bin/yolo so it can be safely sourced without executing main logic (e.g., wrap execution in a main() and only invoke it when run as a script), letting tests import functions without eval/sed parsing.

Suggested change
# and `set -e`, then evals it. This is more robust than per-function sed
# extraction which depends on matching closing braces at column 0.
load_yolo_functions() {
eval "$(sed -n '2,/^# Parse arguments/{
/^set -e$/d
/^# Parse arguments/d
p
}' "$YOLO_BIN")"
# and `set -e`, then sources it. This is more robust than per-function sed
# extraction which depends on matching closing braces at column 0.
load_yolo_functions() {
source <(sed -n '2,/^# Parse arguments/{
/^set -e$/d
/^# Parse arguments/d
p
}' "$YOLO_BIN")

Copilot uses AI. Check for mistakes.
Address CI shellcheck failure and copilot review feedback:

- SC2155: Split local declarations from assignments in install_config()
- SC2034: Remove unused CLI_PODMAN_ARGS variable
- SC1090: Add shellcheck source=/dev/null directives for dynamic source
- Wrap execution code in main() with BASH_SOURCE guard so tests can
  source bin/yolo directly instead of fragile eval/sed extraction

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@yarikoptic yarikoptic requested a review from Copilot March 1, 2026 14:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +62 to +67
@test "--nvidia: podman args contain --device nvidia.com/gpu=all" {
run_yolo --nvidia
assert_success
podman_args_contain "--device"
podman_args_contain "nvidia.com/gpu=all"
}
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

podman_args_contain doesn't use run/assert_success, so if multiple checks exist in one test, an earlier failure can be masked by a later success (because the test's final exit status may end up 0). Recommendation: either (a) wrap each call with run podman_args_contain ... followed by assert_success, or (b) introduce an assert_podman_arg helper that performs run + assert_success for a single arg and use that consistently.

Copilot uses AI. Check for mistakes.
load_yolo_functions
export HOME="$TEST_HOME"
run expand_volume "~/data::ro"
assert_output "$TEST_HOME/data:$TEST_HOME/data:ro"
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test codifies that ~/data::ro expands without :Z, which is inconsistent with the default shorthand behavior (which appends :Z). If the intent is that :: keeps SELinux relabeling by default, consider updating expand_volume to append ,Z when options don't already include Z/z, and update this expectation accordingly; otherwise, update user-facing docs/examples to clearly state that :: options must explicitly include Z when needed.

Suggested change
assert_output "$TEST_HOME/data:$TEST_HOME/data:ro"
assert_output "$TEST_HOME/data:$TEST_HOME/data:ro,Z"

Copilot uses AI. Check for mistakes.
- name: Run full setup (automated)
run: |
# Answer 'no' to installation prompt
echo "n" | ./setup-yolo.sh
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In test-setup, the build-only invocation uses echo \"n\" | ./setup-yolo.sh || true, which suggests this path may exit non-zero. Here (integration-test) the same style of input is not guarded, so the workflow may fail if setup-yolo.sh exits non-zero after a 'no' response while still doing useful work. Recommendation: make setup-yolo.sh exit 0 when the user declines installation (if that's the intended successful path), or adjust this step to tolerate the expected non-zero exit and assert the desired side effects explicitly.

Suggested change
echo "n" | ./setup-yolo.sh
echo "n" | ./setup-yolo.sh || true

Copilot uses AI. Check for mistakes.
@yarikoptic
Copy link
Member Author

Thanks for the review @copilot! Here's where things stand:

Comment 2 (eval/sed brittleness): Already addressed in af66d08bin/yolo now has a main() guard with BASH_SOURCE check, and load_yolo_functions() simply does source "$YOLO_BIN". No more eval/sed.

Comment 3 (podman_args_contain without run/assert_success): Positive podman_args_contain calls that fail will still abort the test at the correct line — bats treats any non-zero command as a failure. We added refute_podman_arg with diagnostics for the trickier negative assertions. Adding run+assert_success wrappers around every positive check would add verbosity without meaningful benefit.

Comments 1 & 4 (tilde in full-form volumes, :Z in shorthand with options): These tests intentionally document expand_volume's current behavior. Changing tilde expansion in full-form or auto-appending ,Z to user-specified options would be behavioral changes to bin/yolo — valid improvements but out of scope for this testing PR. Filed as a follow-up concern.

Comment 5 (integration-test missing || true): Not actually an issue — setup-yolo.sh exits 0 when the user declines installation (line 259: exit 0). The test-setup job uses || true for a different reason (its step is a looser smoke test). The integration-test step correctly expects a clean exit.

Copy link
Contributor

Copilot AI commented Mar 1, 2026

@yarikoptic I've opened a new pull request, #45, to work on those changes. Once the pull request is ready, I'll request review from you.

@yarikoptic yarikoptic merged commit c6132b2 into main Mar 1, 2026
8 of 9 checks passed
@yarikoptic
Copy link
Member Author

not sure if anything else is needed but let's see what copilot comes up with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants