test: driver contract tests formalizing the current driver seam - #204
Conversation
Extends the existing DriverContractTests shared module (test/support/driver_contract_tests.rb) with tests that pin the current de-facto driver interface shared by ChunkyPNGDriver and VipsDriver, run against both via the shared module: - method presence/arity for the ~15-method shared surface (load_images, add_black_box, find_difference_region, crop, from_file, save_image_to, resize_image_to, draw_rectangles, same_pixels?, same_dimension?, height_for, width_for, image_area_size, dimension, supports?) - load_images behavior (loads both images, dimensions comparable) - find_difference_region result shape (Difference exposing region/meta/comparison) - dimension handling (same_dimension? true/false, width/height/dimension/area_size agreement) - same-image fast paths (same_pixels? true/false) - option handling: tolerance, color_distance_limit, skip_area (both drivers support these identically; thresholds chosen with headroom on both drivers' measured values) - error behavior on missing base/new image files (ArgumentError) No production code changed. No driver method renames (dissent #4 in the v2 architecture design excludes those from this PR).
Reviewer's GuideThis PR expands the shared DriverContractTests module to formalize and pin the current driver interface/behavior across ChunkyPNGDriver and VipsDriver, by adding contract tests for method presence/signatures, image loading, difference result shape, dimension and pixel equality handling, options, and error cases, without touching production code. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 17 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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/support/driver_contract_tests.rb" line_range="72-80" />
<code_context>
+
+ # load_images -------------------------------------------------------------
+
+ test "[contract] #load_images loads both images from disk in (old, new) order" do
+ driver = make_comparison(:a, :a).driver
+
+ old_image, new_image = driver.load_images(TEST_IMAGES_DIR / "a.png", TEST_IMAGES_DIR / "b.png")
+
+ assert_not_nil old_image
+ assert_not_nil new_image
+ assert_equal driver.dimension(old_image), driver.dimension(new_image)
+ end
+
+ # find_difference_region result shape --------------------------------------
</code_context>
<issue_to_address>
**issue (testing):** The `#load_images` contract test claims to verify `(old, new)` ordering but only asserts that both returned images are non-nil and have equal dimensions. Because fixtures `a.png` and `b.png` share dimensions, an implementation that returns the images in reverse order passes this test silently.
**Triggers:** When a driver regresses by swapping the old and new images returned from `load_images`.
**Suggested fix:** Assert the returned image contents against the corresponding fixture, not just their dimensions.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: test/support/driver_contract_tests.rb:80
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| test "[contract] #load_images loads both images from disk in (old, new) order" do | ||
| driver = make_comparison(:a, :a).driver | ||
|
|
||
| old_image, new_image = driver.load_images(TEST_IMAGES_DIR / "a.png", TEST_IMAGES_DIR / "b.png") | ||
|
|
||
| assert_not_nil old_image | ||
| assert_not_nil new_image | ||
| assert_equal driver.dimension(old_image), driver.dimension(new_image) | ||
| end |
There was a problem hiding this comment.
issue (testing): The #load_images contract test claims to verify (old, new) ordering but only asserts that both returned images are non-nil and have equal dimensions. Because fixtures a.png and b.png share dimensions, an implementation that returns the images in reverse order passes this test silently.
Triggers: When a driver regresses by swapping the old and new images returned from load_images.
Suggested fix: Assert the returned image contents against the corresponding fixture, not just their dimensions.
1 participant
Why
Step 1 of the v2 PR sequence (PR 10.0). Before any driver file moves or the
BaseDriver→Drivermixin refactor land, this formalizes the driver seamv2 will document as
SnapDiff::Driverwith a regression net: shared contracttests that assert the current de-facto driver interface against both
ChunkyPNGDriverandVipsDriveridentically. Advances issue #166 withouttouching the gate.
Per dissent #4 in the v2 architecture design, driver method renames are
out of scope here — only class/module renames are approved for v2, and the
~14 existing method names (
load_images,find_difference_region, etc.)stay exactly as they are. This PR pins those current names/signatures so a
later inheritance→mixin refactor (which changes method resolution order) has
something to catch an accidental override with.
What
Extends the existing
DriverContractTestsshared module(
test/support/driver_contract_tests.rb, already included by bothChunkyPNGDriverTestandVipsDriverTest) rather than duplicating it, with:(
load_images,add_black_box,find_difference_region,crop,from_file,save_image_to,resize_image_to,draw_rectangles,same_pixels?,same_dimension?,height_for,width_for,image_area_size,dimension,supports?) and arity for thecomparison-taking methods and
load_images.load_imagesbehavior — loads both images from disk.find_difference_regionresult shape — aDifferenceexposingregion,meta(Hash), andcomparison.same_dimension?true/false, andwidth_for/height_for/dimension/image_area_sizeagree with eachother.
same_pixels?true/false.tolerance,color_distance_limit, andskip_area,which both drivers support identically. Thresholds are chosen with
headroom on each driver's actual measured values (verified numerically
before writing the assertions, not guessed).
ArgumentErroron a missing base or new image file.No production code changed. No driver method renames.
Gate evidence (mutation testing, reverted before commit)
Broke two real driver behaviors one at a time and confirmed exactly the
matching contract assertions (plus expected pre-existing collateral tests)
went red, then reverted:
BaseDriver#same_dimension?forced to always returntrue→ the new[contract] #same_dimension? returns false when images differ in dimensionstest failed in all 6 execution contexts (both driver testclasses + ChunkyPNG's nested subclasses), plus 2 pre-existing
dimension-related tests as expected collateral. 8 failures, 2 errors.
Difference#tolerable?forced to always returnfalse→ the new[contract] tolerance option treats small differences as equaltestfailed in all 6 execution contexts, plus 4 pre-existing tolerance tests
as expected collateral. 11 failures.
Both mutations reverted; suite back to green before commit.
Discovered driver divergences (reported, not fixed — per task scope)
VipsDriver#resize_image_toappears to compute the wrong scalefactor.
resize_image_to(image, new_width, new_height)is documentedby its ChunkyPNG counterpart (
image.resample_bilinear(new_width, new_height)) to resize to the literal target dimensions. Verifiedempirically:
ChunkyPNGDriver#resize_image_to(image, 40, 30)on an80x80 source image correctly yields
[40, 30];VipsDriveryields[107, 107]— nowhere near the requested target. Root cause lookslike
image.resize(new_width.to_f / new_height)computing the sourceaspect ratio instead of a width-based scale factor (should likely be
new_width.to_f / width_for(image)). Not covered by any existing test.Recommend a follow-up bug fix + regression test — deliberately not
touched here per this task's "STOP and report, don't fix" scope.
add_black_boxis not actually a black-box operation on ChunkyPNG.ChunkyPNGDriver#add_black_boxis a no-op that returns the imageunchanged;
VipsDriver#add_black_boxgenuinely draws a black rectangle.skip_areastill behaves correctly end-to-end on both drivers (verifiedand now covered by the new shared
skip_areacontract tests) becauseChunkyPNG implements the skip semantics separately, via coordinate
checks in its own difference finder. Not a bug, but a real divergence in
what the method itself does — worth knowing before the v2 mixin
refactor treats
add_black_boxas one contract.difference_levelis not on the same interface surface.VipsDriver#difference_level(diff_mask, old_img, region = nil)is apublic driver method; ChunkyPNG's equivalent lives as a private method
on its internal
DifferenceRegionFinder, not onChunkyPNGDriveritself (
chunky_driver.respond_to?(:difference_level)isfalse).Excluded from the shared contract for that reason.
unless defined?(Vips) ... return endguardin
test/unit/drivers/vips_driver_test.rb(already flagged intest/support/driver_coverage.rb's own comments as a known issue) meansthat in a vips-less environment none of these new contract tests — or
any test in that file — would even be defined, let alone show up as
skipped. Left untouched here (restructuring it is a separate, riskier
change outside this PR's additive scope); every new test added in this
PR uses ordinary
test "..." do ... endblocks with no top-levelreturns, so nothing here adds to that anti-pattern.
Test numbers
267 runs, 806 assertions, 0 failures, 0 errors, 0 skips.369 runs, 1076 assertions, 0 failures, 0 errors, 0 skips(bothvipsandchunky_pngdrivers detected in thisenvironment). The jump from +22 new test methods to +102 runs is
because
ChunkyPNGDriverTesthas 4 pre-existing nested subclasses(
QuickEqualTest,DifferentTest,ColorDistanceTest,HelpersTest)that inherit the shared module's tests too — same multiplication the
pre-existing 5 contract tests already had.
standardrb(full repo): 97 files inspected, no offenses.Ruby:
mise x ruby@4.0.6.🤖 Generated with Claude Code
Summary by Sourcery
Formalize the existing driver seam with shared regression tests for both supported driver implementations.
Enhancements:
Tests:
Review note: under a vips-less runner the 17 new contract tests are silently not defined (the host file gates on
defined?(Vips)with a top-level return — pre-existing pattern); CI-level protection for that case lives intest/support/driver_coverage.rb.