fix: an inherited GIT_DIR silently redirected every baseline lookup - #256
Conversation
`git -C <dir>` sets the working directory, but GIT_DIR and GIT_WORK_TREE override it. A suite launched from a git hook -- which exports both -- therefore read the WRONG repository: $ GIT_DIR=<other> git -C <repo> show HEAD:f.txt fatal: path 'f.txt' exists on disk, but not in 'HEAD' Every baseline lookup returns false, so with `fail_if_new` defaulting to false outside CI, each screenshot is recorded as new and the test PASSES. A visual suite that cannot find its baselines reports success. This is also why the repo's own pre-push hook cannot run the suite. Scrub the three inherited git env vars so `-C` means what this code already assumes it means.
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
Reviewer's GuideScrubs Git-related environment variables from all Git invocations so baseline lookups always use the intended repository, and adds a unit test ensuring checkout_vcs ignores an inherited GIT_DIR. Sequence diagram for repository-safe baseline checkoutsequenceDiagram
participant Suite
participant SnapDiff
participant Git
participant Repository
Suite->>SnapDiff: checkout_vcs(root, screenshot_path, checkout_path)
SnapDiff->>Git: system(GIT_ENV, git, -C, root, show, HEAD:file)
Note over SnapDiff,Git: GIT_ENV clears GIT_DIR, GIT_WORK_TREE, and GIT_INDEX_FILE
Git->>Repository: Resolve HEAD from root
Repository-->>Git: Baseline content
Git-->>SnapDiff: Write checkout_path
SnapDiff-->>Suite: Baseline available for comparison
Flow diagram for preventing inherited Git environment redirectionflowchart LR
A[Inherited GIT_DIR or related Git variables] --> B[GIT_ENV clears Git environment overrides]
B --> C[git -C root show HEAD:file]
C --> D[Correct repository baseline]
D --> E[need_to_compare? can compare screenshot]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The v2.0.0 section was written before #250, #253, #254, #255, #256, #261, #263, #264, #266 and #267 landed, and three of its claims had gone false: - "Known limitations: fork-based parallel tests produce no HTML report ... Fixed in 2.1" -- fixed in 2.0 by #266. Reproduced both sides here: 1.15.1 + `parallelize(workers: 2, threshold: 0)` writes NO report and prints no summary line; master writes one merged report and `4 verified, 4 changed, 0 new`. - "a suite whose only contact with the v1 API is `require \"capybara_screenshot_diff/minitest\"` + `include ...Assertions` still prints nothing" -- #263 made the require doors warn. That exact setup now prints the migration notice; verified in a scratch project. - "Two removals 2.0 cannot warn about ... `driver:` as a setting" -- #263 made both the setting writer and the per-screenshot key warn. Verified: `Capybara::Screenshot::Diff.driver = :vips` prints the removal line with a call site. And the silent-by-design constant list repeated the shape of the beta2 `defined?` mistake: it listed "Os, Region" inside a run of `Capybara::Screenshot::Diff::` names. Probed on master -- `defined?(Capybara::Screenshot::Diff::Os)` and `defined?(Capybara::Screenshot::Diff::Region)` are both nil. The real names are `Capybara::Screenshot::Os` and the top-level `Region`, neither of which existed under `::Diff` in 1.15.1 either. Fully qualified now, and `::Comparison` added to match docs/UPGRADING.md. New material, every claim checked against the code or a live run: - a "why upgrade" section for the four green-suite-testing-nothing bugs (#255, #256, #254, #266), plus the unfollowable CI message (#267) and the fail_if_new precedence change - before/after transcripts of the failure message (#264), taken from the same page rendered on 1.15.1 and on master - the summary line (#261), with the fact that it comes from the HTML reporter and needs its one-line require -- an omission that would have read as a missing feature - the #250 / #253 perf table, attributed to its harness, with columns labelled before/after rather than 1.x/2.0 - the libvips fix is stated as guarded on libvips 8.15+, so a reader on an older libvips knows the bug is still theirs Install snippets stay pinned to 2.0.0.beta3 on purpose: `~> 2.0` resolves to nothing on rubygems today. docs/RELEASE_PREP.md already carries a precise step to swap all five (its grep finds exactly those five), and gains one line so the record-modes placeholder in the entry cannot ship unfilled. `rake test:unit` 651 runs / 0 failures, `standardrb lib test` clean.
beta3 fixed the canonical entry points and shipped almost none of the behaviour. beta4 is the prerelease the 2.0.0 entry actually describes: the four green-suite bugs (#254 #255 #256 #258), the accept workflow (#259), the legible failure message (#264), and the deprecation warnings that make 2.1's removals visible (#246 #263). - `lib/snap_diff/version.rb` -> 2.0.0.beta4. Nothing else holds a version; the gemspec, the legacy version file and the mirror gemspec all read it. Verified with the release workflow's own guard command: `ruby -I lib -r capybara/screenshot/diff/version -e "puts Capybara::Screenshot::Diff::VERSION"` => 2.0.0.beta4 - CHANGELOG: a `[v2.0.0.beta4]` section written as the delta from beta3, plus the record-modes PLACEHOLDER filled from #259 now that it has shipped. `grep -n PLACEHOLDER CHANGELOG.md` returns nothing. - Install snippets moved beta3 -> beta4 in README, docs/UPGRADING.md and docs/migration-guide.md. They stay PINNED: `~> 2.0` resolves to nothing while only prereleases exist, so unpinning belongs to 2.0.0 final, not here. The notes name the #272 caveat explicitly. Removing `skip_area`'s implicit stabilization wait (10.012 s -> 0.009 s measured) means a selector not yet in the DOM now yields no mask, silently, where it previously resolved after the wait. #277's run-level tally shipped in the same beta as the replacement signal, and the notes say so rather than leaving it to be discovered. Gates: `rake test:unit` 720 runs / 2124 assertions / 0 failures / 0 skips under CI=true on 4.0.6, `standardrb lib test` clean over 161 files, and `gem build` produces capybara-screenshot-diff-2.0.0.beta4.gem (93 files, 13 docs, RELEASE_PREP correctly excluded).
Found while triaging a test failure another lane reported as environment-specific noise. It is not noise — it is a silent false pass.
git -C <dir>sets the working directory, butGIT_DIRoverrides it:So when a suite runs with
GIT_DIRin the environment — any git hook exports it —checkout_vcsreads the wrong repository, every baseline lookup fails, andneed_to_compare?is false. Sincefail_if_newdefaults to!ENV["CI"], the screenshot is recorded as new and the test passes. A visual regression suite that cannot find a single baseline reports success.This is the same failure class as the missing-baseline false green, reached by a different route, and it is why this repo's own
pre-pushhook cannot run the suite.The fix scrubs
GIT_DIR,GIT_WORK_TREEandGIT_INDEX_FILEfrom the child environment at all four git call sites, so-Cmeans what the surrounding code already assumes.Guard:
test/unit/vcs_test.rbrunscheckout_vcswithGIT_DIRpointed at an unrelated repository and asserts the baseline is still checked out at the right size. Mutation-checked — emptying the scrubbed-env hash reds it; restored, green.590 unit runs / 0 failures, standardrb clean.
Summary by Sourcery
Ensure Git baseline lookups honor their requested repository even when invoked from environments that define Git repository overrides.
Bug Fixes:
Tests: