refactor: session surface and reporter registration get SnapDiff homes (ADR-008 step 6) - #228
Conversation
ADR-008 step 6, the last alias-safe move of the accepted ADR. Session lifecycle: the per-test AssertionRegistry accessor moves to SnapDiff.session, with SnapDiff.reset and SnapDiff.pending_screenshots_message alongside it in snap_diff/screenshot_assertion.rb (the file that already owns the registry class). CapybaraScreenshotDiff keeps its entire surface as thin forwarders -- singleton_methods + arities dumped before and after, diff is empty (16 public + 1 private, unchanged). Thread.current is fiber-local; that is left exactly as it was (issue #217) -- this relocates the canonical accessor, it does not change the semantics. Reporting completion: SnapDiff::Reporting.register(reporter) is the canonical way in, with the append under the existing mutex (issue #217 item 2); .reporters stays public and mutable for compatibility. The integrations (Minitest, RSpec, Cucumber) and the HTML reporter's auto-registration now call SnapDiff.session / SnapDiff.reset / SnapDiff.pending_screenshots_message / SnapDiff::Reporting.finalize! / SnapDiff::Reporting.register directly instead of routing through CapybaraScreenshotDiff, and require the snap_diff files they actually use. AssertionRegistry#verify no longer reaches back through CapybaraScreenshotDiff for its own assertions -- same objects, one less round trip through the compat surface. Guards: SnapDiff.session and CapybaraScreenshotDiff.registry are the same object; register lands in the array .reporters exposes; 32 concurrent register calls retain all 32; requiring the HTML reporter auto-registers exactly one.
|
Warning Review limit reached
Next review available in: 13 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 (13)
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 GuideRefactors the per-test session and reporter registration to live under the SnapDiff namespace while keeping CapybaraScreenshotDiff’s public API as thin, alias-safe forwarders; adds a synchronized reporter registration path and tests that pin object identity and concurrency guarantees. Sequence diagram for test reset and reporter notificationsequenceDiagram
participant Adapter as Framework adapter
participant SnapDiff as SnapDiff
participant Session as AssertionRegistry session
participant Reporting as SnapDiff::Reporting
participant Reporter as Registered reporter
Adapter->>SnapDiff: reset()
SnapDiff->>Session: assertions
SnapDiff->>Reporting: notify(assertions)
Reporting->>Reporter: notify(assertions)
SnapDiff->>Session: reset()
Sequence diagram for synchronized reporter registrationsequenceDiagram
participant HTML as HTML reporter loader
participant Reporting as SnapDiff::Reporting
participant Mutex as reporters mutex
participant Reporters as reporters array
HTML->>Reporting: reporters
Reporting-->>HTML: reporters array
HTML->>Reporting: register(reporter)
Reporting->>Mutex: synchronize
Mutex->>Reporters: << reporter
Reporters-->>Mutex: updated array
Mutex-->>Reporting: release
Reporting-->>HTML: reporter
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ADR-008 step 6 — the last alias-safe move of the accepted ADR. Two things get their SnapDiff home: the per-test session and reporter registration.
(a) Session surface
CapybaraScreenshotDiff'sclass << selfblock owned the thread-local per-test registry. The canonical accessor now lives next to the class it hands back, inlib/snap_diff/screenshot_assertion.rb:SnapDiff.sessionAssertionRegistry(`Thread.current[...]SnapDiff.resetSnapDiff.pending_screenshots_messagepending_if_newskip messageEverything else the adapters need is already a registry method, so it reads
SnapDiff.session.<verb>— no new module-level forwarders were invented forverify/add_assertion/screenshot_namer/record_new_screenshot.Fiber-local semantics are unchanged.
Thread.current[]splits per fiber; that is pre-existing documented behaviour (issue #217) and this PR relocates the accessor only — no migration tothread_variable_get. The note moved with the code, andsnap_manager.rb's cross-reference to it was updated.Adapter-call inventory (what actually called
CapybaraScreenshotDiff.*)dsl.rb(×3)screenshot_namerSnapDiff.session.screenshot_namerdsl.rbadd_assertionSnapDiff.session.add_assertionscreenshot_matcher.rbrecord_new_screenshotSnapDiff.session.record_new_screenshotintegrations/minitest.rb,integrations/rspec.rbverifySnapDiff.session.verifyintegrations/{minitest,rspec,cucumber}.rbpending_screenshots_messageSnapDiff.pending_screenshots_messageintegrations/{minitest,rspec,cucumber}.rbresetSnapDiff.resetintegrations/{minitest,rspec,cucumber}.rbfinalize_reporters!SnapDiff::Reporting.finalize!reporters/html.rbreporters/reporters <<SnapDiff::Reporting.reporters/.registerAssertionRegistry#verify(internal)CapybaraScreenshotDiff.assertions/.registry.failed_assertionsassertions/failed_assertionsThose three integration files (and
reporters/html.rb) each carried a comment explaining that they had torequire "capybara_screenshot_diff/screenshot_assertion"because the registry machinery lived in the old file. They now requiresnap_diff/screenshot_assertion+snap_diff/reportingdirectly, and the comments say so.Public surface diff — must be empty, and is
CapybaraScreenshotDiffkeeps its entire surface as thin forwarders. Dumpedsingleton_methods(false)+ private singleton methods, with arity and fullparameters, on2ed7073and on this branch:16 public + 1 private (
notify_reporters), identical names, arities and parameter lists.resetandpending_screenshots_messageare hand-written one-line forwarders rather thandef_delegatorsprecisely to keep arity 0 — a Forwardable-generated method reports(*args, **kwargs, &block)and would have shown up in that diff.(b) Reporting completion
SnapDiff::Reporting.register(reporter)is new and appends under the existing mutex — that closes issue #217 item 2's unsynchronized-append hole on the canonical path..reportersstays public and mutable for compatibility (appending to it directly still works, it just skips the lock), soCapybaraScreenshotDiff.reporters/reporters_mutexare unaffected.reporters/html.rb's auto-registration uses.register.Guards added
All four are new pins on seams this PR moves:
registry_concurrency_test.rb) —SnapDiff.sessionandCapybaraScreenshotDiff.registrymust be the same object, not two registries that look alike; a write through one is visible through the other.reporters_mutex_test.rb) —registerreturns the reporter, andSnapDiff::Reporting.reportersis the same array objectCapybaraScreenshotDiff.reportersexposes, with the reporter in it.reporters_mutex_test.rb) — 32 threads registering concurrently, all 32 retained.reporters/html_reporter_test.rb) — requiring the reporter leaves exactly one HTML reporter registered. This seam had no coverage at all before (mutation iii below found that), and this PR changes the line that does it.Existing tests pass unchanged — no canonical-name migrations were forced by the raise-on-warn guard in
test_helper(the flipped call sites are all inlib/, and the compat surface the tests use is byte-identical).Mutation evidence
(i) forwarder shadows instead of shares —
CapybaraScreenshotDiff.registryreturnsSnapDiff::AssertionRegistry.newinstead ofSnapDiff.session:plus 2 collateral failures in the pre-existing concurrency tests. Restored → green.
(ii) unsynchronized
.register— dropping the mutex around@reporters << reporterleaves the threads probe green, 5/5 runs. That is MRI's GVL making a singleArray#<<effectively atomic, exactly as the task anticipated, so the probe is documented in the test as best-effort and the mutex is named as the actual defense. To show the probe still has teeth and that the mutex is what provides the guarantee, the same racy body was tested both ways:registerbodycurrent = @reporters.dup; Thread.pass; current << reporter; @reporters.replace(current)— no lock@mutex.synchronize(iii) html auto-registration broken — the register call replaced with a no-op. On the pre-existing suite this reds nothing (527 runs, 0 failures) — the seam was entirely unguarded. With the new probe:
Restored → green.
Test numbers (mise x ruby@4.0.6 -- bundle exec)
2ed7073rake test:unitrake test+4 in both = the four new guards; same skips. Baseline for the full suite was measured by checking out
2ed7073in this worktree and re-running, not inferred.standardrbon all 13 touched files: no offenses.🤖 Generated with Claude Code
Summary by Sourcery
Move per-test session and reporter registration ownership to their canonical SnapDiff namespaces while preserving backward compatibility.
New Features:
Bug Fixes:
Enhancements:
Tests: