refactor: delete dead code and stale migration narrative (3.0 readiness) - #233
Merged
Conversation
Dead code (no caller in lib, test or docs): - ScreenshotAssertion.assert_image_not_changed - ScreenshotNamer#full_name_with_path / #current_group_directory, and the @screenshot_area duplication of Config#screenshot_area they were the only users of - three pure pass-throughs on Reporters::Default (save_annotation_for, annotate_difference, annotate_skip_areas) plus #save - VipsDriver.difference_area - Deprecation.warn's category: kwarg (one value in the tree) Structural: - AssertionRegistry#verify drops the guard verify_screenshots! already applies and stops computing failed_assertions.first outside the branch that reads it - DSL inlines two empty private hops (build_screenshot_assertion, screenshot_namer) Narrative: collapsed the 20 boilerplate ADR-step forwarder headers under lib/capybara* to one line each and stripped migration chronology from snap_diff.rb, dsl.rb, capybara_screenshot_diff.rb, legacy_shims.rb, image_compare.rb, capture/viewport.rb, screenshoter.rb and snap_manager.rb, keeping the live constraints. rake test:unit 532 -> 522, rake test 565 -> 555, both 0 failures.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
Reviewer's GuideRefactors snap_diff and Capybara integration for 3.0 readiness by deleting unused screenshot APIs and tests, simplifying deprecation/legacy forwarding, and tightening the DSL/assertion/driver flows without changing externally visible behavior. Sequence diagram for simplified screenshot assertion verificationsequenceDiagram
participant Registry as AssertionRegistry
participant Assertion as ScreenshotAssertion
participant Comparison as Comparison
participant Error as ExpectationNotMet
Registry->>Assertion: verify_screenshots!(screenshots)
Assertion->>Comparison: different?
Comparison-->>Assertion: result
Assertion-->>Registry: nil or errors
alt errors returned
Registry->>Registry: failed_assertions.first
Registry->>Error: new(message, caller)
Error-->>Registry: raise
end
Sequence diagram for inlined DSL screenshot assertion constructionsequenceDiagram
actor User
participant DSL as SnapDiff::DSL
participant Session as SnapDiff.session
participant Namer as ScreenshotNamer
participant Matcher as ScreenshotMatcher
participant Assertion as ScreenshotAssertion
User->>DSL: assert_matches_screenshot(name, options)
DSL->>Session: screenshot_namer
Session-->>DSL: Namer
DSL->>Namer: full_name(name)
Namer-->>DSL: full_name
DSL->>Matcher: new(full_name, options)
DSL->>Matcher: build_screenshot_assertion(skip_stack_frames)
Matcher-->>DSL: Assertion or nil
DSL->>Assertion: validate!
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (39)
📝 WalkthroughWalkthroughThis PR updates legacy compatibility documentation, simplifies deprecation APIs, removes obsolete forwarding methods, and streamlines screenshot assertion and naming paths. Runtime screenshot behavior remains unchanged except for the listed API removals and signature updates. ChangesCompatibility cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deletion sweep ahead of 3.0. Every removal below is backed by a search that
found no caller; nothing was deleted on the strength of a prior report alone.
Baseline
rake test:unit532/0 → 522/0;rake test565/0/6 →555/0/6. standardrb clean. No version/CHANGELOG changes.
Search tooling note: serena's Ruby LSP would not start in this worktree
(
LanguageServerTerminatedExceptiononinitialize), so call-site proof isgrepai hybrid search + exhaustive literal grep over the whole repo (all file
types, not just
.rb), plus a check thatsend/public_sendis never usedfor these names (
config.rb:194'spublic_sendis the config-attr generatorand takes only Config attribute names).
A. Dead code
ScreenshotAssertion.assert_image_not_changed(lib/snap_diff/screenshot_assertion.rb)test/unit/dsl_test.rb+ the definition. Only "caller" was a private test helper.ScreenshotNamer#full_name_with_pathscreenshot_namer_test.rb+ the definition. Zero lib callers.ScreenshotNamer#current_group_directoryscreenshot_namer_test.rb, 1 probe indiff_test.rb, the definition.ScreenshotNamer#screenshot_area+@screenshot_area+ the constructor argConfig#screenshot_area.ScreenshotNamer.newhad exactly one arg-passing call site, in the deleted test setup.Reporters::Default#save_annotation_for,#annotate_difference,#annotate_skip_areas,#saveAnnotationServiceimplementation it forwards to. Real callers useAnnotationServicedirectly. Class and thecompare.reporterseam kept.Deprecation.warn'scategory:kwargcategory:: 13 hits, all:constant— 1 inlegacy_shims.rb, 12 in the deprecation test. Word inlined intomessage_for.VipsDriver.difference_areavips_driver_test.rband the definition. Checked the flagged risk:docs/snapdiff.mdenumerates the custom-driver contract as instance methods (same_pixels?,find_difference_region,crop, …) plus the VIPS optional extras (filter_image_with_median,merge,highlight_mask,difference_level) —difference_areais a class method and appears in none of them.difference_area_size_by(the part actually used atvips_driver.rb:64) stays.Nothing was skipped as too risky in group A.
Tests removed, and why each was not a coverage loss
−10 tests, itemised:
#assert_image_not_changed cleans up base image when images are identical/keeps base image when images differ. These areline-for-line duplicates of
screenshot_assertion_test.rb's#archive_baseline! moves the base image over the actual image and is idempotent/#archive_baseline! keeps the baseline when the comparison differs.#assert_image_not_changederror-message testswere kept, not deleted. They are the only pin on the user-facing
multi-line error string that
#validateproduces, and nothing else coversit —
screenshot_assertion_test.rbonly asserts raise/no-raise. The privatehelper now builds the assertion directly instead of routing through the
deleted class method. Deleting them would have been deleting coverage, not
dead code.
#full_name_with_pathtests and 1#current_group_directorytest that pinned the deleted methods, plus#group= adds group to screenshot directory pathand#section= sets section for directory pathwhich asserted throughcurrent_group_directoryand duplicate#directory_parts contains group/section when set. Two tests named#full_name_with_path …only evercalled
#full_name; those were renamed and kept.VipsDriver.difference_area calculates correct area of difference.diff_test.rb'sscreenshot_dirhelper was repointed toFile.join(Capybara::Screenshot.screenshot_area, *directory_parts)so its 4live assertions on section/group directory layout survive the deletion.
B. Stale narrative
# Forwarder (ADR-004 v2 step 6): X now resolves lazily via snap_diff/legacy_shims' const_missing…headers over 2-line requires →one line each. Where a header carried a live constraint (drivers.rb's
same-object guarantee, region.rb's eager top-level alias,
reporters/default.rb's eager-and-silent rationale, image_compare.rb's
"must still provide everything the pre-move file did") the constraint was
kept and only the step numbering dropped.
lib/snap_diff.rb: the ~16-line acyclicity proof arguing against "the oldautoload-based wiring this replaced" → one sentence naming the live rule
(this lean entry must never load the umbrella;
snap_diff_test.rb's "barerequire never loads the umbrella" guard enforces it). The
capybara/dsland forwarder-path constraints were kept verbatim.
lib/snap_diff/dsl.rb: the same argument restated → the live constraintonly (must not require the umbrella, it would cycle back).
capybara_screenshot_diff.rb,legacy_shims.rb: dropped "ADR-008 step 2","ADR-004 v2 step 6", "(v2 step 4)" and the "unlike before this file's units
moved" aside; kept the eager-vs-lazy alias rules and every
defined?/const_defined?exception, which are live.capture/viewport.rb: dropped "when v3 adds scroll preservation … designsits own parameters here" — a promise, not documentation.
screenshoter.rb: dropped the staleTODO: Adds tests when this case is true/ODOT(resize_if_neededis tested now). The retina code itself isuntouched — it is live behaviour for Mac + Selenium + retina users.
snap_manager.rb: dropped# TODO: rename to delete!. Call: this is notworth a 3.0 rename —
cleanup!does more than delete (it also prunesattempt files and clears the tracked set), so
delete!would be the lessaccurate name. Nothing to carry forward.
C. Structural
C.1 —
AssertionRegistry#verify. Removed the duplicatedactive? && fail_on_differenceguard (verify_screenshots!applies it andreturns nil) and moved
failed_assertions.firstinside the raising branch:The flagged risk — that eagerly forcing every comparison through
different?is load-bearing for the reporter flow — was checked, not assumed. A throwaway
probe test printed, for three scenarios, which comparisons got forced
(
compare.processed?), whetherarchive_baseline!had moved the base image,and the raised error's backtrace and message. Run before the edit and again
after:
Identical. It holds because
Comparison#processedmemoises viadifferenceand@error_message ||= reporter.generate, sodifferent?doesits file-touching work exactly once regardless of who calls it first; both
orderings still force every comparison. The probe was deleted after use.
C.2 —
SnapDiff::DSL. Inlined the two genuinely empty private hops,build_screenshot_assertionandscreenshot_namer. The 4-layer spine isotherwise untouched. Note the
skip_stack_framesarithmetic: removing theprivate hop removes a real frame from the stack that
ScreenshotMatcher#create_screenshot_assertionwalks withcaller(n), so theargument goes
+1at the call site rather than the+2a naive inline wouldproduce.
dsl_test.rb'sour_screenshotstack-frame tests pass.Skipped
lib/snap_diff/config.rb— B asked for the "since step 7b" / "invertedfrom the original v2 consolidation" chronology at lines 5, 30, 122 and 188,
but config.rb is owned by a parallel lane and on the do-not-touch list. Left
as-is; the four comments are still there for whoever owns that file.
drivers.rb,utils.rb,config_legacy.rb,legacy_tree_is_alias_only_test.rb— parallel-lane files, not touched. No deletion in this sweep needed them.
require_relative "screenshot_namer"indsl.rbis now arguablyunnecessary (DSL reaches the namer through
SnapDiff.session), butscreenshot_assertion.rbrequires it anyway and pruning require lines is adifferent risk class. Left alone.
🤖 Generated with Claude Code
Summary by Sourcery
Prepare the codebase for 3.0 by removing verified dead code, simplifying affected flows, and eliminating stale migration narrative while preserving live compatibility behavior.
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
Documentation
Refactor
Tests