fix: isolate diff tests from local dotfiles repo state#22
fix: isolate diff tests from local dotfiles repo state#22fullstackjam merged 4 commits intoopenbootdotdev:mainfrom
Conversation
The previous implementation only checked --global git config, but users may have their git identity configured in: - Local repository config (.git/config) - System config (/etc/gitconfig) - Other scopes This change first tries --global, then falls back to checking all scopes if --global returns empty. This ensures we detect git configuration regardless of where it's set. Closes git config detection issue
Adds TestGetGitConfig_FallsBackToAnyScope to verify that GetGitConfig checks all git config scopes (global, local, system) when looking for user.name and user.email, not just --global. Related to git config detection issue
The diffDotfiles function was always checking the local ~/.dotfiles git state, even when comparing snapshots with empty dotfiles URLs. This caused test failures when the user's actual dotfiles repo had uncommitted changes. Fix: Only check dotfiles repo state if at least one URL is configured. If both system and reference have empty dotfiles URLs, skip the git state check entirely. This makes the tests deterministic and not dependent on the user's local dotfiles repo state.
|
👋 Thanks for opening this pull request! 🎉 This is your first PR to OpenBoot — welcome! Before merging:
@fullstackjam will review this soon. Thanks for contributing! 🚀 |
fullstackjam
left a comment
fullstackjam
left a comment
There was a problem hiding this comment.
The goal of isolating tests from local state is correct, but the fix is incomplete:
[HIGH] Early return only handles the all-empty case
The guard only kicks in when both URLs are empty. If one side has a URL configured, the function still reads the real ~/.dotfiles git state, so the test isolation problem isn't fully solved.
[HIGH] The fix should be at the test level, not in production code
The right approach is to isolate the tests themselves: use t.Setenv("HOME", tmpDir) to redirect HOME, then git init a clean fake .dotfiles repo under tmpDir. This gives full control over the test environment without bypassing any production code paths.
[MEDIUM] The system.go change should be a separate PR
The GetGitConfig fallback change is unrelated to test isolation. It's a reasonable fix on its own, but bundling it here makes both changes harder to review and revert independently.
[MEDIUM] assert.IsType(t, "", value) adds no value
The Go type system already guarantees the return type at compile time. Replace this with an assertion on the actual returned value.
Address PR openbootdotdev#22 review feedback: - Remove early return guard from diffDotfiles() that bypassed production code paths when both URLs are empty - Add isolateHome() test helper that sets HOME to a temp dir with a clean .dotfiles git repo, giving full test isolation without modifying prod code - Revert GetGitConfig() fallback change (belongs in separate PR openbootdotdev#21) - Remove TestGetGitConfig_FallsBackToAnyScope test (paired with reverted change) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
Fixes test isolation issue where diff tests failed depending on the user's local ~/.dotfiles git state.
Problem
diffDotfiles()always checks the actual~/.dotfilesgit repository for uncommitted changes. Tests callingCompareSnapshotsorCompareSnapshotToRemotewould fail unpredictably depending on the developer's local dotfiles state.Solution
Isolate tests by redirecting HOME to a temp directory with a clean
.dotfilesgit repo, rather than modifying production code to skip code paths. This ensures all production code paths are fully exercised while keeping tests deterministic.Also reverts the
GetGitConfig()fallback change (belongs in separate PR #21).Changes
diffDotfiles()— production code should not be modified for test convenienceisolateHome()test helper usingt.TempDir()+t.Setenv("HOME", tmpDir)+git initisolateHome(t)in all diff tests that invoke snapshot comparison functionsGetGitConfig()to original form (only--global, no fallback to baregit config)TestGetGitConfig_FallsBackToAnyScopetest (paired with reverted change)Testing
make test-unit🤖 Generated with Claude Code