Skip to content

refactor: eliminate test duplication in workflow (resolves #60) #64

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.

Sign up for GitHub

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

Merged
maxrantil merged 2 commits into from
Nov 4, 2025
Merged

refactor: eliminate test duplication in workflow (resolves #60) #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bcf746
refactor: eliminate test duplication in workflow (resolves #60)
maxrantil Nov 4, 2025
a268f1a
docs: update session handoff for issue #60 completion
maxrantil Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
225 changes: 2 additions & 223 deletions .github/workflows/shell-quality.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,230 +61,9 @@ jobs:
TEMP_HOME=$(mktemp -d)
echo "TEMP_HOME=$TEMP_HOME" >> $GITHUB_ENV
echo "DIAGNOSTICS_DIR=$TEMP_HOME/diagnostics" >> $GITHUB_ENV
mkdir -p "$TEMP_HOME/diagnostics"

- name: Run install.sh in test home (first run)
id: first_run
run: |
# Environment isolation: Unset variables that could affect installation behavior
unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR
export HOME=$TEMP_HOME
START_TIME=$(date +%s%3N)
bash install.sh 2>&1 | tee $DIAGNOSTICS_DIR/install-first-run.log
END_TIME=$(date +%s%3N)
DURATION=$((END_TIME - START_TIME))
echo "first_run_duration=$DURATION" >> $GITHUB_OUTPUT
echo "First run took ${DURATION}ms"
env:
CI: true

- name: Verify key symlinks created
id: verify_symlinks
run: |
# CRITICAL SYMLINKS TESTED IN CI:
# These are essential for basic shell functionality:
# - .zshrc: Shell initialization in ZDOTDIR (required for zsh to function)
# - .zprofile: Shell profile (environment setup, sourced by zsh)
# - .aliases: Core command shortcuts (workflow dependency)
# - nvim/init.vim: Editor configuration (development dependency)
# - .tmux.conf: Session management (SSH workflow requirement)
# - starship.toml: Prompt configuration (startup validation)
#
# CONDITIONAL SYMLINKS (tested if present):
# - .gitconfig: User-specific, conditional linking
# - inputrc: Readline enhancement, non-critical
#
# MAINTENANCE: When adding new critical symlinks to install.sh,
# update the test cases below to verify them.
# Last updated: 2025-11-03 (6 critical symlinks + conditional)

# Check critical symlinks exist
ERRORS=0

# ZDOTDIR is set to $HOME/.config/zsh per .zprofile
ZDOTDIR="$TEMP_HOME/.config/zsh"

echo "Verifying critical symlinks..."

# Check .zshrc in ZDOTDIR
if [ ! -L "$ZDOTDIR/.zshrc" ]; then
echo "ERROR: .zshrc not linked in ZDOTDIR" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log
ERRORS=$((ERRORS + 1))
else
echo "✓ .zshrc (in ZDOTDIR)"
fi

# Check other critical symlinks
for link in ".zprofile" ".aliases" ".config/nvim/init.vim" ".tmux.conf" ".config/starship.toml"; do
if [ ! -L "$TEMP_HOME/$link" ]; then
echo "ERROR: $link not linked" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log
ERRORS=$((ERRORS + 1))
else
echo "✓ $link"
fi
done

# Check conditional symlinks if source exists
echo "Verifying conditional symlinks..."
if [ -f "$GITHUB_WORKSPACE/.gitconfig" ]; then
if [ ! -L "$TEMP_HOME/.gitconfig" ]; then
echo "ERROR: .gitconfig not linked (source exists)" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log
ERRORS=$((ERRORS + 1))
else
echo "✓ .gitconfig"
fi
fi

if [ -f "$GITHUB_WORKSPACE/inputrc" ]; then
if [ ! -L "$TEMP_HOME/.config/shell/inputrc" ]; then
echo "ERROR: inputrc not linked (source exists)" | tee -a $DIAGNOSTICS_DIR/symlink-errors.log
ERRORS=$((ERRORS + 1))
else
echo "✓ inputrc"
fi
fi

if [ $ERRORS -gt 0 ]; then
exit 1
fi

- name: Verify symlink targets
id: verify_targets
run: |
echo "Verifying symlink targets point to correct files..."
ERRORS=0

verify_target() {
local symlink="$1"
local expected_target="$2"

if [ -L "$TEMP_HOME/$symlink" ]; then
actual_target=$(readlink -f "$TEMP_HOME/$symlink")
if [ "$actual_target" != "$expected_target" ]; then
echo "ERROR: $symlink points to $actual_target, expected $expected_target" | tee -a $DIAGNOSTICS_DIR/target-errors.log
return 1
else
echo "✓ $symlink -> $actual_target"
fi
fi
}

verify_target ".zprofile" "$GITHUB_WORKSPACE/.zprofile" || ERRORS=$((ERRORS + 1))
verify_target ".aliases" "$GITHUB_WORKSPACE/.aliases" || ERRORS=$((ERRORS + 1))
verify_target ".config/nvim/init.vim" "$GITHUB_WORKSPACE/init.vim" || ERRORS=$((ERRORS + 1))
verify_target ".tmux.conf" "$GITHUB_WORKSPACE/.tmux.conf" || ERRORS=$((ERRORS + 1))
verify_target ".config/starship.toml" "$GITHUB_WORKSPACE/starship.toml" || ERRORS=$((ERRORS + 1))

if [ $ERRORS -gt 0 ]; then
exit 1
fi

- name: Check for broken symlinks
id: check_broken
run: |
echo "Checking for broken symlinks..."
cd $TEMP_HOME
if find . -type l ! -exec test -e {} \; -print | grep -q .; then
echo "ERROR: Found broken symlinks:" | tee $DIAGNOSTICS_DIR/broken-symlinks.log
find . -type l ! -exec test -e {} \; -print | tee -a $DIAGNOSTICS_DIR/broken-symlinks.log
exit 1
fi
echo "✓ No broken symlinks found"

- name: Test idempotency (second run)
id: second_run
run: |
echo "Testing idempotency - running install.sh again..."
unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR
export HOME=$TEMP_HOME
START_TIME=$(date +%s%3N)
bash install.sh 2>&1 | tee $DIAGNOSTICS_DIR/install-second-run.log
END_TIME=$(date +%s%3N)
DURATION=$((END_TIME - START_TIME))
echo "second_run_duration=$DURATION" >> $GITHUB_OUTPUT
echo "Second run took ${DURATION}ms"

- name: Verify backup functionality
id: verify_backup
run: |
echo "Verifying backup functionality..."

ZDOTDIR="$TEMP_HOME/.config/zsh"

# Create a real file (not symlink) that should be backed up
echo "test content" > "$TEMP_HOME/.test-backup-file"

# Run install.sh again - this should create backup if conflicts exist
unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR
export HOME=$TEMP_HOME
bash install.sh 2>&1 | tee $DIAGNOSTICS_DIR/install-backup-test.log

# Reset ZDOTDIR for verification checks
ZDOTDIR="$TEMP_HOME/.config/zsh"

# Check if backup directory was created (may not be if no conflicts)
BACKUP_DIR=$(find $TEMP_HOME -maxdepth 1 -name ".dotfiles_backup_*" -type d | head -1)

if [ -n "$BACKUP_DIR" ]; then
echo "✓ Backup directory created: $BACKUP_DIR"
else
echo "⚠ No backup directory created (no conflicts detected)"
fi

# Verify backup is not created for symlinks (should replace them silently)
if [ -L "$ZDOTDIR/.zshrc" ]; then
echo "✓ Symlinks replaced correctly without backup"
else
echo "ERROR: .zshrc is not a symlink after re-installation" | tee -a $DIAGNOSTICS_DIR/backup-errors.log
exit 1
fi

- name: Verify idempotency (no duplicates)
id: verify_idempotency
run: |
echo "Verifying no duplicate operations occurred..."

ZDOTDIR="$TEMP_HOME/.config/zsh"

# Check that symlinks still point to correct locations
test -L "$ZDOTDIR/.zshrc" || (echo "ERROR: .zshrc removed during second run" && exit 1)
test -L "$TEMP_HOME/.aliases" || (echo "ERROR: .aliases removed during second run" && exit 1)

# Verify no duplicate directories created
CONFIG_DIRS=$(find $TEMP_HOME/.config -type d -name "nvim" | wc -l)
if [ $CONFIG_DIRS -ne 1 ]; then
echo "ERROR: Duplicate directories created" | tee $DIAGNOSTICS_DIR/idempotency-errors.log
exit 1
fi

echo "✓ Installation is idempotent"

- name: Performance regression check
id: perf_check
run: |
FIRST_RUN=${{ steps.first_run.outputs.first_run_duration }}
SECOND_RUN=${{ steps.second_run.outputs.second_run_duration }}

echo "Performance metrics:"
echo " First run: ${FIRST_RUN}ms"
echo " Second run: ${SECOND_RUN}ms"

# Set baseline threshold (30 seconds = 30000ms)
THRESHOLD=30000

if [ $FIRST_RUN -gt $THRESHOLD ]; then
echo "WARNING: First run exceeded ${THRESHOLD}ms threshold" | tee $DIAGNOSTICS_DIR/performance-warning.log
fi

if [ $SECOND_RUN -gt $THRESHOLD ]; then
echo "WARNING: Second run exceeded ${THRESHOLD}ms threshold" | tee -a $DIAGNOSTICS_DIR/performance-warning.log
fi

# Save metrics for historical tracking
echo "first_run_ms=$FIRST_RUN" >> $DIAGNOSTICS_DIR/performance-metrics.txt
echo "second_run_ms=$SECOND_RUN" >> $DIAGNOSTICS_DIR/performance-metrics.txt

echo "✓ Performance check complete"
- name: Run comprehensive installation tests
run: ./tests/installation-test.sh "$TEMP_HOME" "$GITHUB_WORKSPACE"

- name: Upload diagnostic artifacts
if: failure()
Expand Down
129 changes: 83 additions & 46 deletions SESSION_HANDOVER.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,75 +1,112 @@
# Session Handoff: PR #59 Critical Fixes
# Session Handoff: [Issue #60] - Workflow Refactoring

**Date**: 2025-11-03
**Issue**: #52 - Enhancement: Improve installation testing coverage
**Branch**: fix/issue-52-installation-testing
**PR**: (to be created)
**Date**: 2025-11-04
**Issue**: #60 - Refactor GitHub Actions workflow to eliminate test duplication
**PR**: #64 - refactor: eliminate test duplication in workflow (resolves #60)
**Branch**: feat/issue-60-workflow-refactor

---

## ✅ Work Completed
## ✅ Completed Work

### Critical Fixes Applied (4 blockers)
### Workflow Refactoring
- **Eliminated duplication**: Removed ~240 lines of inline test logic from `.github/workflows/shell-quality.yml`
- **Single invocation**: Replaced with one call to `./tests/installation-test.sh`
- **Maintained functionality**: All 9 test scenarios still run, diagnostic artifacts still upload on failure
- **Result**: installation-test job reduced from ~240 lines to ~26 lines

1. **Security Fix (CVSS 9.0)**: Command injection vulnerability eliminated
- install.sh: Replaced `eval` with allowlist-based variable expansion
- Only safe patterns expanded: `${HOME}`, `${XDG_CONFIG_HOME}`, `$HOME`, `$XDG_CONFIG_HOME`
- Prevents arbitrary code execution via ZDOTDIR manipulation
### Changes Made
1. **File**: `.github/workflows/shell-quality.yml`
- Removed 9 separate inline test steps
- Added single step: `./tests/installation-test.sh "$TEMP_HOME" "$GITHUB_WORKSPACE"`
- Maintained diagnostic artifact upload on failure
- Kept temporary home directory setup

2. **Testing Fix**: Environment isolation for CI/local tests
- tests/installation-test.sh: Added `unset XDG_CONFIG_HOME XDG_DATA_HOME XDG_CACHE_HOME ZDOTDIR`
- .github/workflows/shell-quality.yml: Added environment isolation to all 3 install.sh invocations
- Prevents tests from inheriting parent environment settings
### Benefits Achieved
✅ Single source of truth for test logic
✅ Easier maintenance (update tests in one place)
✅ Guaranteed consistency between CI and local tests
✅ Reduced workflow complexity

3. **Code Quality**: Fixed 5 shellcheck SC2155 warnings
- tests/installation-test.sh lines 171, 252, 279, 303, 304
- Separated local variable declaration from assignment
- Prevents masking command failures

4. **Documentation**: Updated README.md with comprehensive test coverage
- Documented all 9 test scenarios from enhanced test suite
- Added standalone test script usage instructions
- Updated last modified date to 2025-11-03
---

5. **Formatting**: Applied shfmt formatting standards
- Fixed redirect spacing: `2> /dev/null` instead of `2>/dev/null`
- Fixed comment spacing: single space before inline comments
## 🎯 Current Project State

**Tests**: ✅ All CI checks passing on PR #64
**Branch**: feat/issue-60-workflow-refactor, clean working directory
**CI/CD**: ✅ All checks passed (9/9 passed, 0 failed)
**PR Status**: Draft, ready for review and merge

### CI Check Results (PR #64)
- ✅ Scan for Secrets
- ✅ Shell Format Check
- ✅ ShellCheck
- ✅ **Test Installation Script** (refactored workflow - PASSES!)
- ✅ Detect AI Attribution Markers
- ✅ Check Conventional Commits
- ✅ Analyze Commit Quality
- ✅ Run Pre-commit Hooks
- ⏭️ Check Session Handoff Documentation (skipping - expected)

### Agent Validation Status
- [x] architecture-designer: ✅ Complete (recommended this refactoring in issue #60)
- [ ] security-validator: N/A (no security changes)
- [x] code-quality-analyzer: ✅ Complete (YAML formatting verified)
- [ ] test-automation-qa: N/A (test logic unchanged, only location changed)
- [ ] performance-optimizer: N/A (no performance impact)
- [x] documentation-knowledge-manager: ✅ Complete (PR description documents changes)

---

## 🎯 Current State
## 🚀 Next Session Priorities

**Tests**: ✅ Docker build passes, install.sh creates all symlinks correctly
**Branch**: Up to date with origin, 2 commits ahead of master
**CI/CD**: Pending - awaiting final verification after latest fixes
**Security**: ✅ Command injection eliminated, no eval usage
**Immediate Next Steps:**
1. Review PR #64 and merge to master when ready
2. Close issue #60 (will auto-close via PR merge)
3. Consider tackling issue #61 (rollback script - HIGH priority) or #62 (shfmt caching - LOW priority)

### Commits in PR
1. `5d7ef4e` - feat: enhance installation testing coverage (resolves #52)
2. `d226206` - fix: resolve security, testing, and code quality issues
**Roadmap Context:**
- Issue #60: ✅ Complete (PR #64 ready to merge)
- Issue #61: Add automated rollback script (45-60 min, HIGH priority for production safety)
- Issue #62: Optimize CI with shfmt caching (10-15 min, LOW priority optimization)

---

## 📝 Startup Prompt for Next Session

Read CLAUDE.md to understand our workflow, then verify PR #59 CI passes and merge to master.
Read CLAUDE.md to understand our workflow, then review and merge PR #64 for issue #60.

**Immediate priority**: Verify CI pipeline passes (10 minutes)
**Context**: PR #59 fixes 4 critical blockers - security, testing, code quality, docs. All fixes applied and pushed.
**Reference docs**: PR #59, Issue #52, SESSION_HANDOVER.md (this file)
**Ready state**: Branch clean, all commits pushed, pre-commit hooks passing
**Immediate priority**: Review PR #64, merge to master (5-10 minutes)
**Context**: Issue #60 complete - workflow refactored successfully, all CI passing, eliminates ~240 lines of duplication
**Reference docs**: PR #64, Issue #60, SESSION_HANDOVER.md (this file)
**Ready state**: Clean master branch, PR #64 ready to merge, all tests passing

**Expected scope**: Monitor CI, address any remaining failures, merge PR #59 when green
**Expected scope**: Merge PR #64, then tackle issue #61 (rollback script) or #62 (shfmt caching)

---

## 📚 Key Reference Documents

- **PR #59**: https://github.com/maxrantil/dotfiles/pull/59
- **Issue #52**: https://github.com/maxrantil/dotfiles/issues/52
- **Previous session**: SESSION_HANDOVER.md from master branch (PR #55 context)
- **PR #64**: https://github.com/maxrantil/dotfiles/pull/64
- **Issue #60**: https://github.com/maxrantil/dotfiles/issues/60
- **Modified file**: `.github/workflows/shell-quality.yml`

---

## 📋 Notes & Observations

### Pre-existing Issue Identified
**Starship cache test failure**: Docker test (`./tests/docker-test.sh`) fails on Test 3 "Starship cache creation (Issue #7)" with message "Starship cache was not created". This failure:
- Also occurs on master branch (not caused by this PR)
- Is unrelated to the workflow refactoring
- May warrant a separate issue for investigation

### Commit Details
- **Commit**: `3bcf746` - refactor: eliminate test duplication in workflow (resolves #60)
- **Files changed**: 1 file, 2 insertions(+), 223 deletions(-)
- **Pre-commit hooks**: All passed

---

**Session Status**: ✅ FIXES COMPLETE, AWAITING CI VERIFICATION
**Next Action**: Monitor CI pipeline, merge when green
**Session Status**: ✅ ISSUE #60 COMPLETE, PR #64 READY TO MERGE
**Next Action**: Review PR #64 and merge when ready, then select next issue (#61 or #62)
Loading