-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat(validation): add configuration validation utilities #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Implemented validation functions for Mise config, tool versions, Git config, shell config, Brewfile, and Chezmoi directory. - Added comprehensive validation function to check all configurations at once. test(validation): add end-to-end tests for configuration validation - Created BATS tests to verify the functionality of validation utilities. - Included tests for Mise config, tool versions, Git config, Brewfile, and Chezmoi directory. test(e2e): add tests for bootstrap workflow - Implemented tests to ensure bootstrap script and installation scripts are executable and functional. - Verified the presence and execution of library scripts. test(helpers): add custom assertions for BATS tests - Created helper functions for common assertions in BATS tests to improve readability and maintainability. test(unit): add unit tests for installation scripts - Added unit tests for install/homebrew.sh, install/mise.sh, install/chezmoi.sh, and install/brewfile.sh. - Verified the functionality of installation checks and commands. chore(tests): improve test infrastructure and organization - Structured tests into unit and e2e directories for better organization. - Enhanced test helper functions for improved test setup and teardown.
…n scripts - Standardized indentation and formatting in homebrew.sh, mise.sh, bootstrap.sh, coverage.sh, and validation.sh. - Added shellcheck directives to suppress specific warnings. - Enhanced logging functions for better clarity. - Streamlined validation functions for configuration files. - Improved error handling and user feedback in various scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces comprehensive testing infrastructure for the dotfiles repository, including validation utilities, unit tests for installation scripts, end-to-end tests, and CI/CD integration. The changes establish a robust testing framework using BATS (Bash Automated Testing System) while maintaining the existing chezmoi-managed dotfiles structure.
Key Changes:
- Added validation functions for configuration files (mise, git, Brewfile, etc.)
- Implemented unit tests for all installation scripts with mocking capabilities
- Created end-to-end tests for bootstrap workflows and configuration validation
- Established CI/CD pipelines for automated testing on both macOS and Linux
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/install/*.bats | Unit tests for homebrew, mise, chezmoi, and brewfile installation scripts |
| tests/test_helper.bash | Test setup/teardown functions and mocking utilities |
| tests/helpers/assertions.bash | Custom assertion functions for BATS tests |
| tests/e2e/*.bats | End-to-end tests for bootstrap workflow and configuration validation |
| scripts/lib/validation.sh | Configuration validation utilities for all dotfiles |
| scripts/lib/detect.sh | OS and environment detection utilities |
| scripts/lib/common.sh | Shared logging and utility functions |
| scripts/bootstrap.sh | Main bootstrap orchestration script |
| scripts/coverage.sh | Code coverage report generation using kcov |
| install/*.sh | Installation scripts for homebrew, mise, chezmoi, and brewfile management |
| mise.toml | Task definitions for running tests, linters, and other development tasks |
| .github/workflows/*.yml | CI/CD workflows for unit tests and E2E testing |
| README.md | Updated documentation with testing instructions and bootstrap workflow |
| Brewfile | Package manifest including testing tools |
| .chezmoiignore | Updated to exclude test infrastructure from deployment |
| run install_mise_tools "${TEST_TEMP_DIR}/nonexistent.toml" | ||
| [ "$status" -eq 0 ] | ||
| [[ "$output" =~ "Using .tool-versions" ]] |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test verifies that install_mise_tools falls back to .tool-versions when the config.toml doesn't exist, but it doesn't verify that mise install was actually called with the correct fallback configuration. Consider adding a verification step to ensure the TOOL_VERSIONS_PATH was used.
|
|
||
| @test "get_homebrew_prefix returns /opt/homebrew on arm64" { | ||
| # Mock uname to return arm64 | ||
| mock_command "uname" "arm64" 0 |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test mocks uname to return 'arm64', but real uname -m output format varies by system. Consider testing with actual uname output format to ensure the function handles real-world responses correctly.
| export CI="true" | ||
| export GITHUB_ACTIONS="false" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting CI=true globally in setup may cause tests to behave differently than in actual non-CI environments. Consider only setting these variables in specific tests that need to verify CI-specific behavior, or document why this is set globally.
| export CI="true" | |
| export GITHUB_ACTIONS="false" | |
| # NOTE: Do not set CI or GITHUB_ACTIONS globally here. | |
| # If a test needs to simulate CI, set these variables in that test. |
| ((line_num++)) | ||
| # Skip empty lines and comments | ||
| [[ -z "$line" ]] || [[ "$line" =~ ^# ]] && continue | ||
|
|
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern for validating tool versions may be too restrictive. Some tools use version formats like '20.x' or 'latest' that include additional characters. Consider expanding the pattern or documenting the expected format.
| # Allow tool names with [a-zA-Z0-9_-]+, and version strings with common characters: [a-zA-Z0-9._\-/*+]+ | |
| # This supports versions like '20.x', 'latest', 'lts/*', 'stable', etc. | |
| if ! [[ "$line" =~ ^[a-zA-Z0-9_-]+[[:space:]]+[a-zA-Z0-9._\-/*+]+$ ]]; then |
| local test_count=0 | ||
| while IFS= read -r test_file; do | ||
| ((test_count++)) | ||
| log_info "[$test_count] Running: $(basename "$test_file")" | ||
|
|
||
| # Run kcov with test file | ||
| kcov \ | ||
| --exclude-pattern=/usr/include,/tmp,/opt/homebrew \ | ||
| "$COVERAGE_DIR/$(basename "$test_file" .bats)" \ | ||
| "$test_file" || log_warning "Test failed: $test_file" | ||
| done < <(find "${REPO_ROOT}/tests/unit" -name "*.bats" -type f) | ||
|
|
||
| log_info "" | ||
| log_success "Coverage generation complete!" | ||
| log_info "Coverage report: $COVERAGE_DIR/index.html" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling suppresses test failures with || log_warning, which means failed tests won't cause the coverage script to exit with a non-zero status. This could hide test failures. Consider collecting failures and exiting with an error status if any tests fail.
| local test_count=0 | |
| while IFS= read -r test_file; do | |
| ((test_count++)) | |
| log_info "[$test_count] Running: $(basename "$test_file")" | |
| # Run kcov with test file | |
| kcov \ | |
| --exclude-pattern=/usr/include,/tmp,/opt/homebrew \ | |
| "$COVERAGE_DIR/$(basename "$test_file" .bats)" \ | |
| "$test_file" || log_warning "Test failed: $test_file" | |
| done < <(find "${REPO_ROOT}/tests/unit" -name "*.bats" -type f) | |
| log_info "" | |
| log_success "Coverage generation complete!" | |
| log_info "Coverage report: $COVERAGE_DIR/index.html" | |
| local test_failed=0 | |
| while IFS= read -r test_file; do | |
| ((test_count++)) | |
| log_info "[$test_count] Running: $(basename "$test_file")" | |
| # Run kcov with test file | |
| if ! kcov \ | |
| --exclude-pattern=/usr/include,/tmp,/opt/homebrew \ | |
| "$COVERAGE_DIR/$(basename "$test_file" .bats)" \ | |
| "$test_file" | |
| then | |
| log_warning "Test failed: $test_file" | |
| test_failed=1 | |
| fi | |
| done < <(find "${REPO_ROOT}/tests/unit" -name "*.bats" -type f) | |
| log_info "" | |
| log_success "Coverage generation complete!" | |
| log_info "Coverage report: $COVERAGE_DIR/index.html" | |
| if [[ "$test_failed" -ne 0 ]]; then | |
| log_error "One or more tests failed." | |
| exit 1 | |
| fi |
| description = "Export current Homebrew packages to Brewfile at ~/.config/homebrew" | ||
| run = """ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| echo "Dumping Brewfile to ~/.config/homebrew/Brewfile..." | ||
| # Create directory if it doesn't exist | ||
| mkdir -p ~/.config/homebrew | ||
| # Export Brewfile | ||
| brew bundle dump --force --file="$HOME/.config/homebrew/Brewfile" | ||
| echo "Brewfile exported successfully!" | ||
| echo "" | ||
| echo "Next steps:" | ||
| echo " 1. Review: cat ~/.config/homebrew/Brewfile" | ||
| echo " 2. Add to chezmoi: chezmoi add ~/.config/homebrew/Brewfile" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The brew:dump task writes to ~/.config/homebrew/Brewfile, but the repository's Brewfile is at the root. This creates potential confusion about which Brewfile is the source of truth. Consider documenting the relationship between these files or consolidating to a single location.
| description = "Export current Homebrew packages to Brewfile at ~/.config/homebrew" | |
| run = """ | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| echo "Dumping Brewfile to ~/.config/homebrew/Brewfile..." | |
| # Create directory if it doesn't exist | |
| mkdir -p ~/.config/homebrew | |
| # Export Brewfile | |
| brew bundle dump --force --file="$HOME/.config/homebrew/Brewfile" | |
| echo "Brewfile exported successfully!" | |
| echo "" | |
| echo "Next steps:" | |
| echo " 1. Review: cat ~/.config/homebrew/Brewfile" | |
| echo " 2. Add to chezmoi: chezmoi add ~/.config/homebrew/Brewfile" | |
| description = "Export current Homebrew packages to the canonical Brewfile at the repository root" | |
| run = """ | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| # Set the path to the canonical Brewfile in the dotfiles repo root | |
| DOTFILES_REPO="${HOME}/dotfiles" | |
| BREWFILE_PATH="${DOTFILES_REPO}/Brewfile" | |
| echo "Dumping Brewfile to ${BREWFILE_PATH} (repo root)..." | |
| # Export Brewfile | |
| brew bundle dump --force --file="${BREWFILE_PATH}" | |
| echo "Brewfile exported successfully!" | |
| echo "" | |
| echo "Next steps:" | |
| echo " 1. Review: cat ${BREWFILE_PATH}" | |
| echo " 2. Commit the updated Brewfile to your dotfiles repository" |
| description = "Export current Homebrew packages to Brewfile at ~/.config/homebrew" | ||
| run = """ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| echo "Dumping Brewfile to ~/.config/homebrew/Brewfile..." | ||
| # Create directory if it doesn't exist | ||
| mkdir -p ~/.config/homebrew | ||
| # Export Brewfile | ||
| brew bundle dump --force --file="$HOME/.config/homebrew/Brewfile" | ||
| echo "Brewfile exported successfully!" | ||
| echo "" | ||
| echo "Next steps:" | ||
| echo " 1. Review: cat ~/.config/homebrew/Brewfile" | ||
| echo " 2. Add to chezmoi: chezmoi add ~/.config/homebrew/Brewfile" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The brew:dump task writes to ~/.config/homebrew/Brewfile, but the repository's Brewfile is at the root. This creates potential confusion about which Brewfile is the source of truth. Consider documenting the relationship between these files or consolidating to a single location.
| description = "Export current Homebrew packages to Brewfile at ~/.config/homebrew" | |
| run = """ | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| echo "Dumping Brewfile to ~/.config/homebrew/Brewfile..." | |
| # Create directory if it doesn't exist | |
| mkdir -p ~/.config/homebrew | |
| # Export Brewfile | |
| brew bundle dump --force --file="$HOME/.config/homebrew/Brewfile" | |
| echo "Brewfile exported successfully!" | |
| echo "" | |
| echo "Next steps:" | |
| echo " 1. Review: cat ~/.config/homebrew/Brewfile" | |
| echo " 2. Add to chezmoi: chezmoi add ~/.config/homebrew/Brewfile" | |
| description = "Export current Homebrew packages to a staging Brewfile (~/.config/homebrew/Brewfile). Review and copy to ./Brewfile to update the repo's source of truth." | |
| run = """ | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| echo "Dumping Brewfile to ~/.config/homebrew/Brewfile (staging location)..." | |
| # Create directory if it doesn't exist | |
| mkdir -p ~/.config/homebrew | |
| # Export Brewfile to staging location | |
| brew bundle dump --force --file="$HOME/.config/homebrew/Brewfile" | |
| echo "Brewfile exported to ~/.config/homebrew/Brewfile." | |
| echo "" | |
| echo "IMPORTANT: The canonical Brewfile for this repository is at the repo root (./Brewfile)." | |
| echo "To update the tracked Brewfile, review the changes and copy the staging file to the repo root:" | |
| echo "" | |
| echo " diff ./Brewfile ~/.config/homebrew/Brewfile" | |
| echo " cp ~/.config/homebrew/Brewfile ./Brewfile" | |
| echo "" | |
| echo "Then add and commit the updated ./Brewfile to version control." | |
| echo "" | |
| echo "If using chezmoi, you may also want to run: chezmoi add ./Brewfile" |
| sudo apt-get install -y bats shellcheck | ||
| # Install shfmt manually on Linux | ||
| curl -fsSL https://github.com/mvdan/sh/releases/download/v3.7.0/shfmt_v3.7.0_linux_amd64 -o /tmp/shfmt |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shfmt version is hardcoded in the workflow. Consider using a variable or 'latest' tag to make version updates easier, or at minimum extract the version to a variable at the top of the job for easier maintenance.
| # Allow some empty directories (like subdirectories) | ||
| [ "$empty_dirs" -lt 5 ] |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number '5' for allowed empty directories is arbitrary and undocumented. This threshold may need adjustment as the repository grows. Consider either removing this test, making it more specific, or documenting why 5 is the appropriate threshold.
| # Allow some empty directories (like subdirectories) | |
| [ "$empty_dirs" -lt 5 ] | |
| # Allow some empty directories (like subdirectories). | |
| # The threshold of 5 is arbitrary and may need adjustment as the repository grows. | |
| # If you add more empty directories for structure or future use, update this value accordingly. | |
| local max_empty_dirs=5 | |
| [ "$empty_dirs" -lt "$max_empty_dirs" ] |
There was a problem hiding this 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 26 out of 27 changed files in this pull request and generated no new comments.
|
@copilot fix CI failures |
|
@andrewmcodes I've opened a new pull request, #21, to work on those changes. Once the pull request is ready, I'll request review from you. |
test(validation): add end-to-end tests for configuration validation
test(e2e): add tests for bootstrap workflow
test(helpers): add custom assertions for BATS tests
test(unit): add unit tests for installation scripts
chore(tests): improve test infrastructure and organization
PR Purpose
The context for this pull request or the Conventional Commits type that best matches the changes.
Changes Summary
Summarize what you've changed and why. Make sure to mention if this is a breaking change.
Screenshots
Add screenshots of the feature in action or a video/GIF walkthrough of the UX. Remove if not applicable (e.g. refactoring).
Areas of interest
Edge cases, workarounds, hacks, security concerns, and other areas of the code you'd like the reviewer to pay extra attention to or give feedback on.