test: pin config default-eval timing across entry points (ADR-005 step 1 guard) - #223
Conversation
…p 1 guard) Subprocess probes that pin, for each documented entry point (capybara_screenshot_diff, capybara_screenshot_diff/minitest, snap_diff, capybara/screenshot/diff): - every SnapDiff::Config::MAPPING setting reads identically through both surfaces (SnapDiff.config.x and the legacy mattr_accessor) and matches the expected default - fail_if_new's ENV["CI"] read happens at require time and is frozen: mutating ENV after the require (even before the first read) is not seen - root's Rails.root / pwd fallback is likewise evaluated at require time and frozen against post-require Rails.root reassignment or chdir - Diff.default_options[:wait] is the opposite: it reads Capybara.default_max_wait_time live, at call time These are green on current master by design: they pin CURRENT behavior so the coming ADR-005 storage inversion (SnapDiff::Config becoming the single store, config_legacy flipped to delegating writers) goes red if it shifts when any default is evaluated or what it evaluates to. Gate-checked: temporarily hoisting the fail_if_new ENV read from class-body eval to a lazy read-time default turned 8 of the 12 probes red; reverting restored green.
Reviewer's GuideAdds a new unit test suite that probes and pins the timing and value snapshots of configuration defaults across all documented entry points, ensuring require-time vs call-time behavior stays stable through upcoming config storage refactors. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 22 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 (1)
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 found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test/unit/config_default_timing_test.rb" line_range="65-68" />
<code_context>
+ require "tmpdir"
+ Dir.chdir(Dir.tmpdir)
+
+ # 1) Every mapped setting reads the same through both surfaces.
+ SnapDiff::Config::MAPPING.each do |name, (mod, mattr)|
+ check("SnapDiff.config.#{name} vs #{mod}.#{mattr}", mod.public_send(mattr), SnapDiff.config.public_send(name))
+ end
+
+ # 2) Expected default values (CI unset, no Rails, at require time).
</code_context>
<issue_to_address>
**issue (testing):** The probe does not assert that every key in `SnapDiff::Config::MAPPING` has an expected default. It only checks that mapped values agree between the two surfaces, while the separate hard-coded expectations can silently omit a newly added mapping and allow its incorrect default to pass.
**Triggers:** When a setting is added to `MAPPING` without also being added to the hard-coded expectation hash.
**Suggested fix:** Build the expected values from a complete key set and assert `SnapDiff::Config::MAPPING.keys.sort == expected.keys.sort` before checking the values.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: test/unit/config_default_timing_test.rb:68
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # 1) Every mapped setting reads the same through both surfaces. | ||
| SnapDiff::Config::MAPPING.each do |name, (mod, mattr)| | ||
| check("SnapDiff.config.#{name} vs #{mod}.#{mattr}", mod.public_send(mattr), SnapDiff.config.public_send(name)) | ||
| end |
There was a problem hiding this comment.
issue (testing): The probe does not assert that every key in SnapDiff::Config::MAPPING has an expected default. It only checks that mapped values agree between the two surfaces, while the separate hard-coded expectations can silently omit a newly added mapping and allow its incorrect default to pass.
Triggers: When a setting is added to MAPPING without also being added to the hard-coded expectation hash.
Suggested fix: Build the expected values from a complete key set and assert SnapDiff::Config::MAPPING.keys.sort == expected.keys.sort before checking the values.
…p 7b) (#230) config_legacy.rb was the last file in the v1 trees holding real logic. Step 1 moved config STORAGE to SnapDiff::Config but left the DERIVED values behind, so deleting lib/capybara/ at 3.0 would have lost behaviour rather than being a `git rm`. Moved into SnapDiff::Config as instance methods: - #active? (was Capybara::Screenshot.active?) - #screenshot_area (was Capybara::Screenshot.screenshot_area) - #screenshot_area_abs - #default_options (was Diff.default_options, incl. the vips tolerance 0.001 literal) Inverted so the canonical names hold the bodies: - SnapDiff.compare now builds the Comparison; Diff.compare forwards. - SnapDiff.start now does the two-arg yield; Diff.configure forwards. The Config::MAPPING accessor generator moved to snap_diff/config.rb alongside the storage it delegates to -- same reason legacy_shims.rb generates the legacy constants from the snap_diff side: the generator is code, and the v1 trees must stay alias-only. Net effect: the alias-only gate's ALLOWED_WITH_CODE allowlist is now EMPTY, and config_legacy.rb is checked by the general rule like every other legacy file. The pinned method inventory that narrowed its exemption goes with the exemption. default_options[:wait] stays a call-time read of Capybara.default_max_wait_time -- freezing it into Config#initialize reds all four #223 timing guards. Found while moving: the active? precedence rule had no test. Replacing the whole expression with a bare `enabled` kept all 529 unit tests green. Its truth table is now pinned through both the canonical method and the legacy forwarder. AVAILABLE_DRIVERS stays in config_legacy.rb (it is a bare constant assignment, so alias-shaped and never a gate blocker). Moving the storage to SnapDiff::Drivers was tried and reverted: eagerly it needs Utils, which requires Drivers back; lazily it breaks image_compare_test's published stubbing point, because Utils reads Drivers.available and would no longer see a stubbed legacy constant. Legacy public surface verified byte-identical: singleton_methods + parameters for Capybara::Screenshot and Capybara::Screenshot::Diff are unchanged. On the SnapDiff side, .compare gains an explicit signature (was a `(...)` pass-through) and .start loses its &block capture (now a bare yield) -- both a consequence of the bodies landing there, and both call-compatible. rake test:unit 530 runs / 0 failures; rake test 563 runs / 0 failures.
First commit of accepted ADR-005 (config storage inversion): before
SnapDiff::Configbecomes the single config store withconfig_legacy.rbflipped to delegating writers, the timing of default evaluation needs a guard net.mattr_accessorclass-body block defaults are evaluated once at require time with whatever ENV/Rails/pwd state exists at that moment — the #222 load-order guard pins the require graph, but nothing pinned this. This PR only adds the guard; no behavior changes.What it pins (current master behavior, per entry point)
12 subprocess probes — 3 per documented entry point (
capybara_screenshot_diff,capybara_screenshot_diff/minitest,snap_diff,capybara/screenshot/diff):fail_if_newENV["CI"]rootRails.root, else pwdRails.rootreassignment /chdirnot seendefault_options[:wait]Capybara.default_max_wait_timeMAPPINGsettingsSnapDiff.config.x== legacy mattr == expected default)Write-surface bidirectionality is already covered by
snap_diff_config_test.rband is not duplicated here.Gate evidence (the guard bites)
Temporarily hoisted the
fail_if_newENV read from class-body eval to a lazy read-time default (mattr_writer+ memoizingdef self.fail_if_new):Capybara::Screenshot::Diff.fail_if_new: expected true, got falseon the CI-set probes; expected-false failures on the snapshot probes, which mutateENV["CI"]post-require pre-first-read exactly to distinguish lazy from eager)Probes mutate inputs after require but before first read, so both memoized and non-memoized lazy variants are caught.
Test numbers
rake test:uniton master: 492 runs, 0 failuresrake test:unit504 runs / 0 failures; fullrake test537 runs, 1484 assertions, 0 failures, 0 errors, 6 skips (pre-existing); standardrb clean (143 files, no offenses)🤖 Generated with Claude Code
Summary by Sourcery
Pin configuration default evaluation timing and cross-surface values as a guard for the ADR-005 storage inversion.
Enhancements:
Tests: