refactor: explicit baseline archiving + #inspect on assertion/result - #214
Conversation
Guards the file-level side effect (base image moved over the actual image on pass, kept on failure) through both validate! and the delayed CapybaraScreenshotDiff.verify path, ahead of extracting the mutation into an explicit archive step.
Extract the hidden file mutation from the verify read path into an explicit ScreenshotAssertion#archive_baseline! (amendment item 5, 5.5-lite): validate now asks the pure difference question and then archives the baseline as a named, idempotent step; the legacy assert_image_not_changed class method delegates to the instance flow unchanged. Behavior-preserving: same files written at the same times, all pre-existing tests pass untouched. Add one-line #inspect to ScreenshotAssertion (name, state, paths; never triggers the comparison) and ComparisonResult (difference metrics + paths) for 3am debugging (item 8). The reporter error message keeps its exact byte format via to_h.to_json.
Reviewer's GuideRefactors screenshot verification so that baseline archiving is an explicit, idempotent operation and adds lightweight #inspect debug summaries to both screenshot assertions and comparison results while preserving existing external behavior and error formats. Sequence diagram for explicit baseline archiving during validationsequenceDiagram
participant Caller
participant ScreenshotAssertion
participant Comparison
participant FileSystem
Caller->>ScreenshotAssertion: validate
ScreenshotAssertion->>Comparison: different?
alt comparison differs
Comparison-->>ScreenshotAssertion: true
ScreenshotAssertion->>Comparison: error_message
ScreenshotAssertion-->>Caller: validation error
else comparison matches
Comparison-->>ScreenshotAssertion: false
ScreenshotAssertion->>ScreenshotAssertion: archive_baseline!
ScreenshotAssertion->>Comparison: base_image_path
ScreenshotAssertion->>Comparison: image_path
ScreenshotAssertion->>FileSystem: FileUtils.mv(base, image, force: true)
ScreenshotAssertion-->>Caller: nil
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 19 minutes Limit 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. On a passing comparison, the code moves the baseline file over the captured image and removes the temporary baseline, so a mistaken result can leave filesystem artifacts altered after the code is reverted. The affected screenshots can be regenerated or repaired, making the impact bounded rather than permanent application-data loss.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
5.5-lite items 5 + 8 (v2-core-redesign amendment). Behavior-preserving; no pre-existing test expectation changed.
Item 5 — explicit
archive_baseline!The verify path hid a file mutation inside the read-like
validatecall: on a passing comparison,ScreenshotAssertion.assert_image_not_changedsilentlyFileUtils.mv-ed the base image over the actual image (D5/B1 — the reasonvalidatewas not idempotent).ScreenshotAssertion#validatenow asks the pure difference question, then calls the new named, idempotent#archive_baseline!on pass — same files written at the same times.assert_image_not_changedclass method delegates to the instance flow (dsl_test pins its combined behavior; kept byte-identical).ScreenshotAssertionper the 5.5-lite trim — no Verifier, no Result type, no rename.Item 8 —
#inspectScreenshotAssertion#inspect: one line with name, state (pending/matches/different), new/base paths — never triggers the lazy comparison.ComparisonResult#inspect: one line withdifferent?,failed_by, area size, region, difference level, paths. The reporter error message keeps its exact former byte format by usingto_h.to_jsondirectly (previously routed throughinspect).Gate evidence
Guard-first: 4 end-to-end guards pinning archive-on-pass / keep-on-failure through both
validate!and delayedCapybaraScreenshotDiff.verify(commit 1), plus dsl_test's existing class-method pins.Mutation checks (each applied, run, reverted):
validatesilently skipsarchive_baseline!→ 3 tests red (the 2 new guards plus the pre-existing "cleans up base image when images are identical" pin in dsl_test.rb:247).#inspectreturnnil→ 3 inspect tests red (2 assertion + 1 result).Tests:
rake test:unit448 runs / 0 failures (baseline 439 at 99c71a6, +9 new);rake test481 runs / 0 failures / 6 skips; standardrb clean on changed files.🤖 Generated with Claude Code
Summary by Sourcery
Separate baseline archiving from comparison validation and improve debugging visibility for assertions and results.
Bug Fixes:
Enhancements:
Tests: