refact: simplify test framework integrations #141
Conversation
WalkthroughThis update introduces a significant refactor of the image comparison and screenshot diffing system. It adds new classes for modularizing image loading, preprocessing, and difference detection, and implements a strategy-based approach for screenshot capture. Error handling is improved with filtered backtraces, and test coverage is expanded with new and updated tests. The cleanup process for test artifacts is now more granular, focusing on individual snapshots. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant SnapManager
participant ScreenshotCoordinator
participant CaptureStrategy
participant Screenshoter
participant ComparisonLoader
participant ImagePreprocessor
participant DifferenceFinder
Test->>SnapManager: create snapshot
Test->>ScreenshotCoordinator: capture(snap, capture_options, comparison_options)
ScreenshotCoordinator->>CaptureStrategy: select strategy (standard/stable)
CaptureStrategy->>Screenshoter: take_comparison_screenshot(snap)
Screenshoter->>ComparisonLoader: load images (base, new)
ComparisonLoader->>ImagePreprocessor: preprocess images (mask/filter)
ImagePreprocessor-->>ComparisonLoader: processed images
ComparisonLoader-->>Screenshoter: Comparison object
Screenshoter-->>CaptureStrategy: Comparison object
CaptureStrategy-->>ScreenshotCoordinator: Comparison object
ScreenshotCoordinator-->>Test: Comparison object
Test->>DifferenceFinder: find_difference(Comparison)
DifferenceFinder-->>Test: Difference object
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and simplifies the test framework integrations for screenshot diff functionality by streamlining error handling, assertion mechanisms, and image comparison procedures. Key changes include:
- Replacing StandardError with a custom ErrorWithFilteredBacktrace for improved backtrace filtering.
- Renaming and unifying methods to use “assertion” terminology instead of “job” for consistency.
- Enhancing image comparison with early file equality checks and refined difference finding logic.
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb | Introduces a custom error class that filters backtraces. |
| lib/capybara_screenshot_diff/backtrace_filter.rb | Implements logic for filtering backtrace lines. |
| lib/capybara_screenshot_diff.rb | Modifies the custom error inheritance for improved error reporting. |
| lib/capybara/screenshot/diff/test_methods.rb | Refactors test methods to use assertions with updated scheduling and error handling. |
| lib/capybara/screenshot/diff/standard_capture_strategy.rb | Provides a default capture strategy using the generic screenshoter. |
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Adjusts error raising for unstable images using the new initializer format. |
| lib/capybara/screenshot/diff/stable_capture_strategy.rb | Implements a capture strategy waiting for page content stability. |
| lib/capybara/screenshot/diff/screenshoter.rb | Adopts the new error raising pattern and updates backtrace handling. |
| lib/capybara/screenshot/diff/screenshot_matcher.rb | Switches to building screenshot assertions with refined stack frame handling. |
| lib/capybara/screenshot/diff/screenshot_coordinator.rb | Introduces a unified API for coordinating screenshot capture strategies. |
| lib/capybara/screenshot/diff/reporters/default.rb | Adjusts how diff masks and skip areas are handled during annotation. |
| lib/capybara/screenshot/diff/image_preprocessor.rb | Adds image preprocessing capabilities, such as applying skip areas and median filters. |
| lib/capybara/screenshot/diff/image_compare.rb | Enhances image comparison by adding early file equality checks and refactors difference creation. |
| lib/capybara/screenshot/diff/drivers/base_driver.rb | Adds a supports? method to check for driver feature support. |
| lib/capybara/screenshot/diff/difference_finder.rb | Refines the logic for determining image differences. |
| lib/capybara/screenshot/diff/difference.rb | Updates the Difference class for improved clarity and backward compatibility. |
| lib/capybara/screenshot/diff/comparison_loader.rb | Introduces a loader to assemble comparisons from raw images. |
| lib/capybara/screenshot/diff/comparison.rb | Adds a skip_area accessor to the comparison object. |
| lib/capybara/screenshot/diff/capture_strategy.rb | Provides an abstract base class for customizing capture strategies. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
lib/capybara/screenshot/diff/difference.rb (1)
96-103: 🛠️ Refactor suggestion
to_hblows up whenmetaisnil
merge!(meta)raisesTypeErrorifmetaisnil.
Guard with a default:- {area_size: region_area_size, region: coordinates}.merge!(meta) + {area_size: region_area_size, region: coordinates}.merge!(meta || {})
🧹 Nitpick comments (23)
.gitignore (1)
4-4: Clarify ignore pattern for.windsurf
To explicitly ignore the directory and all its contents, append a trailing slash:-/.windsurf +/.windsurf/test/capybara_screenshot_diff/snap_manager_test.rb (2)
35-43: Incorrect test method name.The test name refers to
#screenshots_dirbut it's actually testing the#snapshotsmethod, as shown in the assertions.- test "#screenshots_dir returns all created snapshots" do + test "#snapshots returns all created snapshots" do
45-54: Incorrect test method name.Similar to the previous test, this test name should refer to
#snapshotsinstead of#screenshots_dir.- test "#screenshots_dir ignores attempts" do + test "#snapshots includes snaps with attempts" dolib/capybara/screenshot/diff/capture_strategy.rb (1)
1-29: Well-designed abstract base class with a minor style issue.The abstract
CaptureStrategyclass provides a solid foundation for the strategy pattern implementation with clear documentation, proper encapsulation, and interface definition.However, there's a linting error on line 13:
- @capture_options = capture_options + @capture_options = capture_optionsThe unnecessary spacing around the assignment operator should be removed to pass the StandardRB linting rules.
🧰 Tools
🪛 GitHub Actions: Lint
[error] 13-13: StandardRB: Unnecessary spacing detected and operator '=' should be surrounded by a single space. These offenses are autocorrectable.
lib/capybara_screenshot_diff/snap_manager.rb (1)
70-70: Consider addressing the TODO commentThe file contains a TODO comment (line 46) suggesting the
cleanup!method should be renamed todelete!. Since you're already refactoring this class, consider addressing this TODO by either:
- Renaming the method
- Adding a
delete!method as an alias- Removing the TODO if it's no longer relevant
test/capybara/screenshot/diff/comparison_loader_test.rb (1)
13-33: Well-structured test for ComparisonLoaderThe test effectively covers the happy path for the
ComparisonLoaderclass, verifying that it correctly loads images and returns a properly configured Comparison object.Consider adding tests for edge cases:
- What happens with invalid paths?
- How does the loader behave when image loading fails?
- What happens when the driver returns unexpected values?
lib/capybara/screenshot/diff/difference_finder.rb (1)
41-66: Reduce complexity in the call methodThe
callmethod has slightly high cyclomatic complexity (8/7 according to RuboCop). While the current implementation is logical and follows a clear layered approach, it could be more maintainable if refactored into smaller methods.Consider extracting each comparison layer into a separate method:
- def call(comparison, quick_mode: true) - # Process the comparison and return result - - # Handle dimension differences - unless driver.same_dimension?(comparison) - result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path, {different_dimensions: true}) - return quick_mode ? [false, result] : result - end - - # Handle identical pixels - if driver.same_pixels?(comparison) - result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path) - return quick_mode ? [true, result] : result - end - - # Handle early return for non-tolerable options - if quick_mode && without_tolerable_options? - return [false, nil] - end - - # Process difference region - region = driver.find_difference_region(comparison) - - # Only create a proper difference object if we've completed the comparison - quick_mode ? [!region.different?, region] : region - end + def call(comparison, quick_mode: true) + # Process the comparison and return result + + # Check dimensions first (fastest check) + result = check_dimensions(comparison, quick_mode) + return result if result + + # Check pixel equality next + result = check_pixel_equality(comparison, quick_mode) + return result if result + + # Early return for non-tolerable options in quick mode + if quick_mode && without_tolerable_options? + return [false, nil] + end + + # Finally, detailed region analysis (most expensive) + analyze_difference_region(comparison, quick_mode) + end + + private + + def check_dimensions(comparison, quick_mode) + unless driver.same_dimension?(comparison) + result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path, {different_dimensions: true}) + return quick_mode ? [false, result] : result + end + nil + end + + def check_pixel_equality(comparison, quick_mode) + if driver.same_pixels?(comparison) + result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path) + return quick_mode ? [true, result] : result + end + nil + end + + def analyze_difference_region(comparison, quick_mode) + region = driver.find_difference_region(comparison) + quick_mode ? [!region.different?, region] : region + end🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 41-66: Cyclomatic complexity for
callis too high. [8/7](Metrics/CyclomaticComplexity)
test/capybara/screenshot/diff/image_compare_refactor_test.rb (1)
32-42: Thorough test for dimension changesThis test properly verifies both the dimension change detection and the reporter type, ensuring the refactored code correctly handles images with different dimensions.
Consider adding tests for additional edge cases if not covered elsewhere:
- Handling of missing files
- Error scenarios
- Behavior with various comparison option combinations
lib/capybara/screenshot/diff/image_preprocessor.rb (1)
34-45: Missingpathleads to vague warning messages.
#callpassesnilforpath, so if the median filter is unsupported the warning prints
“Median filter has been skipped for because …”. Consider forwarding a more helpful
identifier (e.g. index or synthetic name) or guarding againstnil.- def call(images) - images.map { |image| process_image(image, nil) } + def call(images) + images.each_with_index.map do |image, idx| + name = "image##{idx}" + process_image(image, name) + end endtest/capybara/screenshot/diff/image_preprocessor_test.rb (1)
56-63: Test does not exercise path propagation.Because
ImagePreprocessor#callsuppliesnilforpath, theimage_pathoption you set
here is never used. The warning assertion therefore passes even if the path handling is
broken. Consider switching the test toprocess_comparison(which does forward a path)
or altering the production code as suggested to include synthetic names.test/support/stub_test_methods.rb (2)
25-30: Potential name confusion – parameter order vs. YARD docs.
make_comparison(fixture_base_image, fixture_new_image = nil, …)receives base then
new, but in everyday language “new” comes first (“actual vs. expected”). Consider
renaming the parameters or adding a short comment clarifying the order to avoid misuse.
61-67: Non-deterministic snapshot names may hinder test reproducibility.
Time.now.nseccan collide in very fast test runs and makes snapshot folders harder to
inspect. Using an incrementing counter or UUID that’s stub-able in tests would improve
repeatability.lib/capybara/screenshot/diff/screenshot_matcher.rb (2)
57-61: RuboCop hint – trailing comma & readability.A tiny style nit: add a trailing comma to the last item of the multiline array to satisfy
Style/TrailingCommaInArrayLiteraland avoid noisy CI.- ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options) + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options),🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 60-60: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
85-91: Dead code:#build_screenshoter_foris now unused.Since capture is fully delegated to
ScreenshotCoordinator, this helper can be removed to
simplify the class and reduce ABC complexity flagged by RuboCop.test/capybara/screenshot/diff/test_methods_test.rb (1)
4-4: Duplicaterequireappears unnecessary
capybara_screenshot_diff/screenshot_assertionalready loads the main gem, so the additional plainrequire "capybara_screenshot_diff"is redundant and adds ~50 ms of start-up time in CI.
Consider removing it unless you measured a real need (e.g. lazy–load issues in Ruby < 2.4).-require "capybara_screenshot_diff"lib/capybara/screenshot/diff/difference.rb (1)
8-20: Duplicate description & long headerLines 8-20 repeat the “Represents a difference between two images” sentence twice and provide a very long header that is hard to maintain.
A concise docstring plus YARD tags is usually enough:-# Represents a difference between two images -# -# This value object encapsulates ... -# output of the comparison process, containing all data needed for reporting. -# Represents a difference between two images +# A value object describing how two screenshots differ (or do not differ). +# It is produced by image-comparison code and consumed by reporters.test/capybara/screenshot/diff/difference_finder_test.rb (2)
24-35: Dead helper –create_difference_factoryis never usedThe lambda factory is defined but never referenced in the spec suite, which:
- Increases cognitive load for future maintainers.
- Hints at an unfinished refactor.
Unless you intend to inject a custom factory in upcoming tests, please remove the method to keep the test lean.
100-115: Coupling tests to internal call-counts makes them brittleAssertions on
@driver.dimension_check_calls.sizeetc. verify implementation rather than behaviour.
Minor internal optimisations (e.g. memoising pixel equality) will break the test while leaving user-visible semantics intact.Prefer to assert only on the public contract (returned
Differenceobject, truthiness, etc.).
If you really need to ensure an optimisation, move such expectations to a dedicated lower-level unit test for the driver.test/capybara/screenshot/diff/test_doubles.rb (2)
92-96: Overridingclasscan break introspection & mocksReturning
Objectfrom#classavoids recursion but also removes the ability for callers (including Ruby core helpers, test frameworks, and meta-programming utilities) to accurately introspect the double’s real class. This can cause subtle failures, e.g.obj.class.name,pp obj, or RSpec’sbe_amatcher.If recursion on
#classis the only concern, consider a safer alternative:- def class - Object - end + # Pretend to be the real driver class without triggering recursion + def class + Drivers::VipsDriver if @is_vips_driver + rescue NameError + super + endor simply remove the override and stub
is_a?/supports?where needed.
88-90:supports?accepts splatted positional args – consider mirroring the real signatureUsing
def supports?(...)swallows any arguments but also hides arity problems if upstream code changes the method contract. Prefer forwarding keywords/positionals explicitly or adding*args, **kwto surface unexpected parameters during tests.lib/capybara/screenshot/diff/image_compare.rb (3)
37-43:ensure_files_exist!executed twice per comparison – redundant exception
initializealready callsensure_files_exist!; calling it again inquick_equal?adds overhead and raises an exception twice on missing files.
Consider removing the second invocation and rely on the earlier check, or makeensure_files_exist!idempotent but silent, returningfalsewhen files are absent instead of raising, soquick_equal?can simply returnfalse.
140-143:images_exist?duplicatesensure_files_exist!logicThe separate checks (
ensure_files_exist!,images_exist?) risk drifting out of sync.
Prefer one single responsibility – either:
- keep
ensure_files_exist!as the authoritative check and call it where needed, or- replace both with
images_exist?returning a boolean and raise only at the call-site that needs the exception.This also makes the code easier to unit-test.
146-152: Rescuing all exceptions hides IO errors
files_identical?rescues barerescue, which catchesNoMemoryError,SystemExit, etc. Restrict the rescue toStandardError:- rescue + rescue StandardErrorFailing fast on non-standard errors avoids masking fatal conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (39)
.gitignore(1 hunks)lib/capybara/screenshot/diff/capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/comparison.rb(1 hunks)lib/capybara/screenshot/diff/comparison_loader.rb(1 hunks)lib/capybara/screenshot/diff/difference.rb(2 hunks)lib/capybara/screenshot/diff/difference_finder.rb(1 hunks)lib/capybara/screenshot/diff/drivers/base_driver.rb(1 hunks)lib/capybara/screenshot/diff/drivers/vips_driver.rb(1 hunks)lib/capybara/screenshot/diff/image_compare.rb(4 hunks)lib/capybara/screenshot/diff/image_preprocessor.rb(1 hunks)lib/capybara/screenshot/diff/reporters/default.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_coordinator.rb(1 hunks)lib/capybara/screenshot/diff/screenshot_matcher.rb(3 hunks)lib/capybara/screenshot/diff/screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/stable_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/stable_screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/standard_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/test_methods.rb(3 hunks)lib/capybara_screenshot_diff.rb(1 hunks)lib/capybara_screenshot_diff/backtrace_filter.rb(1 hunks)lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb(1 hunks)lib/capybara_screenshot_diff/minitest.rb(1 hunks)lib/capybara_screenshot_diff/rspec.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(1 hunks)lib/capybara_screenshot_diff/snap.rb(1 hunks)lib/capybara_screenshot_diff/snap_manager.rb(3 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(1 hunks)test/capybara/screenshot/diff/difference_finder_test.rb(1 hunks)test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_refactor_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_test.rb(2 hunks)test/capybara/screenshot/diff/image_preprocessor_test.rb(1 hunks)test/capybara/screenshot/diff/test_doubles.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(4 hunks)test/capybara/screenshot/diff_test.rb(2 hunks)test/capybara_screenshot_diff/snap_manager_test.rb(1 hunks)test/support/screenshoter_stub.rb(1 hunks)test/support/stub_test_methods.rb(2 hunks)test/test_helper.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (14)
lib/capybara/screenshot/diff/stable_screenshoter.rb (1)
lib/capybara_screenshot_diff/attempts_reporter.rb (1)
generate(13-19)
test/test_helper.rb (1)
lib/capybara_screenshot_diff/snap_manager.rb (2)
cleanup!(47-52)cleanup!(54-56)
lib/capybara/screenshot/diff/drivers/base_driver.rb (1)
test/capybara/screenshot/diff/test_doubles.rb (1)
supports?(88-90)
lib/capybara/screenshot/diff/comparison.rb (1)
lib/capybara/screenshot/diff/difference.rb (2)
skip_area(72-74)options(64-66)
test/capybara_screenshot_diff/snap_manager_test.rb (3)
lib/capybara_screenshot_diff/snap_manager.rb (3)
snapshot(17-21)snapshot(23-25)provision_snap_with(36-40)test/test_helper.rb (1)
fixture_image_path_from(44-46)lib/capybara_screenshot_diff/snap.rb (1)
next_attempt_path!(35-40)
lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
lib/capybara_screenshot_diff.rb (1)
active?(42-44)
lib/capybara/screenshot/diff/stable_capture_strategy.rb (2)
lib/capybara/screenshot/diff/capture_strategy.rb (3)
initialize(11-26)initialize(12-15)take_comparison_screenshot(19-21)lib/capybara/screenshot/diff/standard_capture_strategy.rb (3)
initialize(11-21)initialize(12-16)take_comparison_screenshot(18-20)
lib/capybara_screenshot_diff/snap.rb (1)
lib/capybara_screenshot_diff/snap_manager.rb (1)
abs_path_for(27-29)
lib/capybara/screenshot/diff/standard_capture_strategy.rb (3)
lib/capybara/screenshot/diff/capture_strategy.rb (3)
initialize(11-26)initialize(12-15)take_comparison_screenshot(19-21)lib/capybara/screenshot/diff/stable_capture_strategy.rb (3)
initialize(11-20)initialize(12-15)take_comparison_screenshot(17-19)lib/capybara/screenshot/diff/reporters/default.rb (1)
driver(112-114)
test/capybara/screenshot/diff/image_compare_test.rb (4)
test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb (1)
make_comparison(148-151)test/support/stub_test_methods.rb (1)
make_comparison(25-29)lib/capybara/screenshot/diff/image_compare.rb (2)
quick_equal?(46-57)different?(66-68)lib/capybara/screenshot/diff/difference.rb (1)
different?(52-54)
lib/capybara/screenshot/diff/screenshot_coordinator.rb (3)
lib/capybara/screenshot/diff/capture_strategy.rb (1)
take_comparison_screenshot(19-21)lib/capybara/screenshot/diff/stable_capture_strategy.rb (1)
take_comparison_screenshot(17-19)lib/capybara/screenshot/diff/standard_capture_strategy.rb (1)
take_comparison_screenshot(18-20)
test/capybara/screenshot/diff/test_methods_test.rb (5)
lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
assert_image_not_changed(59-72)test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb (1)
make_comparison(148-151)test/support/stub_test_methods.rb (2)
make_comparison(25-29)create_snapshot_for(61-67)lib/capybara/screenshot/diff/test_methods.rb (1)
screenshot(101-119)lib/capybara_screenshot_diff/dsl.rb (1)
screenshot(13-16)
test/support/stub_test_methods.rb (3)
test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb (1)
make_comparison(148-151)lib/capybara_screenshot_diff/snap_manager.rb (3)
provision_snap_with(36-40)snapshot(17-21)snapshot(23-25)test/test_helper.rb (1)
fixture_image_path_from(44-46)
lib/capybara/screenshot/diff/screenshot_matcher.rb (3)
lib/capybara/screenshot/diff/test_methods.rb (1)
build_screenshot_assertion(137-139)lib/capybara/screenshot/diff/screenshot_coordinator.rb (1)
capture(23-27)lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
from(17-25)
🪛 GitHub Actions: Lint
lib/capybara/screenshot/diff/capture_strategy.rb
[error] 13-13: StandardRB: Unnecessary spacing detected and operator '=' should be surrounded by a single space. These offenses are autocorrectable.
🪛 RuboCop (1.75.5)
lib/capybara/screenshot/diff/difference_finder.rb
[convention] 41-66: Cyclomatic complexity for call is too high. [8/7]
(Metrics/CyclomaticComplexity)
lib/capybara/screenshot/diff/screenshot_matcher.rb
[convention] 25-62: Assignment Branch Condition size for build_screenshot_assertion is too high. [<6, 40, 4> 40.64/23]
(Metrics/AbcSize)
[convention] 60-60: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🔇 Additional comments (43)
.gitignore (1)
1-1: Add editor backup file ignore pattern
The*~entry cleanly filters out common editor or shell backup files, keeping the repo tidy.lib/capybara/screenshot/diff/drivers/base_driver.rb (1)
33-36: Good addition of thesupports?method.This method enables a standardized way for drivers to declare support for optional features by checking if the driver responds to a specific method name. This fits well with the overall refactoring strategy of modularizing the screenshot diffing system.
Looking at the test doubles implementation, this will allow conditional application of features based on driver capabilities.
lib/capybara/screenshot/diff/comparison.rb (1)
5-7: Well-designed accessor forskip_area.This method provides a clean, direct way to access the
:skip_areaoption from the comparison options hash. It aligns with the refactoring approach where image preprocessing responsibilities have been delegated to a newImagePreprocessorclass.This change improves encapsulation and ensures consistent access to skip area information across the diffing and reporting workflow.
test/test_helper.rb (1)
34-35: Good replacement of manual cleanup with managed cleanup.Replacing direct file removal with
CapybaraScreenshotDiff::SnapManager.cleanup!is a better approach as it:
- Provides more granular control over test artifact cleanup
- Aligns with the new snapshot lifecycle management
- Ensures consistency with the rest of the codebase
It's helpful that you've left the old approach commented out for reference.
lib/capybara/screenshot/diff/drivers/vips_driver.rb (1)
43-43: Improved indentation for better readability.This whitespace adjustment improves the formatting of the warning message, making it more consistently indented and easier to read. While a minor change, it contributes to the overall code quality improvements in the image diffing subsystem.
lib/capybara/screenshot/diff/stable_screenshoter.rb (1)
103-103: Good improvement to error reporting!You're now explicitly passing the
callerstack to theUnstableImageexception, which will provide better debugging context when unstable image errors occur. This aligns well with the other error handling improvements in this PR.lib/capybara/screenshot/diff/screenshoter.rb (1)
89-89: Improved exception handling with proper stack traceThe explicit passing of
callerto theExpectationNotMetexception will provide better context in error messages, making debugging easier when images fail to load within the timeout period.lib/capybara_screenshot_diff.rb (2)
12-13: Good addition of filtered backtrace supportAdding this requirement for the new error class with filtered backtrace capability is a good step toward improving the debugging experience.
15-15: Excellent error hierarchy improvementChanging the base error class to inherit from
ErrorWithFilteredBacktraceinstead ofStandardErrorwill ensure all derived exceptions benefit from the backtrace filtering, providing cleaner and more relevant error traces.lib/capybara_screenshot_diff/minitest.rb (1)
42-42: Good preservation of original backtraceThis change ensures that the original exception's backtrace is preserved when wrapping the
ExpectationNotMeterror into aMinitest::Assertion. This maintains important debugging context that would otherwise be lost.test/support/screenshoter_stub.rb (1)
14-14: Enhanced filename normalization for consistent handlingThis new pattern removes numeric suffixes (like "_123") before file extensions, complementing the existing normalizations for attempt patterns and numeric prefixes. This ensures consistent filename handling across the refactored screenshot diffing system.
test/capybara/screenshot/diff_test.rb (2)
146-146: Updated mock expectations to match new assertion flowThe addition of a second expectation for
different?is correct and necessary to accommodate the updated screenshot assertion workflow which now checks differences twice during the error handling process.
179-180: Improved mock expectations with explicit purpose commentsGood improvement by adding explicit comments to clarify the purpose of each
different?expectation - one for backtrace finding and one for message finding. This makes the test intentions much clearer.lib/capybara_screenshot_diff/rspec.rb (1)
30-30: Enhanced error traceability by preserving the original backtraceThis change properly preserves the original backtrace when re-raising exceptions, making debugging significantly easier by maintaining the context of where the error originally occurred.
lib/capybara_screenshot_diff/snap.rb (1)
52-52: More precise attempt file pattern matchingThe pattern has been refined to match only files with exactly two-digit numeric suffixes, aligning with the standardized formatting in
next_attempt_path!(line 37). This ensures consistent handling of attempt files throughout the snapshot lifecycle.lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb (1)
1-15: Well-designed error class with filtered backtrace.The implementation of this error class is clean and follows good Ruby practices. It filters backtraces to make error messages more readable by removing irrelevant framework lines, which will improve debugging experience.
The private documentation tags are appropriate for an internal utility class.
lib/capybara/screenshot/diff/reporters/default.rb (3)
48-48: Improved direct property access.Changing from meta hash access to direct method access is a good refactoring that makes the code more explicit and easier to maintain.
53-53: Property correctly moved to appropriate class.Moving the skip_area property to the Comparison class makes the responsibility boundaries clearer, as skip areas are a property of the comparison rather than the difference itself.
91-91: Consistent use of direct accessor.This change consistently applies the same pattern of using direct accessors instead of meta hash, which improves code readability and maintainability.
lib/capybara/screenshot/diff/stable_capture_strategy.rb (1)
1-23: Well-implemented strategy pattern for screenshot capture.This class follows the strategy pattern correctly by inheriting from the abstract base class and properly implementing the required method. The delegation to StableScreenshoter keeps this class focused and maintains single responsibility.
The documentation explains the purpose of this strategy clearly - waiting for page content to stabilize by taking multiple screenshots and comparing them.
test/capybara_screenshot_diff/snap_manager_test.rb (1)
56-62: Good test for file extension handling.This test properly verifies that the snapshot method correctly handles file extensions for different snapshot versions while preserving the original name.
lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
96-104: Improved error reporting with contextual backtraces.Good enhancement to error handling. The changes now:
- Add an early return condition that matches the check used in
verify_screenshots!- Retrieve failed assertions before verification to access their contextual information
- Pass the caller information from the first failed assertion to the exception, which will provide better context about where the screenshot assertion failure occurred
This will make debugging screenshot test failures much easier by showing the actual location in the test code that triggered the failure.
lib/capybara/screenshot/diff/standard_capture_strategy.rb (1)
1-24: Well-structured implementation of the Strategy pattern.This implementation of
StandardCaptureStrategyfollows good OO design practices:
- Clear class documentation explaining its purpose as the default capture strategy
- Proper inheritance from the abstract
CaptureStrategyclass- Effective use of delegation to the
Screenshoterclass for the actual screenshot captureThe strategy pattern allows for swappable screenshot capture implementations without changing client code, which supports the overall refactoring goal of modularizing the screenshot diffing system.
lib/capybara_screenshot_diff/backtrace_filter.rb (1)
1-20: Good utility class for improving error backtraces.This
BacktraceFilterclass is a well-designed utility that improves debugging by:
- Filtering out noise from error backtraces (framework and library internals)
- Focusing the backtrace on user code where the actual issue is likely to be
- Supporting both the gem's own code and common Ruby frameworks
The implementation is clean, well-documented, and follows the single responsibility principle. This will make error messages much more useful when debugging screenshot test failures.
test/capybara/screenshot/diff/image_compare_test.rb (4)
19-19: Improved test setup consistencyGood refactoring to use the
make_comparisonhelper method consistently throughout the test file instead of direct instantiation ofImageCompare. This improves code maintainability and keeps the test setup logic centralized.
63-65: Enhanced test coverage for chunky_png driverThe updated test now properly verifies both
quick_equal?anddifferent?methods, which aligns with the layered optimization approach in theImageCompareclass. This provides more comprehensive test coverage for the driver's behavior.
69-71: Improved test coverage for dimensions optionGood enhancement to verify both
quick_equal?anddifferent?methods when testing the dimensions option. This ensures that both the fast path and the full comparison path are tested.
83-84:Details
✅ Verification successful
Verify if test behavior is still correct
The assertion
assert comparison.quick_equal?is now added after instantiating a comparison with an invalid driver. Since we expect an error during instantiation, this assertion might never be reached.
🏁 Script executed:
#!/bin/bash # Check the implementation of AVAILABLE_DRIVERS constant and how it's used rg -A 10 "AVAILABLE_DRIVERS" lib/capybara/screenshot/diff/ # Look for any error handling in the instantiation process rg -A 5 -B 5 "raise" lib/capybara/screenshot/diff/image_compare.rbLength of output: 1872
:auto driver falls back to a valid adapter, so the test assertion is reachable
The:autodriver is reassigned toAVAILABLE_DRIVERS.firstinutils.rb, meaning instantiation will not raise an error. Therefore, theassert comparison.quick_equal?check remains correct and will be executed as intended.lib/capybara/screenshot/diff/comparison_loader.rb (1)
1-34: Well-structured class with clear single responsibilityGood addition of the
ComparisonLoaderclass that follows the Single Responsibility Principle by focusing solely on loading images and creating aComparisonobject. The code is well-documented with comments explaining its purpose and behavior.The implementation correctly:
- Takes a driver in the constructor
- Provides a
callmethod that loads images and returns a configuredComparisonobject- Contains clear documentation of parameters and return values
This is a good example of modularizing functionality for better maintainability.
test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb (1)
149-150: Improved test helper implementationGood refactoring of the
make_comparisonhelper method to explicitly create a snapshot usingcreate_snapshot_forand then instantiate anImageCompareobject. This improves the clarity of the test code and reduces dependencies on the superclass method.The change aligns with the broader refactoring of centralizing and streamlining image comparison responsibilities.
lib/capybara_screenshot_diff/snap_manager.rb (3)
14-14: Good addition of snapshot trackingAdding the
@snapshotsset to track all created snapshots is a good enhancement that enables more granular management of test artifacts.
18-20: Improved snapshot lifecycle managementGood enhancement to track each created
Snapin the@snapshotsset. This enables better snapshot lifecycle management and supports the refactored cleanup process.
48-51: More granular cleanup processExcellent improvement to the
cleanup!method that now performs granular cleanup by iterating over tracked snapshots instead of recursively removing the entire root directory. This change:
- Allows for more targeted cleanup
- Reduces the risk of accidentally removing unrelated files
- Supports better debugging by being more explicit about what's being cleaned up
This refinement aligns with the overall improvements in snapshot management throughout the codebase.
lib/capybara/screenshot/diff/screenshot_coordinator.rb (3)
11-27: Well-designed coordination module with clean strategy selectionThe
ScreenshotCoordinatormodule provides a clear public API and effectively implements the Strategy pattern for screenshot capture. The implementation follows good Ruby practices with module functions and clear method signatures.
30-33: Clean strategy factory implementationThe strategy selection logic is simple and effective, choosing between stable and standard capture strategies based on the presence of a stability time limit option.
37-44: Good handling of different input typesThe
ensure_snapmethod properly handles both Snap objects and string names, providing a consistent interface for the coordinator regardless of input format.lib/capybara/screenshot/diff/difference_finder.rb (1)
9-22: Clear documentation of the layered comparison approachThe documentation clearly explains the responsibility of the class and its role in the comparison process. This is excellent for maintaining the code and helping future developers understand the design decisions.
test/capybara/screenshot/diff/image_compare_refactor_test.rb (1)
14-30: Good test coverage of core comparison predicatesThe tests effectively cover the behavior of the refactored
ImageCompareclass for the primary use cases of comparing identical and different images.lib/capybara/screenshot/diff/image_preprocessor.rb (1)
53-60: Guard against repeated work & return type consistency.
driver.filter_image_with_medianpresumably returns a new image, but if the driver
mutates in-place and returnsnilyou will silently returnnilfrom the whole pipeline.
Safer:if driver.supports?(:filter_image_with_median) - driver.filter_image_with_median(image, median_filter_window_size) + driver.filter_image_with_median(image, median_filter_window_size) || imagelib/capybara/screenshot/diff/screenshot_matcher.rb (1)
41-47: Early exit skips screenshot capture whenfail_if_newis true.Raising before a fresh screenshot is taken means you cannot visually inspect what the page
looked like. Many teams prefer to still capture the actual screenshot and then fail.
Verify that this behavioural change is intentional; otherwise move the check after
ScreenshotCoordinator.capture.lib/capybara/screenshot/diff/test_methods.rb (3)
108-116: Double+1onskip_stack_framesmay offset backtraces
screenshotadds+1when callingbuild_screenshot_assertion, and that method adds another+1before delegating toScreenshotMatcher.
Unless each intermediate frame is purposely skipped, this results in two frames being dropped for every call site, which can hide the real origin of a failure.-assertion = build_screenshot_assertion(screenshot_full_name, options, skip_stack_frames: skip_stack_frames + 1) +assertion = build_screenshot_assertion(screenshot_full_name, options, skip_stack_frames:)and drop the extra
+1insidebuild_screenshot_assertion.Please confirm intended behaviour.
80-83:Details
❌ Incorrect review comment
Constant name differs from namespace – risk of
NameError
schedule_match_jobcallsCapybaraScreenshotDiff.add_assertion, but the file lives underCapybara::Screenshot::Diff.
If the top-level constant is not defined, assertions will never be stored.
🏁 Script executed:
#!/bin/bash # Verify constant and method exist rg -n $'^module CapybaraScreenshotDiff' rg -n $'def +add_assertion'Length of output: 1171
🏁 Script executed:
#!/bin/bash sed -n '1,50p' lib/capybara_screenshot_diff.rbLength of output: 1691
🏁 Script executed:
#!/bin/bash sed -n '1,200p' lib/capybara/screenshot/diff/test_methods.rbLength of output: 5664
No risk of NameError – constant lookup is correct
Ruby’s unqualified constant lookup will traverse the lexical scope of
Capybara::Screenshot::Diff::TestMethodsup to the top‐level, finding the
CapybaraScreenshotDiffmodule defined inlib/capybara_screenshot_diff.rb.
Because the gem’s main file is required (defining that module) before any
tests or calls toschedule_match_job, the call to
CapybaraScreenshotDiff.add_assertion(job)will always succeed at runtime.• The bare constant
CapybaraScreenshotDiffinschedule_match_jobresolves to::CapybaraScreenshotDiff.
• The top‐level module is defined during gem initialization, well before any invocation of this method.No changes are necessary.
Likely an incorrect or invalid review comment.
124-128:Details
✅ Verification successful
job.callermay benil– verify API compatibility
invoke_match_jobassumesScreenshotAssertionresponds tocaller, but previous implementations stored the backtrace separately.
IfScreenshotAssertiondoes not exposecaller,_raise_errorwill fail and swallow the original assertion message.Run the search below to check for an accessor:
🏁 Script executed:
#!/bin/bash # Look for `def caller` or `attr_... :caller` in ScreenshotAssertion rg -n --context 2 $'class ScreenshotAssertion|module ScreenshotAssertion' ; rg -n '\bcaller\b' | headLength of output: 1771
🏁 Script executed:
#!/bin/bash # Search for ScreenshotJob class and its methods rg -n 'class .*ScreenshotJob' -A 5 lib rg -n 'def caller' -A 3 lib/capybara/screenshot/diff/test_methods.rb rg -n 'invoke_match_job' -A 5 lib/capybara/screenshot/diff/test_methods.rbLength of output: 525
🏁 Script executed:
#!/bin/bash # Show the beginning of test_methods.rb to inspect how jobs are created and stored sed -n '1,200p' lib/capybara/screenshot/diff/test_methods.rbLength of output: 5664
calleraccessor confirmed in ScreenshotAssertion – no changes neededWe’ve verified that
ScreenshotAssertiondeclaresattr_accessor :callerinlib/capybara_screenshot_diff/screenshot_assertion.rb:8, so callingjob.calleris API-compatible and won’t raise a NoMethodError.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/capybara/screenshot/diff/image_compare.rb (1)
147-153: Inefficient binary file comparisonThe current implementation reads entire files into memory for comparison, which can be inefficient for large image files.
Consider using
FileUtils.compare_fileinstead for a more efficient streaming comparison:- def files_identical?(file1, file2) - # Compare file contents - File.read(file1) == File.read(file2) - rescue - # If there's any error reading the files, they're not identical - false - end + require "fileutils" # Add this near other requires + + def files_identical?(file1, file2) + FileUtils.compare_file(file1, file2) + rescue + # If there's any error reading the files, they're not identical + false + endAlternatively, if you prefer to keep the current approach, use
File.binreadinstead ofFile.readto handle binary files correctly:- File.read(file1) == File.read(file2) + File.binread(file1) == File.binread(file2)
🧹 Nitpick comments (4)
lib/capybara/screenshot/diff/image_compare.rb (1)
150-152: Improve error handling specificityThe current rescue block catches all exceptions, which could mask important errors unrelated to file comparison.
Consider catching specific exceptions to make the code more robust:
def files_identical?(file1, file2) # Compare file contents File.read(file1) == File.read(file2) - rescue + rescue IOError, Errno::ENOENT, Errno::EACCES => e + # Log the specific error for debugging + puts "File comparison error: #{e.message}" if driver_options[:debug] # If there's any error reading the files, they're not identical false endlib/capybara/screenshot/diff/test_methods.rb (3)
46-62: Consider refactoring the redundant synchronization pattern.The method first synchronizes the namer's state with instance variables before delegating to it. This pattern is repeated in multiple methods (
screenshot_dir,group_parts), which could lead to maintenance issues if the variables get out of sync.Consider extracting this synchronization to a single private method:
-def build_full_name(name) - # Delegate to screenshot_namer while maintaining compatibility with legacy approach - if @screenshot_namer - # Update the namer's section and group from our instance variables - @screenshot_namer.section = @screenshot_section - @screenshot_namer.group = @screenshot_group - @screenshot_namer.full_name(name) - else +def build_full_name(name) + # Delegate to screenshot_namer while maintaining compatibility with legacy approach + if @screenshot_namer + sync_namer_with_instance_vars + @screenshot_namer.full_name(name) + elseWith a private method added:
def sync_namer_with_instance_vars @screenshot_namer.section = @screenshot_section @screenshot_namer.group = @screenshot_group end
67-77: Redundant synchronization pattern repeated here.This method follows the same pattern of synchronizing the namer's state with instance variables before delegating to it.
Consider using the same refactoring approach suggested for
build_full_nameto reduce duplication.
101-114: Same redundant synchronization pattern in group_parts.This method follows the same pattern of synchronizing the namer's state with instance variables before delegating to it.
Consider using the proposed
sync_namer_with_instance_varsmethod here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
lib/capybara/screenshot/diff/capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/image_compare.rb(3 hunks)lib/capybara/screenshot/diff/test_methods.rb(5 hunks)lib/capybara_screenshot_diff/screenshot_namer.rb(1 hunks)test/capybara/screenshot/diff/stable_screenshoter_test.rb(2 hunks)test/capybara/screenshot/diff_test.rb(4 hunks)test/capybara_screenshot_diff/screenshot_namer_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/capybara/screenshot/diff/stable_screenshoter_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- test/capybara/screenshot/diff_test.rb
- lib/capybara/screenshot/diff/capture_strategy.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/capybara_screenshot_diff/screenshot_namer_test.rb (1)
lib/capybara_screenshot_diff/screenshot_namer.rb (7)
group(34-37)current_group_directory(71-73)full_name(42-51)directory_parts(62-67)section(27-30)full_name_with_path(56-58)clear_current_group_directory(77-80)
🔇 Additional comments (20)
lib/capybara/screenshot/diff/image_compare.rb (10)
3-8: Good addition of specialized modulesThe introduction of specialized modules (
ComparisonLoader,ImagePreprocessor,DifferenceFinder) reflects good separation of concerns, making the codebase more maintainable and testable.
15-28: Excellent documentation of the optimization strategyThe expanded documentation clearly explains the layered approach to image comparison, which is crucial for understanding the performance benefits of this refactoring. This documentation will be valuable for future maintainers.
37-37: Early validation of file existenceGood practice to validate file existence in the constructor to fail fast. This prevents errors from cascading further in the processing pipeline.
46-56: Well-implemented optimization strategyThe
quick_equal?method now follows a proper optimization path by checking file sizes and performing a byte-by-byte comparison before resorting to more expensive image processing operations.
58-61: Centralized file existence checkMoving the file existence check to a dedicated method is a good refactoring that prevents code duplication and provides clear error messages.
89-99: Good use of lazy initialization for dependenciesUsing memoized accessors for specialized components ensures they're only created when needed and improves code organization.
101-110: Improved separation of concernsThe
find_differencemethod now properly delegates to specialized components, creating a clean workflow from validation to comparison.
124-131: Well-documented method with clear responsibilitiesThe
load_comparisonmethod has good documentation and a clear single responsibility - to load and preprocess images for comparison.
112-116: Good practice resetting cached valuesResetting the cached error message and reporter when the difference object changes ensures consistency between the difference object and its reporting.
133-139: Helpful utility methods for null objectsThe addition of helper methods for creating null objects is a good application of the Null Object pattern, improving code readability and error handling.
lib/capybara_screenshot_diff/screenshot_namer.rb (1)
12-86: Implementation looks solid with good organization and documentation.The
ScreenshotNamerclass effectively encapsulates screenshot naming logic and directory management in a reusable component. The class has clear responsibilities, well-documented methods, and proper handling of edge cases (nil values, empty strings). The hierarchical organization with sections and groups provides a flexible way to structure screenshots.I particularly like:
- The clear separation of concerns
- Thorough documentation explaining the purpose of each method
- Proper handling of edge cases
- Clean implementation of directory path construction
test/capybara_screenshot_diff/screenshot_namer_test.rb (1)
7-154: Comprehensive test coverage for the new ScreenshotNamer class.The test suite thoroughly covers all functionality of the
ScreenshotNamerclass, including:
- Group and section settings
- Counter behavior and reset logic
- Path generation with various combinations of settings
- Directory operations
- Edge cases like nil or empty values
Good practices observed:
- Proper setup/teardown with temporary directories
- Isolated tests with clear purposes
- Testing both happy paths and edge cases
lib/capybara/screenshot/diff/test_methods.rb (8)
17-17: Appropriate addition of the new dependency.Good addition of the required file for the new
ScreenshotNamerclass.
33-36: Good integration of the new ScreenshotNamer class.The initialization now creates a
ScreenshotNamerinstance while maintaining the original instance variables for backward compatibility.
79-82: Good update of the ScreenshotNamer when section changes.The method properly updates both the instance variable and the namer object.
84-91: Good update of the ScreenshotNamer when group changes.The method correctly updates both the instance variable and the namer object.
132-143: Improved error handling and conditional logic.The refactored code provides cleaner conditional handling for delayed execution and returns clearer values. The early return when assertion is false is a good pattern.
151-152: Simplified error raising with appropriate backtrace.Good improvement by passing the caller context to the error raising method.
158-159: Good use of the custom exception class with backtrace.Properly utilizing the custom exception class to maintain the backtrace context.
161-163: Well-refactored screenshot assertion building.The method has been simplified to delegate to the
ScreenshotMatcherclass, maintaining proper backtrace handling with theskip_stack_framesparameter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/capybara/screenshot/diff/test_methods.rb (1)
74-74: Consider reusing the result from build_full_name.There seems to be a potential duplication since you're calculating the full name again, although it was already determined in
build_full_name. Consider passing the full name from the caller to avoid calculating it twice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
lib/capybara/screenshot/diff/test_methods.rb(3 hunks)lib/capybara_screenshot_diff/dsl.rb(0 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(4 hunks)test/capybara/screenshot/diff/screenshoter_test.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(4 hunks)test/capybara/screenshot/diff_test.rb(6 hunks)test/fixtures/files/rspec_spec.rb(1 hunks)test/integration/test_methods_system_test.rb(1 hunks)test/support/non_minitest_assertions.rb(1 hunks)test/test_helper.rb(2 hunks)
💤 Files with no reviewable changes (1)
- lib/capybara_screenshot_diff/dsl.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- test/test_helper.rb
- lib/capybara_screenshot_diff/screenshot_assertion.rb
- test/capybara/screenshot/diff_test.rb
- test/capybara/screenshot/diff/test_methods_test.rb
🧰 Additional context used
🪛 GitHub Actions: Test
test/fixtures/files/rspec_spec.rb
[error] 16-16: RSpec test failure in CapybaraScreenshotDiff::RspecTest#test_should_support_rspec: Screenshot does not match for 'index' with area_size 6336 and max_color_distance 102.5.
[error] 38-38: RSpec test failure in CapybaraScreenshotDiff::RspecTest#test_should_support_rspec: Screenshot does not match for 'index' with area_size 6336 and max_color_distance 102.5.
🔇 Additional comments (14)
test/fixtures/files/rspec_spec.rb (2)
27-27: Namespace change is aligned with the refactoringThe change from expecting
Capybara::Screenshot::Diff::TestMethodstoCapybaraScreenshotDiff::DSLis consistent with the broader refactoring to simplify test framework integrations.
27-27: Note: Screenshot matching failures in pipelineThere are pipeline failures related to screenshot matching on lines 16 and 38. These might be caused by the refactoring changes to how screenshots are handled or by environmental differences in the test runner.
You may need to update the reference screenshots or adjust the tolerance parameters for the tests.
test/integration/test_methods_system_test.rb (1)
18-18: Consistent module inclusion refactoringUpdating to use
CapybaraScreenshotDiff::DSLinstead of the previous TestMethods module is consistent with the broader refactoring pattern. This simplifies the module inclusion chain while maintaining the same functionality.test/capybara/screenshot/diff/screenshoter_test.rb (1)
9-9: Consistent DSL module consolidationThe change to include
CapybaraScreenshotDiff::DSLinstead of the previous module is part of the overall test framework simplification. This approach centralizes the DSL methods while maintaining compatibility with existing tests.test/support/non_minitest_assertions.rb (1)
9-9: Consistent integration simplificationThe change to include
CapybaraScreenshotDiff::DSLaligns with the overall refactoring strategy. This simplifies how non-Minitest frameworks integrate with the screenshot diffing functionality while maintaining the same setup and teardown behavior.lib/capybara/screenshot/diff/test_methods.rb (9)
17-17: Good addition of the new screenshot_namer dependency.This change supports the refactoring effort by properly requiring the new ScreenshotNamer class that centralizes screenshot naming logic.
35-35: Good refactoring to use the centralized screenshot namer.Initializing with the shared screenshot namer instance improves modularity by delegating naming responsibilities to a specialized class.
43-44: Well-designed delegation to screenshot_namer.Good refactoring to delegate all naming operations to the specialized ScreenshotNamer class. This approach centralizes the screenshot naming logic, making the code more maintainable and easier to test.
Also applies to: 47-48, 51-52, 58-59
77-79: Good improvement to screenshot assertion building.The transition from
build_screenshot_matches_jobtobuild_screenshot_assertionprovides clearer semantics and better represents the purpose of the operation. The early return on line 79 is also a good defensive programming practice.
81-88: Cleaner conditional flow for handling screenshot assertions.This refactoring improves readability by clearly separating the decision to delay or immediately invoke the screenshot matching assertion.
92-95: Good simplification of the schedule_match_job method.Making this method private and simplifying its implementation improves the API by clearly indicating it's an internal implementation detail.
108-108: Improved error reporting with proper backtrace handling.Directly passing the backtrace to the exception constructor preserves the original call stack, which will make debugging easier.
111-113: Good implementation of the new screenshot assertion building.The new method properly leverages the ScreenshotMatcher class and provides better stack frame skipping for improved error reporting.
3-115: Overall excellent refactoring that improves modularity and maintainability.This refactoring successfully:
- Centralizes screenshot naming logic in a dedicated class
- Improves error handling with better backtrace support
- Streamlines the API by making internal methods private
- Enhances readability with clearer method names and semantics
- Maintains backward compatibility while improving the internal structure
The changes are well-aligned with the PR objectives to simplify test framework integrations.
There was a problem hiding this comment.
Pull Request Overview
A refactor to streamline test framework integration by removing legacy test methods, centralizing screenshot naming and orchestration, and improving error backtrace filtering.
- Removes the old
TestMethodsmodule in favor of a modular DSL and coordinator - Introduces
ScreenshotCoordinatorfor unified capture strategies (standard/stable) - Enhances error classes with filtered backtraces and optimizes image comparison logic
Reviewed Changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara_screenshot_diff/backtrace_filter.rb | Adds backtrace filtering class to strip library paths and common gems |
| lib/capybara_screenshot_diff.rb | Switches error base class to use filtered backtrace |
| lib/capybara/screenshot/diff/screenshot_namer_dsl.rb | Introduces DSL for section/group naming, replacing the removed test methods |
| lib/capybara/screenshot/diff/screenshot_coordinator.rb | Centralizes capture strategy selection and snapshot orchestration |
| lib/capybara/screenshot/diff/image_compare.rb | Refactors layered image comparison with early exit checks and preprocessing |
Comments suppressed due to low confidence (1)
lib/capybara/screenshot/diff/image_compare.rb:39
quick_equal?does not initialize or assign@differencebefore callingdifferent?, leading to incorrect results or errors. Consider computing the difference in quick mode or returning explicitly based on file checks only.
def quick_equal?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/capybara/screenshot/diff/screenshot_namer_dsl.rb (1)
3-17: Trim unnecessaryrequires to speed-up boot time & lower couplingThe DSL itself only needs access to
CapybaraScreenshotDiff::ScreenshotNamer, yet it eagerly loads Capybara, Rails, custom image-processing helpers, drivers, VCS helpers, etc. This:
- increases gem load-time (all test runs pay the cost),
- pulls in heavy transitive dependencies when the DSL is required stand-alone,
- hides the DSL’s true dependency surface, making future refactors harder.
Consider deferring heavyweight requires to the components that actually use them or lazily loading them (e.g. inside the first method call).
-require "English" -require "capybara" -require "action_controller" -require "action_dispatch" -require "active_support/core_ext/string/strip" -require "pathname" - -require_relative "drivers" -require_relative "image_compare" -require_relative "vcs" -require_relative "browser_helpers" -require_relative "region" - -require_relative "screenshot_matcher" -require "capybara_screenshot_diff/screenshot_namer" +require "capybara_screenshot_diff/screenshot_namer" # the only direct dependencyIf any of the dropped files are still required elsewhere, load them where they are used rather than here.
lib/capybara_screenshot_diff/dsl.rb (1)
31-33:optionsstill contains:delayedwhen forwarded toScreenshotMatcher
ScreenshotMatcher.new(name, options)will receive every key inoptions, including the DSL-specific:delayedflag that it probably does not understand.
IfScreenshotMatcherdoes not ignore unknown keys, callers could get confusing “unknown option” errors.-assertion = build_screenshot_assertion(full_name, options, skip_stack_frames: skip_stack_frames + 1) +matcher_opts = options.except(:delayed) +assertion = build_screenshot_assertion(full_name, matcher_opts, skip_stack_frames: skip_stack_frames + 1)(Use
Hash#exceptfrom ActiveSupport oroptions.dup.tap { |h| h.delete(:delayed) }to avoid mutating the original hash.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
lib/capybara/screenshot/diff/screenshot_namer_dsl.rb(1 hunks)lib/capybara/screenshot/diff/test_methods.rb(0 hunks)lib/capybara_screenshot_diff.rb(1 hunks)lib/capybara_screenshot_diff/dsl.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(5 hunks)test/capybara/screenshot/diff_test.rb(6 hunks)
💤 Files with no reviewable changes (1)
- lib/capybara/screenshot/diff/test_methods.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/capybara_screenshot_diff.rb
- lib/capybara_screenshot_diff/screenshot_assertion.rb
- test/capybara/screenshot/diff_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/capybara/screenshot/diff/screenshot_namer_dsl.rb (2)
test/capybara/screenshot/diff_test.rb (1)
screenshot_namer(237-239)lib/capybara_screenshot_diff/screenshot_namer.rb (2)
section(27-30)group(34-37)
🔇 Additional comments (2)
lib/capybara/screenshot/diff/screenshot_namer_dsl.rb (1)
24-37: Globalscreenshot_namerstate may leak between parallel tests
#screenshot_section/#screenshot_groupmutate a single globalCapybaraScreenshotDiff.screenshot_namer.
In parallel test runners (e.g. RSpec--jobs, Minitest’s parallel), two threads may change section/group concurrently, causing interleaved names.Please verify that:
- tests are never run concurrently or
- the underlying
ScreenshotNameris thread-safe / duplicated per test process or- the DSL resets state (e.g.
ensure { screenshot_section(nil); screenshot_group(nil) }) at the end of each example.Failing to do so will randomly mismatch screenshots and baselines.
lib/capybara_screenshot_diff/dsl.rb (1)
58-62:skip_stack_framesis incremented twice – confirm intent
#screenshotadds+1when callingbuild_screenshot_assertion, and that method adds another+1before delegating toScreenshotMatcher.
This means the effective skip count isrequested + 2. If the goal is to hide both wrapper methods from the backtrace that’s correct, otherwise real error locations may be skipped.Please double-check that the resulting backtrace still points to the user’s test line; if not, adjust one of the increments.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and simplifies the test framework integrations for visual regression testing by streamlining image comparison, error filtering, and screenshot naming. Key changes include the introduction of a new backtrace filter module and error handling using filtered backtraces, the addition of a screenshot naming DSL and a screenshot coordinator for better modularity, and the removal of legacy test methods.
Reviewed Changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara_screenshot_diff/backtrace_filter.rb | Adds a backtrace filter to remove unnecessary lines from error backtraces. |
| lib/capybara_screenshot_diff.rb | Updates error handling to use ErrorWithFilteredBacktrace. |
| lib/capybara/screenshot/diff/test_methods.rb | Removes legacy test helper methods. |
| lib/capybara/screenshot/diff/standard_capture_strategy.rb | Introduces a standard capture strategy for immediate screenshot capture. |
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Updates error raising for unstable images with enhanced backtrace. |
| lib/capybara/screenshot/diff/stable_capture_strategy.rb | Implements a stable capture strategy that waits for page content stabilization. |
| lib/capybara/screenshot/diff/screenshoter.rb | Adjusts error reporting for image loading timeouts with improved backtrace. |
| lib/capybara/screenshot/diff/screenshot_namer_dsl.rb | Provides a DSL for setting screenshot sections and groups. |
| lib/capybara/screenshot/diff/screenshot_matcher.rb | Refactors screenshot matching logic and error raising when base screenshots are missing. |
| lib/capybara/screenshot/diff/screenshot_coordinator.rb | Adds a coordinator to choose and execute the appropriate capture strategy. |
| lib/capybara/screenshot/diff/reporters/default.rb | Updates annotation methods to reference the new comparison structure. |
| lib/capybara/screenshot/diff/image_preprocessor.rb | Adds an image preprocessor for applying skip areas and filters. |
| lib/capybara/screenshot/diff/image_compare.rb | Optimizes image comparison with early, file-based checks and improved processing. |
| lib/capybara/screenshot/diff/drivers/base_driver.rb | Adds a supports? helper to the base driver for feature detection. |
| lib/capybara/screenshot/diff/difference_finder.rb | Enhances difference finding logic and performance optimizations. |
| lib/capybara/screenshot/diff/difference.rb | Updates the Difference value object with keyword support and additional accessors. |
| lib/capybara/screenshot/diff/comparison_loader.rb | Introduces a helper for loading and assembling image comparisons. |
| lib/capybara/screenshot/diff/comparison.rb | Augments Comparison with a helper method for accessing skip areas. |
| lib/capybara/screenshot/diff/capture_strategy.rb | Defines an abstract base class for screenshot capture strategies. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
lib/capybara/screenshot/diff/reporters/default.rb (1)
78-85: 🛠️ Refactor suggestionAdd trailing comma & avoid emitting a non-existent heat-map path
- RuboCop warns about the missing trailing comma on the last element of this multi-line array.
- When
difference.diff_maskis nil we never callsave_heatmap_diff, yet the error message still listsheatmap_diff_path, which can confuse users (and CI logs) because the file is absent.Suggested refactor:
- [ - "(#{difference.inspect})", - image_path.to_path, - annotated_base_image_path.to_path, - annotated_image_path.to_path, - heatmap_diff_path.to_path - ].join(NEW_LINE) + message_lines = [ + "(#{difference.inspect})", + image_path.to_path, + annotated_base_image_path.to_path, + annotated_image_path.to_path, + ] + message_lines << heatmap_diff_path.to_path if difference.diff_mask + message_lines.join(NEW_LINE)This both satisfies Style/TrailingCommaInArrayLiteral and prevents dangling paths in logs.
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/screenshot_matcher.rb (1)
85-91: 💡 Verification agent🧩 Analysis chain
Potentially unused method
The
build_screenshoter_formethod appears to be unused after the refactoring, as screenshot creation is now delegated toScreenshotCoordinator.capture. Consider removing this method if it's not used elsewhere.
🏁 Script executed:
#!/bin/bash # Verify if build_screenshoter_for is used anywhere in the codebase rg --type ruby "build_screenshoter_for" -C 2Length of output: 732
Remove unused
build_screenshoter_formethodA search across the repo shows no calls to
build_screenshoter_forbeyond its own declaration. It can be safely removed to eliminate dead code.• Location:
- lib/capybara/screenshot/diff/screenshot_matcher.rb (approx. lines 85–91)
Suggested removal diff:
- def build_screenshoter_for(capture_options, comparison_options = {}) - if capture_options[:stability_time_limit] - StableScreenshoter.new(capture_options, comparison_options) - else - Diff.screenshoter.new(capture_options, comparison_options[:driver]) - end - end
♻️ Duplicate comments (5)
lib/capybara/screenshot/diff/screenshot_matcher.rb (1)
50-50: Typo in commentThe comment "Load new screenshot from Browser" should be corrected.
- # Load new screenshot from Browser + # Load a new screenshot from Browserlib/capybara/screenshot/diff/difference.rb (2)
21-39: 🛠️ Refactor suggestionAvoid custom
initialize; usekeyword_init: trueon the Struct insteadThe manual splat + kwargs dance re-implements functionality Ruby already gives you for free (
Struct.new(..., keyword_init: true)) and brings several risks:
- Mixed positional + keyword arguments silently drop the positional ones.
- The hard-coded
<= 6heuristic will break if a seventh attribute is ever added.- Calling
super(*args)relies on callers always passing the exact arity/ordering.The previous review already covered this; the safest route is to delete
#initializeentirely and declare the struct withkeyword_init: true, which lets both positional and keyword construction continue to work.-class Difference < Struct.new(:region, :meta, :comparison, :failed_by, :image_path, :base_image_path) +# NOTE: keyword_init keeps positional support while adding robust kwarg support. +class Difference < Struct.new( + :region, :meta, :comparison, :failed_by, :image_path, :base_image_path, + keyword_init: true +)Then the whole custom
initializeblock can be removed.
41-49: 🛠️ Refactor suggestionParameter order in
build_nullis still reversed – leads to swapped paths
build_null(comparison, base_path, new_path …)passesnew_pathintoimage_pathandbase_pathintobase_image_path, the opposite of what the argument names advertise. This is the same issue pointed out in an earlier review.Either flip the order or switch to explicit keywords to make the call-site unambiguous:
-def self.build_null(comparison, base_path, new_path, failed_by = nil) - Difference.new( - nil, - {difference_level: nil, max_color_distance: 0}, - comparison, - failed_by, - new_path, # ← paths are swapped - base_path - ).freeze +def self.build_null(comparison:, base_image_path:, new_image_path:, failed_by: nil) + new( + comparison: comparison, + failed_by: failed_by, + image_path: new_image_path, + base_image_path: base_image_path, + region: nil, + meta: {difference_level: nil, max_color_distance: 0}, + ).freeze endtest/capybara_screenshot_diff/dsl_test.rb (1)
41-44: Same string-comparison caveat as above applies here forvalue. Consider theGem::Versionapproach for consistency.lib/capybara/screenshot/diff/image_compare.rb (1)
148-155: Efficient file comparison with proper error handlingUsing
FileUtils.identical?is more efficient than reading entire files into memory, addressing previous performance concerns. The added error handling ensures robustness.
🧹 Nitpick comments (9)
lib/capybara/screenshot/diff/reporters/default.rb (1)
90-95: Guard against unexpected nildiff_maskinsidesave_heatmap_diff
save_heatmap_diffis currently protected by the caller (annotate_and_save_images), but adding an internal guard makes the method safer for direct use and future refactors:- merged_image = driver.merge(new_image, base_image) - highlighted_mask = driver.highlight_mask(difference.diff_mask, merged_image, color: DIFF_COLOR) + return unless difference.diff_mask + + merged_image = driver.merge(new_image, base_image) + highlighted_mask = driver.highlight_mask(difference.diff_mask, merged_image, color: DIFF_COLOR)A trivial check avoids accidental
NilClasserrors if the method is invoked elsewhere.test/support/stub_test_methods.rb (1)
4-4: Require path looks good – consider removing the now-empty file altogether
require_relative "capybara_screenshot_diff/dsl_stub"correctly loads the consolidated helper.
Because the original helper implementation was fully extracted, this file now only delegates; you might delete it and referencecapybara_screenshot_diff/dsl_stubdirectly fromtest_helper.rbto reduce indirection.test/capybara/screenshot/diff/image_preprocessor_test.rb (1)
97-104: Consider simplifying the warning capture blockThe capture and verification logic could be simplified for better readability.
- warning_output = capture_io do - result = preprocessor.call(@test_images) - - - # Verify images unchanged - assert_equal @test_images, result - assert_empty driver.filter_calls - end + warning_output = capture_io do + result = preprocessor.call(@test_images) + end + + # Verify images unchanged + assert_equal @test_images, result + assert_empty driver.filter_callslib/capybara/screenshot/diff/screenshot_matcher.rb (2)
25-62: Refactored method is more functional but still complexThe renamed
build_screenshot_assertionmethod now creates a richer object and properly handles error cases, but it's still quite complex as indicated by the RuboCop ABC size warning (40.64/23).Consider breaking down this method further to reduce complexity:
- Extract the early return conditions into separate guard methods
- Move the area calculation logic to a separate method
- Extract the assertion creation logic to a dedicated method
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 25-62: Assignment Branch Condition size for build_screenshot_assertion is too high. [<6, 40, 4> 40.64/23]
(Metrics/AbcSize)
[convention] 60-60: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
57-61: Consider adding trailing comma to multiline arrayFollowing Ruby style conventions, add a trailing comma to the last element of the multiline array.
CapybaraScreenshotDiff::ScreenshotAssertion.from([ caller(skip_stack_frames + 1), screenshot_full_name, - ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options) + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options), ])🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 60-60: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/support/capybara_screenshot_diff/dsl_stub.rb (1)
1-4: Consider adding a module-level documentation commentAdd a descriptive comment explaining the purpose and usage of this module to help other developers understand its role in the test suite.
+# DSLStub provides test helpers for screenshot comparison testing. +# It manages test setup/teardown with SnapManager and provides methods +# to create snapshots, prepare test images, and build test doubles for image comparison. + module CapybaraScreenshotDiff module DSLStub extend ActiveSupport::Concernlib/capybara_screenshot_diff/dsl.rb (1)
30-34: Convert symbol screenshot names to String to avoid surprisingfull_nameerrors
full_name = CapybaraScreenshotDiff.screenshot_namer.full_name(name)receivesname, which may be a Symbol (tests callscreenshot(:a)).
UnlessScreenshotNamer#full_nameexplicitly handles Symbols, you could raise aNoMethodErrorwhen it tries to perform string operations. A simple coercion keeps the API forgiving:- full_name = CapybaraScreenshotDiff.screenshot_namer.full_name(name) + full_name = CapybaraScreenshotDiff.screenshot_namer.full_name(name.to_s)test/capybara_screenshot_diff/dsl_test.rb (2)
24-27: UseGem::Versionfor RUBY_VERSION comparison to avoid string pitfallsString comparison works for “2.x” releases but will fail once minor versions reach double digits (e.g.,
"2.10" < "2.4"). Safer:value = if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("2.4") 187.4 else 188 end
127-133: Silence RuboCop parser error by naming the splat argumentStatic analysis flagged
unexpected token tRPAREN; it stems fromdef assert_image_not_changed(*).
The anonymous splat is valid Ruby but trips some older parsers. Naming the splat keeps intent while restoring tool compatibility:-def assert_image_not_changed(*) +def assert_image_not_changed(*args) CapybaraScreenshotDiff::ScreenshotAssertion.assert_image_not_changed(*args) end🧰 Tools
🪛 RuboCop (1.75.5)
[fatal] 132-132: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure usingTargetRubyVersionparameter, underAllCops)(Lint/Syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (23)
lib/capybara/screenshot/diff/difference.rb(2 hunks)lib/capybara/screenshot/diff/image_compare.rb(3 hunks)lib/capybara/screenshot/diff/reporters/default.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_matcher.rb(3 hunks)lib/capybara/screenshot/diff/screenshot_namer_dsl.rb(1 hunks)lib/capybara_screenshot_diff/backtrace_filter.rb(1 hunks)lib/capybara_screenshot_diff/dsl.rb(1 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(1 hunks)test/capybara/screenshot/diff/difference_finder_test.rb(1 hunks)test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb(2 hunks)test/capybara/screenshot/diff/drivers/vips_driver_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_refactor_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_test.rb(2 hunks)test/capybara/screenshot/diff/image_preprocessor_test.rb(1 hunks)test/capybara/screenshot/diff/screenshoter_test.rb(1 hunks)test/capybara/screenshot/diff/stable_screenshoter_test.rb(3 hunks)test/capybara/screenshot/diff/test_methods_test.rb(0 hunks)test/capybara/screenshot/diff_test.rb(8 hunks)test/capybara_screenshot_diff/dsl_test.rb(1 hunks)test/integration/test_methods_system_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/support/stub_test_methods.rb(1 hunks)test/test_helper.rb(2 hunks)
💤 Files with no reviewable changes (1)
- test/capybara/screenshot/diff/test_methods_test.rb
🚧 Files skipped from review as they are similar to previous changes (11)
- test/integration/test_methods_system_test.rb
- test/capybara/screenshot/diff/stable_screenshoter_test.rb
- test/test_helper.rb
- test/capybara/screenshot/diff/screenshoter_test.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- lib/capybara/screenshot/diff/screenshot_namer_dsl.rb
- lib/capybara_screenshot_diff/backtrace_filter.rb
- test/capybara/screenshot/diff/image_compare_refactor_test.rb
- test/capybara/screenshot/diff/image_compare_test.rb
- test/capybara/screenshot/diff_test.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/capybara/screenshot/diff/screenshot_matcher.rb (4)
lib/capybara_screenshot_diff/dsl.rb (1)
build_screenshot_assertion(61-65)test/capybara/screenshot/diff/test_doubles.rb (1)
exist?(23-25)lib/capybara/screenshot/diff/screenshot_coordinator.rb (1)
capture(23-27)lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
from(17-25)
test/capybara/screenshot/diff/image_preprocessor_test.rb (2)
lib/capybara/screenshot/diff/image_preprocessor.rb (2)
call(34-36)skip_area(69-71)test/capybara/screenshot/diff/test_doubles.rb (2)
call(110-114)size(19-21)
🪛 RuboCop (1.75.5)
lib/capybara/screenshot/diff/screenshot_matcher.rb
[convention] 25-62: Assignment Branch Condition size for build_screenshot_assertion is too high. [<6, 40, 4> 40.64/23]
(Metrics/AbcSize)
[convention] 60-60: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/capybara_screenshot_diff/dsl_test.rb
[fatal] 132-132: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
lib/capybara/screenshot/diff/reporters/default.rb
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🔇 Additional comments (26)
test/capybara/screenshot/diff/drivers/vips_driver_test.rb (1)
17-17: DSL stub inclusion looks correct – verify that the helper is auto-required in every test run
include CapybaraScreenshotDiff::DSLStubbrings the new helper mix-ins into the test context and aligns this file with the refactor.
Just double-check thattest_helper.rb(orrails_helper.rb) still pulls intest/support/stub_test_methods.rb; otherwise, the constant may not be loaded when somebody runs this test in isolation (e.g. viaruby vips_driver_test.rb).
If that path is already covered, no further action is needed.test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb (2)
12-12: Updated module inclusion aligns with refactoringThis change replaces the previous
TestMethodsStubmodule with the newCapybaraScreenshotDiff::DSLStubmodule, which is part of the broader test framework refactoring.
149-150: Improved implementation of make_comparisonThe implementation now explicitly creates a snapshot and instantiates
ImageComparewith the snapshot paths and options. This approach is more direct and easier to understand than the previous implementation that usedsuperwith merged options.test/capybara/screenshot/diff/image_preprocessor_test.rb (1)
1-112: Well-structured test suite for the new ImagePreprocessor classThis new test file thoroughly covers the
ImagePreprocessorclass functionality with clear test cases for:
- No preprocessing options
- Skip area specification
- Median filter with VipsDriver
- Median filter with non-VipsDriver
Each test follows a clear setup, action, and verification pattern, making the tests easy to understand and maintain.
lib/capybara/screenshot/diff/screenshot_matcher.rb (1)
41-46: Improved error handling for missing base screenshotsNow raises a clear error when
fail_if_newis true and the base screenshot is missing, which is more explicit than the previous silent return.test/support/capybara_screenshot_diff/dsl_stub.rb (2)
5-16: Good setup and teardown handlingThe setup and teardown hooks properly manage the test environment by:
- Initializing a SnapManager instance
- Replacing the real screenshoter with a stub
- Cleaning up after tests
- Resetting configuration
This ensures tests are isolated and don't interfere with each other.
19-61: Well-designed helper methods for testingThe provided helper methods cover all the necessary functionality for testing screenshot diffing:
- Creating snapshots
- Setting up test images
- Building test doubles
- Taking stable screenshots
These methods will make it much easier to write clean, focused tests for the screenshot diffing functionality.
lib/capybara/screenshot/diff/difference.rb (1)
108-115: Verify fallback attribute names – possible typo may returnnil
new_image_pathfalls back tocomparison&.new_image_path, whileoriginal_image_pathfalls back tocomparison&.base_image_path.If the comparison object actually exposes
original_image_path(as historical code did), the second method will always yieldnil, breaking report generation. Please confirm the attribute names onComparisonand align them.- base_image_path || comparison&.base_image_path + base_image_path || comparison&.original_image_path(Or rename the reader on
Comparisonfor consistency.)lib/capybara_screenshot_diff/dsl.rb (1)
61-65:skip_stack_framesis incremented twice – confirm this is intentional
screenshotadds+1andbuild_screenshot_assertionadds another+1, so the user-supplied value is effectively+2.
This matches the current test expectations, but the double bump is subtle and will bite future refactors. Consider passing the raw value through once and letting the matcher handle the single increment.lib/capybara/screenshot/diff/image_compare.rb (17)
3-5: Good addition of required dependenciesAdding explicit requirements for
pathnameandfileutilsimproves clarity about dependencies and ensures the necessary libraries are available.
7-10: Excellent modularization of dependenciesThe refactoring properly separates concerns by requiring new collaborator classes for loading, preprocessing, and difference detection, which improves maintainability and testability.
17-30: Great documentation of the optimization strategyThe comprehensive documentation clearly explains the layered optimization approach and performance considerations, which is valuable for future maintenance and understanding the class's behavior.
39-39: Early validation of required filesMoving the file existence check to initialization is a good practice that prevents operations on invalid inputs.
48-55: Efficient file comparison optimizationThe refactored
quick_equal?method now includes a fast path using file size and content comparison, which can avoid expensive image processing operations for identical files.
57-58: Simplified return logicUsing
!different?is a clear and concise way to determine equality when the quick file checks don't provide a definitive answer.
60-63: Centralized file existence validationExtracting the file existence checks into a dedicated method eliminates duplication and improves error handling consistency.
76-76: Clean memoization patternUsing the builder method pattern with memoization for the reporter improves code organization and performance.
84-84: Added quick_mode parameter to support optimizationAdding the
quick_modeparameter allows client code to specify when full image comparison can be skipped, supporting the optimization strategy.
91-93: Proper use of dependency injection patternCreating and memoizing collaborator objects follows good object-oriented design principles and makes testing easier with potential for mocking.
95-101: Well-structured collaborator initializationThe consistent pattern for initializing and caching collaborator objects improves readability and performance by avoiding redundant object creation.
103-112: Effective delegation to specialized componentsThe refactored
find_differencemethod now properly delegates responsibilities to specialized components, improving separation of concerns and making the code more maintainable.
114-118: Clean state management in setterThe
difference=setter correctly resets dependent cached values when the difference is updated, preventing stale state issues.
120-123: Good default handlingThe
build_reportermethod handles the case where difference is nil by providing a sensible default, making the code more robust.
125-133: Well-documented method with clear responsibilityThe
load_comparisonmethod has excellent documentation and a clear single responsibility, enhancing maintainability.
135-141: Helpful utility methods for special casesThe null object pattern implementation with
build_null_differenceandbuild_null_comparisonis a clean way to handle edge cases without conditional logic throughout the codebase.
143-146: Simple and clear existence checkThe
images_exist?method provides a readable way to check if both required files exist.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/capybara/screenshot/diff/screenshot_matcher.rb (1)
50-50: Fixed comment typo.The comment has been properly corrected to remove the grammatical error.
🧹 Nitpick comments (2)
lib/capybara/screenshot/diff/screenshot_matcher.rb (2)
25-25: Good method signature refactoring but implementation is complex.The renamed method with the new
skip_stack_framesparameter properly supports the enhanced error reporting with filtered backtraces. However, the method has a high ABC complexity score (40.64/23).Consider breaking this method into smaller, more focused methods to reduce complexity and improve maintainability:
- def build_screenshot_assertion(skip_stack_frames: 0) + def build_screenshot_assertion(skip_stack_frames: 0) + check_window_size + prepare_screenshot_options + check_base_screenshot + + capture_options, comparison_options = extract_capture_and_comparison_options!(driver_options) + capture_screenshot(capture_options, comparison_options) + + # Pre-computation: No need to compare without base screenshot + return unless @snapshot.base_path.exist? + + create_screenshot_assertion(skip_stack_frames, comparison_options) + end + + private + + def check_window_size + return if BrowserHelpers.window_size_is_wrong?(Screenshot.window_size) + end + + def prepare_screenshot_options + area_calculator = AreaCalculator.new(driver_options.delete(:crop), driver_options[:skip_area]) + driver_options[:crop] = area_calculator.calculate_crop + driver_options[:skip_area] = area_calculator.calculate_skip_area + driver_options[:driver] = Drivers.for(driver_options[:driver]) + end + + def check_base_screenshot + @snapshot.checkout_base_screenshot + + if Capybara::Screenshot::Diff.fail_if_new && [email protected]_path.exist? + raise CapybaraScreenshotDiff::ExpectationNotMet.new(<<~ERROR.chomp, caller) + No existing screenshot found for #{@snapshot.base_path}! + To stop seeing this error disable by `Capybara::Screenshot::Diff.fail_if_new=false` + ERROR + end + end + + def capture_screenshot(capture_options, comparison_options) + Capybara::Screenshot::Diff::ScreenshotCoordinator.capture(@snapshot, capture_options, comparison_options) + end + + def create_screenshot_assertion(skip_stack_frames, comparison_options) + CapybaraScreenshotDiff::ScreenshotAssertion.from([ + caller(skip_stack_frames + 1), + screenshot_full_name, + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options), + ])🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 25-62: Assignment Branch Condition size for build_screenshot_assertion is too high. [<6, 40, 4> 40.64/23]
(Metrics/AbcSize)
57-61: Enhanced assertion creation with stack trace information.The code now returns a richer
ScreenshotAssertionobject with proper stack trace information, which improves error reporting.Add a trailing comma after the last element in the array to conform to Ruby style conventions:
CapybaraScreenshotDiff::ScreenshotAssertion.from([ caller(skip_stack_frames + 1), screenshot_full_name, - ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options) + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options), ])🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 60-60: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
lib/capybara/screenshot/diff/screenshot_matcher.rb(3 hunks)
🧰 Additional context used
🪛 RuboCop (1.75.5)
lib/capybara/screenshot/diff/screenshot_matcher.rb
[convention] 25-62: Assignment Branch Condition size for build_screenshot_assertion is too high. [<6, 40, 4> 40.64/23]
(Metrics/AbcSize)
[convention] 60-60: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🔇 Additional comments (3)
lib/capybara/screenshot/diff/screenshot_matcher.rb (3)
9-9: Correct addition of the screenshot coordinator dependency.This new require statement properly integrates the centralized screenshot capture functionality, which is a key part of the refactoring effort.
41-46: Improved error handling for missing base screenshots.This change enhances user experience by providing a clear error message when the base screenshot is missing and
fail_if_newis enabled, rather than silently proceeding.
51-51: Good delegation to the ScreenshotCoordinator.This change properly delegates screenshot capture to the centralized coordinator, which aligns with the architectural refactoring to modularize capture strategies.
There was a problem hiding this comment.
Pull Request Overview
The PR simplifies test framework integrations by refactoring screenshot comparison logic and removing legacy test methods. Key changes include the removal of outdated test methods, enhanced error reporting with filtered backtraces, and the introduction of modular components (capture strategies, screenshot naming DSL, and image preprocessing).
Reviewed Changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara_screenshot_diff/backtrace_filter.rb | Introduces a backtrace filter to clean up error backtraces. |
| lib/capybara_screenshot_diff.rb | Updates error inheritance and replaces legacy test method requirements. |
| lib/capybara/screenshot/diff/test_methods.rb | Legacy test methods module has been removed. |
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Updates error raising to use a constructor with caller information. |
| lib/capybara/screenshot/diff/screenshoter.rb | Refactors error raising for image load timeouts. |
| lib/capybara/screenshot/diff/screenshot_matcher.rb | Adjusts the screenshot assertion API and error behavior for missing baselines. |
| lib/capybara/screenshot/diff/reporters/default.rb | Updates diff mask access and annotation methods for clarity. |
| lib/capybara/screenshot/diff/image_compare.rb | Enhances file-based pre-checks and error handling for missing images. |
| lib/capybara/screenshot/diff/difference.rb | Improves the difference object with backward compatibility accessors. |
| Other files | Introduce new modular components for capture strategies, image preprocessing, and comparison loading. |
Comments suppressed due to low confidence (4)
lib/capybara/screenshot/diff/test_methods.rb:1
- The legacy test methods module has been completely removed. Ensure that all tests and consumers have been updated to use the new DSL and modular components, and that no dependent functionality is broken.
-# frozen_string_literal: true
lib/capybara/screenshot/diff/screenshot_matcher.rb:41
- [nitpick] The error message now clearly includes the missing baseline screenshot path; please confirm that this provides sufficient guidance for users to address the issue.
if Capybara::Screenshot::Diff.fail_if_new && [email protected]_path.exist?
lib/capybara/screenshot/diff/stable_screenshoter.rb:103
- Using the constructor with caller information improves error traceability; please verify that the UnstableImage error class consistently accepts and handles these arguments across all usages.
raise CapybaraScreenshotDiff::UnstableImage.new(attempts_reporter.generate, caller)
lib/capybara/screenshot/diff/image_compare.rb:39
- The addition of an 'ensure_files_exist!' check raising ArgumentError improves robustness; please confirm that downstream integrations expect this error type instead of a custom error.
ensure_files_exist!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/capybara/screenshot/diff/screenshot_matcher.rb (3)
25-39: Well-structured method refactoringThe refactored
build_screenshot_assertionmethod follows a clear sequence of operations with appropriate separation of concerns. Breaking down the functionality into smaller private methods makes the code more maintainable and easier to understand.I would suggest adding a method comment to document the purpose and return value for better documentation.
+ # Builds a screenshot assertion that verifies current browser state matches the base screenshot + # @param skip_stack_frames [Integer] Number of stack frames to skip when reporting errors + # @return [CapybaraScreenshotDiff::ScreenshotAssertion, nil] Screenshot assertion or nil if base screenshot doesn't exist def build_screenshot_assertion(skip_stack_frames: 0)
87-93: Add trailing comma for better diffsConsider adding a trailing comma after the last item in the multiline array to comply with style conventions and improve git diffs when items are added to the array later.
caller(skip_stack_frames + 1), screenshot_full_name, - ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options) + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options), ])🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
35-36: Consider formatting the TODO as a proper commentThe existing comment seems to be a TODO item or suggestion for future work.
# Pre-computation: No need to compare without base screenshot - # NOTE: Consider to return PreValid Assertion Value Object with hard coded valid result + # TODO: Consider returning a PreValid Assertion Value Object with hard-coded valid result return unless need_to_compare?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/capybara/screenshot/diff/screenshot_matcher.rb(2 hunks)lib/capybara_screenshot_diff.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/capybara_screenshot_diff.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/capybara/screenshot/diff/screenshot_matcher.rb (10)
lib/capybara_screenshot_diff/dsl.rb (1)
build_screenshot_assertion(61-65)test/capybara/screenshot/diff/test_doubles.rb (1)
exist?(23-25)lib/capybara/screenshot/diff/browser_helpers.rb (3)
window_size_is_wrong?(26-29)selenium?(22-24)session(95-97)test/integration/browser_screenshot_test.rb (1)
window_size(219-225)lib/capybara/screenshot/diff/reporters/default.rb (1)
driver(113-115)lib/capybara/screenshot/diff/area_calculator.rb (2)
calculate_crop(12-19)calculate_skip_area(23-44)lib/capybara/screenshot/diff/drivers.rb (1)
for(7-12)lib/capybara_screenshot_diff/snap.rb (1)
checkout_base_screenshot(22-24)lib/capybara/screenshot/diff/screenshot_coordinator.rb (1)
capture(23-27)lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
from(17-25)
🪛 RuboCop (1.75.5)
lib/capybara/screenshot/diff/screenshot_matcher.rb
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🔇 Additional comments (6)
lib/capybara/screenshot/diff/screenshot_matcher.rb (6)
9-9: Good addition of the screenshot coordinatorThe new dependency on the ScreenshotCoordinator module supports the refactoring strategy to centralize screenshot capture logic.
43-45: Good extraction of comparison checkClear and concise helper method that improves readability of the main method.
47-62: Excellent window size validationThe implementation of window size validation is thorough and provides a detailed error message that will help developers quickly identify and fix window size mismatches. This is a significant improvement over the previous silent failure.
64-70: Good options preparationThis method properly separates the concern of preparing screenshot options. The use of AreaCalculator and setting driver options follows the single responsibility principle.
72-81: Improved base screenshot validationThe explicit error raising with a helpful message when base screenshots are missing (and fail_if_new is true) is a good improvement over silent failure.
83-85: Good delegation to Screenshot CoordinatorProperly delegates to the new centralized coordinator, which aligns with the refactoring strategy.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
Dockerfile (1)
11-41: 🛠️ Refactor suggestion
⚠️ Potential issueRemove unnecessary sudo and streamline apt commands
Usingsudoin a Dockerfile is redundant because the default user in aRUNstep is root (DL3004). You can also save space by adding--no-install-recommendsto yourapt-get installand cleaning up the apt lists. Additionally, fix the duplicate/incorrect FFTW package entry (fftw3-devvslibfftw3-dev).Proposed diff:
--- a/Dockerfile +++ b/Dockerfile @@ 11,41c11,42 -RUN --mount=type=cache,target=/var/cache/apt \ - sudo sed -i 's|http://security.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list && \ - sudo apt-get update -qq && \ - sudo apt-get install -qq --fix-missing \ +RUN --mount=type=cache,target=/var/cache/apt \ + sed -i 's|http://security.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list && \ + apt-get update -qq && \ + apt-get install -yqq --no-install-recommends --fix-missing \ @@ - fftw3-dev \ + libfftw3-dev \ @@ - sudo apt-get autoclean + apt-get autoclean && \ + rm -rf /var/lib/apt/lists/*🧰 Tools
🪛 Hadolint (2.12.0)
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
test/integration/browser_screenshot_test.rb (1)
1-245:⚠️ Potential issueAddress persistent test failures in screenshot comparison tests.
The pipeline shows several failing tests despite your changes. Based on the error details, you'll need to address differences in screenshot comparisons across multiple tests.
Given the refactoring of the screenshot comparison framework mentioned in the PR summary, these failures might be due to subtle changes in how screenshots are captured or processed. Consider:
- Adding appropriate tolerance settings to the failing tests, similar to what you did on line 109
- Updating baseline screenshots to match the new rendering behavior
- Investigating if the refactored image comparison components are handling skip_area and crop operations consistently
The failing tests are:
- test_skip_area_by_css_selectors (line 157)
- test_skip_area_accepts_passing_multiple_coordinates_as_one_array (line 103)
- test_crop_and_skip_area_by_css_selectors (line 167)
- test_skip_area_accepts_css_selector (line 132)
- test_crop_accepts_css_selector (line 119)
Most failures show significant color distance (max_color_distance=102.5), which suggests substantial visual differences rather than minor pixel-level variations.
🧰 Tools
🪛 GitHub Actions: Test
[error] 157-157: Test failure: Screenshot difference detected in test_skip_area_by_css_selectors. Difference details: area_size=1, region=[7,91,7,91], max_color_distance=8.0.
[error] 103-103: Test failure: Screenshot difference detected in test_skip_area_accepts_passing_multiple_coordinates_as_one_array. Difference details: area_size=630, region=[7,90,77,99], max_color_distance=102.5.
[error] 167-167: Test failure: Screenshot difference detected in test_crop_and_skip_area_by_css_selectors. Difference details: area_size=4350, region=[0,2,87,52], max_color_distance=102.5.
[error] 132-132: Test failure: Screenshot difference detected in test_skip_area_accepts_css_selector. Multiple differences with area_size=1, region=[7,91,7,91], max_color_distance=8.0 in several related screenshots.
[error] 119-119: Test failure: Screenshot difference detected in test_crop_accepts_css_selector. Difference details: area_size=6264, region=[0,2,87,74], max_color_distance=102.5.
🧹 Nitpick comments (3)
Dockerfile (2)
43-44: Eliminate redundant sudo in directory setup
sudois unnecessary when running as root—simply create and permission the directories directly.Proposed diff:
--- a/Dockerfile +++ b/Dockerfile @@ 43c43 -RUN sudo mkdir -p /bundle /tmp/.X11-unix && \ - sudo chmod a+rwx /bundle /tmp/.X11-unix +RUN mkdir -p /bundle /tmp/.X11-unix && \ + chmod a+rwx /bundle /tmp/.X11-unix🧰 Tools
🪛 Hadolint (2.12.0)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
6-6: Consider pinning the base image for reproducibility
Locking thejetthoughts/cimg-ruby:3.4-chromereference to a digest (SHA256) guards against unexpected upstream changes.test/integration/browser_screenshot_test.rb (1)
19-19: Added flexibility for disabling rollback of comparison files.The addition of the
DISABLE_ROLLBACK_COMPARISON_RUNTIME_FILESenvironment variable provides a more granular way to control the cleanup behavior, allowing developers to preserve comparison files even when not in debug mode.Consider documenting these environment variables in a README or configuration guide to make it clear how they affect test behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (8)
test/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.png
📒 Files selected for processing (5)
.dockerignore(1 hunks)Dockerfile(2 hunks)test/capybara/screenshot/diff_test.rb(8 hunks)test/integration/browser_screenshot_test.rb(3 hunks)test/test_helper.rb(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .dockerignore
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_helper.rb
- test/capybara/screenshot/diff_test.rb
🧰 Additional context used
🪛 GitHub Actions: Test
test/integration/browser_screenshot_test.rb
[error] 157-157: Test failure: Screenshot difference detected in test_skip_area_by_css_selectors. Difference details: area_size=1, region=[7,91,7,91], max_color_distance=8.0.
[error] 103-103: Test failure: Screenshot difference detected in test_skip_area_accepts_passing_multiple_coordinates_as_one_array. Difference details: area_size=630, region=[7,90,77,99], max_color_distance=102.5.
[error] 167-167: Test failure: Screenshot difference detected in test_crop_and_skip_area_by_css_selectors. Difference details: area_size=4350, region=[0,2,87,52], max_color_distance=102.5.
[error] 132-132: Test failure: Screenshot difference detected in test_skip_area_accepts_css_selector. Multiple differences with area_size=1, region=[7,91,7,91], max_color_distance=8.0 in several related screenshots.
[error] 119-119: Test failure: Screenshot difference detected in test_crop_accepts_css_selector. Difference details: area_size=6264, region=[0,2,87,74], max_color_distance=102.5.
🪛 Hadolint (2.12.0)
Dockerfile
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🔇 Additional comments (2)
Dockerfile (1)
46-46: Workdir placement is good
PlacingWORKDIR /appat the end minimizes layer invalidation and groups build steps logically.test/integration/browser_screenshot_test.rb (1)
241-241: Improved error message readability.Adding an extra newline before the error messages list makes the output more readable when multiple failures occur.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and simplifies the test framework integrations by removing legacy modules and introducing a modular, performance-optimized image comparison pipeline with new capture strategies, DSL for naming, and centralized orchestration.
- Removed the old TestMethods integration and replaced it with a coordinator-based capture/assertion flow.
- Added Standard and Stable capture strategies with improved backtrace and stability handling.
- Refactored ImageCompare into a layered pipeline: loader, preprocessor, finder, and reporter.
- Introduced a DSL (
ScreenshotNamerDSL) and.dockerignoreupdates, plus Dockerfile caching optimizations.
Reviewed Changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara/screenshot/diff/test_methods.rb | Removed outdated test framework integration module. |
| lib/capybara/screenshot/diff/standard_capture_strategy.rb | Introduced standard capture strategy class. |
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Enhanced stable screenshot failure to include backtrace. |
| lib/capybara/screenshot/diff/stable_capture_strategy.rb | Added stable capture strategy for page stabilization. |
| lib/capybara/screenshot/diff/screenshoter.rb | Updated image load timeout error to include backtrace. |
| lib/capybara/screenshot/diff/screenshot_namer_dsl.rb | New DSL for screenshot section and group naming. |
| lib/capybara/screenshot/diff/screenshot_matcher.rb | Refactored matcher to use coordinator and assertions. |
| lib/capybara/screenshot/diff/screenshot_coordinator.rb | Centralized capture orchestration via coordinator. |
| lib/capybara/screenshot/diff/reporters/default.rb | Updated reporter to use new difference mask property. |
| lib/capybara/screenshot/diff/image_preprocessor.rb | Added preprocessing filters for skip area and median. |
| lib/capybara/screenshot/diff/image_compare.rb | Refactored performant image comparison pipeline. |
| lib/capybara/screenshot/diff/drivers/base_driver.rb | Added supports? helper for driver capability checks. |
| lib/capybara/screenshot/diff/difference_finder.rb | Created dedicated difference finder class. |
| lib/capybara/screenshot/diff/difference.rb | Expanded Difference struct with metadata accessors. |
| lib/capybara/screenshot/diff/comparison_loader.rb | Introduced loader to build comparison objects. |
| lib/capybara/screenshot/diff/comparison.rb | Extended Comparison with skip_area helper. |
| lib/capybara/screenshot/diff/capture_strategy.rb | Defined abstract base for capture strategies. |
| Dockerfile | Optimized build with mount-based caching and combined steps. |
| .dockerignore | Added ignore patterns for screenshot artifacts. |
Comments suppressed due to low confidence (2)
lib/capybara/screenshot/diff/image_compare.rb:57
- The method
quick_equal?callsdifferent?, butdifferent?is not defined in this class, which will cause a NoMethodError. Consider definingdifferent?or invoking it on a properDifferenceinstance after comparison.
!different?
lib/capybara/screenshot/diff/screenshot_matcher.rb:90
- The variable
screenshot_full_nameis not defined in this scope; it should be passed intocreate_screenshot_assertionor stored on the instance to avoid a NameError at runtime.
screenshot_full_name,
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the visual regression testing framework by modularizing screenshot capture, image comparison, and error reporting components, while also streamlining Docker build configuration.
- Removed legacy test methods in favor of specialized classes.
- Refactored image comparison logic with early file checks and enhanced error reporting.
- Updated Docker and driver configurations for improved setup and maintainability.
Reviewed Changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara/screenshot/diff/test_methods.rb | Removed legacy test methods module. |
| lib/capybara/screenshot/diff/standard_capture_strategy.rb | Introduced default capture strategy using the generic screenshoter. |
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Refactored error raising to instantiate exceptions with caller info. |
| lib/capybara/screenshot/diff/screenshoter.rb | Updated error handling for image loading timeouts. |
| lib/capybara/screenshot/diff/screenshot_matcher.rb | Reworked comparison job building into assertion creation. |
| lib/capybara/screenshot/diff/reporters/default.rb | Updated diff mask access to align with the new Difference interface. |
| lib/capybara/screenshot/diff/* | Multiple modules refactored for image comparison, difference finding, and loading. |
| Dockerfile | Simplified image build configuration and adjusted dependency handling. |
| .dockerignore | Added ignores to reduce build overhead. |
Comments suppressed due to low confidence (3)
Dockerfile:6
- The removal of the '--platform=linux/amd64' flag can affect builds on non-amd64 architectures. Confirm that the target architecture is correctly managed elsewhere in your build configuration.
FROM jetthoughts/cimg-ruby:3.4-chrome
lib/capybara/screenshot/diff/stable_screenshoter.rb:103
- Updating error raising to use the constructor with '.new' and passing 'caller' improves traceability. Ensure that all exception instantiations across the project follow this updated pattern for consistency.
raise CapybaraScreenshotDiff::UnstableImage.new(attempts_reporter.generate, caller)
lib/capybara/screenshot/diff/reporters/default.rb:48
- The updated use of 'difference.diff_mask' instead of 'difference.meta[:diff_mask]' aligns with the new Difference interface. Verify that all parts of the codebase and tests are updated to use this new accessor.
save_heatmap_diff if difference.diff_mask
pftg
force-pushed
the
refact-simplification
branch
from
dba4ecd to
6dd3f9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/capybara/screenshot/diff/difference_finder.rb (1)
41-66: 🛠️ Refactor suggestionInconsistent return type in
callmethodThe
callmethod returns either an array[boolean, Difference]or aDifferenceobject depending onquick_mode, leading to an inconsistent API. Consider unifying the return type or introducing a wrapper to handle both cases cleanly.return ComparisonResult.new(is_equal: false, difference: result) end # Handle identical pixels if driver.same_pixels?(comparison) result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path) return ComparisonResult.new(is_equal: true, difference: result) end # Handle early return for non-tolerable options if quick_mode && without_tolerable_options? return ComparisonResult.new(is_equal: false, difference: nil) end # Process difference region region = driver.find_difference_region(comparison) # Return the result wrapped in ComparisonResult ComparisonResult.new(is_equal: !region.different?, difference: region)🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 41-66: Cyclomatic complexity for
callis too high. [8/7](Metrics/CyclomaticComplexity)
🧹 Nitpick comments (3)
Dockerfile (1)
41-41: Consider removing APT lists to shrink final image.
apt-get autocleanclears retrieved package files but leaves/var/lib/apt/lists. For minimal image size, you may want:RUN apt-get autoclean && rm -rf /var/lib/apt/lists/*This is optional but recommended for production images.
lib/capybara/screenshot/diff/reporters/default.rb (1)
83-85: Enhanced error message with heatmap diffThe error message now includes the heatmap diff path, providing more comprehensive information about available diff images.
annotated_base_image_path.to_path, annotated_image_path.to_path, - heatmap_diff_path.to_path + heatmap_diff_path.to_path,🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/screenshot_matcher.rb (1)
87-93: Add trailing comma to multiline array.According to RuboCop, a trailing comma is missing after the last item in the multiline array.
def create_screenshot_assertion(skip_stack_frames, comparison_options) CapybaraScreenshotDiff::ScreenshotAssertion.from([ caller(skip_stack_frames + 1), screenshot_full_name, - ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options) + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options), ]) end🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (53)
test/fixtures/app/doc/screenshots/linux/cuprite/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.png
📒 Files selected for processing (59)
.dockerignore(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)Dockerfile(2 hunks)bin/dtest(1 hunks)lib/capybara/screenshot/diff/capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/comparison.rb(1 hunks)lib/capybara/screenshot/diff/comparison_loader.rb(1 hunks)lib/capybara/screenshot/diff/difference.rb(2 hunks)lib/capybara/screenshot/diff/difference_finder.rb(1 hunks)lib/capybara/screenshot/diff/drivers/base_driver.rb(1 hunks)lib/capybara/screenshot/diff/drivers/vips_driver.rb(1 hunks)lib/capybara/screenshot/diff/image_compare.rb(3 hunks)lib/capybara/screenshot/diff/image_preprocessor.rb(1 hunks)lib/capybara/screenshot/diff/reporters/default.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_coordinator.rb(1 hunks)lib/capybara/screenshot/diff/screenshot_matcher.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_namer_dsl.rb(1 hunks)lib/capybara/screenshot/diff/screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/stable_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/stable_screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/standard_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/test_methods.rb(0 hunks)lib/capybara_screenshot_diff.rb(1 hunks)lib/capybara_screenshot_diff/backtrace_filter.rb(1 hunks)lib/capybara_screenshot_diff/dsl.rb(1 hunks)lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb(1 hunks)lib/capybara_screenshot_diff/minitest.rb(2 hunks)lib/capybara_screenshot_diff/rspec.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(5 hunks)lib/capybara_screenshot_diff/screenshot_namer.rb(1 hunks)lib/capybara_screenshot_diff/snap.rb(1 hunks)lib/capybara_screenshot_diff/snap_manager.rb(3 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(1 hunks)test/capybara/screenshot/diff/difference_finder_test.rb(1 hunks)test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb(2 hunks)test/capybara/screenshot/diff/drivers/vips_driver_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_refactor_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_test.rb(2 hunks)test/capybara/screenshot/diff/image_preprocessor_test.rb(1 hunks)test/capybara/screenshot/diff/screenshoter_test.rb(1 hunks)test/capybara/screenshot/diff/stable_screenshoter_test.rb(3 hunks)test/capybara/screenshot/diff/test_doubles.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(0 hunks)test/capybara/screenshot/diff_test.rb(8 hunks)test/capybara_screenshot_diff/dsl_test.rb(1 hunks)test/capybara_screenshot_diff/screenshot_namer_test.rb(1 hunks)test/capybara_screenshot_diff/snap_manager_test.rb(1 hunks)test/fixtures/app/index-with-anim.html(1 hunks)test/fixtures/app/index-without-img.html(1 hunks)test/fixtures/app/index.html(1 hunks)test/fixtures/files/rspec_spec.rb(1 hunks)test/integration/browser_screenshot_test.rb(9 hunks)test/integration/test_methods_system_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/support/non_minitest_assertions.rb(1 hunks)test/support/screenshoter_stub.rb(1 hunks)test/support/stub_test_methods.rb(1 hunks)test/test_helper.rb(2 hunks)
💤 Files with no reviewable changes (2)
- test/capybara/screenshot/diff/test_methods_test.rb
- lib/capybara/screenshot/diff/test_methods.rb
✅ Files skipped from review due to trivial changes (3)
- test/capybara/screenshot/diff/screenshoter_test.rb
- test/capybara/screenshot/diff/image_compare_refactor_test.rb
- test/support/capybara_screenshot_diff/dsl_stub.rb
🚧 Files skipped from review as they are similar to previous changes (46)
- .gitignore
- lib/capybara/screenshot/diff/stable_screenshoter.rb
- .dockerignore
- lib/capybara/screenshot/diff/drivers/base_driver.rb
- .github/workflows/test.yml
- lib/capybara/screenshot/diff/drivers/vips_driver.rb
- test/fixtures/app/index-with-anim.html
- lib/capybara_screenshot_diff/minitest.rb
- test/support/non_minitest_assertions.rb
- lib/capybara/screenshot/diff/screenshoter.rb
- test/test_helper.rb
- test/fixtures/files/rspec_spec.rb
- test/integration/test_methods_system_test.rb
- test/capybara/screenshot/diff/drivers/vips_driver_test.rb
- test/capybara_screenshot_diff/snap_manager_test.rb
- lib/capybara/screenshot/diff/comparison.rb
- lib/capybara_screenshot_diff/snap.rb
- test/support/screenshoter_stub.rb
- lib/capybara/screenshot/diff/stable_capture_strategy.rb
- test/fixtures/app/index.html
- lib/capybara/screenshot/diff/screenshot_namer_dsl.rb
- lib/capybara_screenshot_diff/backtrace_filter.rb
- lib/capybara/screenshot/diff/screenshot_coordinator.rb
- test/integration/browser_screenshot_test.rb
- lib/capybara_screenshot_diff/rspec.rb
- lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb
- lib/capybara_screenshot_diff.rb
- lib/capybara/screenshot/diff/standard_capture_strategy.rb
- bin/dtest
- test/capybara/screenshot/diff/stable_screenshoter_test.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- test/fixtures/app/index-without-img.html
- lib/capybara/screenshot/diff/capture_strategy.rb
- test/capybara/screenshot/diff/image_compare_test.rb
- lib/capybara/screenshot/diff/comparison_loader.rb
- test/capybara_screenshot_diff/screenshot_namer_test.rb
- lib/capybara_screenshot_diff/screenshot_namer.rb
- lib/capybara_screenshot_diff/snap_manager.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
- test/capybara/screenshot/diff/image_preprocessor_test.rb
- lib/capybara/screenshot/diff/difference.rb
- lib/capybara_screenshot_diff/screenshot_assertion.rb
- lib/capybara_screenshot_diff/dsl.rb
- lib/capybara/screenshot/diff/image_preprocessor.rb
- test/capybara/screenshot/diff_test.rb
- test/capybara/screenshot/diff/test_doubles.rb
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb (2)
test/capybara/screenshot/diff/image_compare_test.rb (2)
include(15-88)include(90-139)test/support/capybara_screenshot_diff/dsl_stub.rb (1)
create_snapshot_for(55-61)
test/capybara_screenshot_diff/dsl_test.rb (3)
lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
registry(132-134)test/support/capybara_screenshot_diff/dsl_stub.rb (1)
create_snapshot_for(55-61)test/capybara/screenshot/diff/test_doubles.rb (1)
exist?(23-25)
lib/capybara/screenshot/diff/difference_finder.rb (3)
lib/capybara/screenshot/diff/reporters/default.rb (3)
driver(113-115)comparison(117-119)base_image_path(105-107)lib/capybara/screenshot/diff/difference.rb (3)
options(45-47)new_image_path(90-92)build_null(22-31)lib/capybara/screenshot/diff/region.rb (1)
empty?(80-82)
🪛 RuboCop (1.75.5)
test/capybara_screenshot_diff/dsl_test.rb
[fatal] 132-132: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
lib/capybara/screenshot/diff/difference_finder.rb
[convention] 41-66: Cyclomatic complexity for call is too high. [8/7]
(Metrics/CyclomaticComplexity)
lib/capybara/screenshot/diff/reporters/default.rb
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/screenshot_matcher.rb
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🪛 Hadolint (2.12.0)
Dockerfile
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🔇 Additional comments (31)
Dockerfile (4)
6-6: Verify multi-architecture compatibility after removing platform specifier.The explicit
--platform=linux/amd64was dropped from the base image. Ensure this image still builds and runs correctly across your CI environments (e.g., Apple M1 runners).Please confirm that your CI matrix covers both amd64 and arm64 builds, or adjust the build command to pin the appropriate platform if needed.
8-9: Consolidated ENV into a single instruction—good improvement.Grouping
DEBIAN_FRONTENDandBUNDLE_PATHin oneENVreduces image layers and keeps related configuration together.
43-44: Dropsudoand merge directory setup into a single RUN layer.You can remove
sudoand combine this with the previousRUNto reduce layers:
[essential_refactor]- RUN sudo mkdir -p /bundle /tmp/.X11-unix && \ - sudo chmod a+rwx /bundle /tmp/.X11-unix + RUN mkdir -p /bundle /tmp/.X11-unix && \ + chmod a+rwx /bundle /tmp/.X11-unix🧰 Tools
🪛 Hadolint (2.12.0)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
46-46: WORKDIR relocation is appropriate.Moving
WORKDIR /appto the end ensures all prerequisite setup happens before changing context. This aligns with best practices for clarity.lib/capybara/screenshot/diff/reporters/default.rb (3)
48-48: Updated attribute access patternThe code now accesses
difference.diff_maskdirectly instead of the previousdifference.meta[:diff_mask]. This aligns with the refactoredDifferenceclass that provides direct accessors for attributes.
53-53: Updated skip area access through comparison objectThe skip area is now accessed through
difference.comparison.skip_areainstead of directly from the difference object, properly reflecting the new object hierarchy.
92-92: Updated direct attribute accessSimilar to line 48, the code now accesses
difference.diff_maskdirectly instead of using the meta hash, maintaining consistency with the updatedDifferenceclass.test/support/stub_test_methods.rb (1)
4-4: Well-structured DSL stub replacementGood job replacing the previous
TestMethodsStubmodule with a more focused require statement that loads the newCapybaraScreenshotDiff::DSLStub. This change aligns with the broader refactoring to modularize test support utilities.test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb (2)
12-12: Updated to use the new DSL stubThe test now uses
CapybaraScreenshotDiff::DSLStubinstead of the oldTestMethodsStub, aligning with the refactored test support utilities.
149-151: Improved snapshot creation and comparisonThe
make_comparisonmethod now explicitly creates a snapshot usingcreate_snapshot_forand instantiatesImageComparedirectly with the snapshot's paths. This is more transparent and easier to understand than the previous implementation that relied on superclass delegation.lib/capybara/screenshot/diff/difference_finder.rb (3)
6-22: Well-documented class with clear responsibilitiesExcellent documentation that explains the purpose and approach of the
DifferenceFinderclass. The layered comparison approach is well-described, making it clear how this class fits into the overall architecture.
23-23: Clear definition of tolerable optionsGood job defining the tolerable options as a frozen constant, making it clear which options can affect the tolerance of the comparison.
70-72: Efficient check for tolerable optionsGood implementation of
without_tolerable_options?using array intersection to check if any tolerable options are present in the options hash.lib/capybara/screenshot/diff/screenshot_matcher.rb (6)
9-9: Great addition of the screenshot coordinator dependency.The new
screenshot_coordinatormodule will help centralize screenshot capture strategies, improving modularity and making the code more maintainable.
25-39: Excellent refactoring of the screenshot assertion process.The renamed method now has a clear, step-by-step structure with appropriate early returns and error handling. The addition of the
skip_stack_framesparameter will help provide more accurate error locations in test failures.
43-45: Good extraction of the comparison check logic.This helper method improves readability by clearly expressing the intent of checking if comparison is needed based on base screenshot existence.
47-62: Improved error handling for window size mismatches.This explicit window size check with a detailed error message is much better than the previous silent return behavior. The error includes both expected and actual window sizes, making it easier to diagnose the issue.
72-81: Enhanced error handling for missing base screenshots.The explicit error raising with a helpful message is a significant improvement over silent return behavior, making debugging easier when
fail_if_newis enabled.
83-85: Good delegation to the screenshot coordinator.Delegating screenshot capture to the new coordinator improves separation of concerns and aligns with the architectural changes in this PR.
test/capybara_screenshot_diff/dsl_test.rb (5)
12-20: Good test for missing screenshot error handling.This test properly validates the new explicit error raising behavior when a screenshot is missing and
fail_if_newis enabled.
22-33: Comprehensive test for assert_image_not_changed.This test thoroughly validates the error message formatting for image differences, including handling Ruby version differences correctly.
52-55: Good test for driver options support.The test appropriately skips if the vips driver is not available, making it more robust across different environments.
61-80: Excellent test for stack frame skipping.This test validates that the
skip_stack_framesparameter correctly influences which caller information is recorded in assertions, ensuring proper error reporting in test failures.
82-100: Thorough testing of inline screenshot validation.These tests cover both success and failure cases for inline screenshot assertions, ensuring the feature works correctly in both scenarios.
lib/capybara/screenshot/diff/image_compare.rb (7)
3-10: Good modularization of the image comparison system.Adding these new dependencies reflects the improved modularization of responsibilities into focused classes for image loading, preprocessing, and difference detection.
17-30: Excellent documentation of the optimization strategy.The expanded documentation clearly explains the layered optimization approach, which will help future developers understand the performance benefits and design rationale.
48-58: Breaking change: quick_equal? now raises exceptions.The refactored
quick_equal?method now raises exceptions when files don't exist instead of returning false as before. This could impact consumers expecting a boolean result.Consider preserving the original behavior to maintain backward compatibility:
def quick_equal? - ensure_files_exist! + begin + ensure_files_exist! + rescue ArgumentError + return false + end # Quick file size check - if sizes are equal, perform a simple file comparison if base_image_path.size == image_path.size # If we have identical files (same size and content), we can return true immediately # without more expensive comparison return true if files_identical?(base_image_path, image_path) end !different? end
60-63: Good centralization of file existence checking.Creating a dedicated method for checking file existence improves code organization and provides clearer error messages.
91-101: Good use of memoization for collaborator classes.These helper methods follow the principle of lazy initialization, improving performance by only creating instances when needed.
103-112: Well-structured find_difference method.The method has a clear flow: validate images exist, load and preprocess images, then delegate to the difference finder. The
quick_modeparameter provides flexibility in the comparison process.
148-155: Great performance improvement using FileUtils.identical?Replacing the previous implementation that loaded entire files into memory with
FileUtils.identical?significantly reduces memory usage and improves performance for large images.
pftg
force-pushed
the
refact-simplification
branch
3 times, most recently
from
9fd5512 to
646b6b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
Dockerfile (2)
11-14:⚠️ Potential issueRemove unnecessary
sudoin RUN instructions.Docker build steps run as root by default, so
sudois redundant and may lead to permission inconsistencies.RUN --mount=type=cache,target=/var/cache/apt \ - sudo sed -i 's|http://security.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list && \ - sudo apt-get update -qq && \ - sudo apt-get install -qq --fix-missing \ + sed -i 's|http://security.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list && \ + apt-get update -qq && \ + apt-get install -qq --fix-missing \🧰 Tools
🪛 Hadolint (2.12.0)
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
41-41: 🛠️ Refactor suggestionAdd cleanup for apt lists to reduce image size.
After
apt-get autoclean, it's recommended to remove package lists to reduce the final image size.- sudo apt-get autoclean + sudo apt-get autoclean && \ + sudo rm -rf /var/lib/apt/lists/*lib/capybara/screenshot/diff/difference_finder.rb (1)
57-59:⚠️ Potential issueInconsistent return value for early exit.
When returning early due to non-tolerable options, you return
[false, nil], but for dimension differences (line 47), you return[false, result]. This inconsistency could lead to bugs if callers expect a non-nil difference object.- return [false, nil] + result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path, {quick_exit: true}) + return [false, result]test/capybara_screenshot_diff/dsl_test.rb (1)
131-133: Syntax error: splat without a parameter name
def assert_image_not_changed(*)is invalid syntax in Ruby ≥ 2.4 and breaks the test file.
A named splat keeps the same semantics while being valid:-def assert_image_not_changed(*) - CapybaraScreenshotDiff::ScreenshotAssertion.assert_image_not_changed(*) +def assert_image_not_changed(*args) + CapybaraScreenshotDiff::ScreenshotAssertion.assert_image_not_changed(*args) end🧰 Tools
🪛 RuboCop (1.75.5)
[fatal] 132-132: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure usingTargetRubyVersionparameter, underAllCops)(Lint/Syntax)
🧹 Nitpick comments (5)
lib/capybara/screenshot/diff/reporters/default.rb (1)
83-84: Add a trailing comma after the last array item.The multiline array is missing a trailing comma after the last item, which is inconsistent with Ruby style conventions.
image_path.to_path, annotated_base_image_path.to_path, annotated_image_path.to_path, - heatmap_diff_path.to_path + heatmap_diff_path.to_path,🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/difference_finder.rb (2)
41-66: Consider refactoring the complexcallmethod.The
callmethod has high cyclomatic complexity. Consider extracting the dimension check, pixel check, and region analysis into separate private methods to improve readability and maintainability.def call(comparison, quick_mode: true) - # Process the comparison and return result + # Check dimensions first (fast fail) + result = check_dimensions(comparison) + return format_result(false, result, quick_mode) if result + # Check for identical pixels + result = check_identical_pixels(comparison) + return format_result(true, result, quick_mode) if result + # Handle early return for non-tolerable options + if quick_mode && without_tolerable_options? + result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path, {quick_exit: true}) + return [false, result] + end + # Process difference region + region = driver.find_difference_region(comparison) + format_result(!region.different?, region, quick_mode) +end +private +def check_dimensions(comparison) + return nil if driver.same_dimension?(comparison) + build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path, {different_dimensions: true}) +end +def check_identical_pixels(comparison) + return nil unless driver.same_pixels?(comparison) + build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path) +end +def format_result(is_equal, result, quick_mode) + quick_mode ? [is_equal, result] : result +end🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 41-66: Cyclomatic complexity for
callis too high. [8/7](Metrics/CyclomaticComplexity)
41-41: Improve method documentation.The documentation could be clearer about the return value structure, especially with the conditional return type based on
quick_mode.-# @return [Array, Difference] Either [is_equal, difference] or a Difference object depending on mode +# @return [Array<Boolean, Difference>, Difference] +# When quick_mode is true: Returns [is_equal, difference] where is_equal is a boolean indicating equality +# When quick_mode is false: Returns just the Difference object🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 41-66: Cyclomatic complexity for
callis too high. [8/7](Metrics/CyclomaticComplexity)
lib/capybara/screenshot/diff/screenshot_matcher.rb (1)
88-92: Add trailing comma to satisfy RuboCop and avoid diff noiseRuboCop (
Style/TrailingCommaInArrayLiteral) flagged the missing comma after the last array element. Adding it also makes future diffs cleaner.- ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options) + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options),🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/image_compare.rb (1)
39-49: Duplicate file-existence validation
ensure_files_exist!is invoked ininitializeand again inquick_equal?, raising the same errors twice.
Dropping the second call avoids redundant work and keeps the method focused on its purpose.Also applies to: 60-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (75)
test/fixtures/app/doc/screenshots/linux/cuprite/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.png
📒 Files selected for processing (59)
.dockerignore(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)Dockerfile(2 hunks)bin/dtest(1 hunks)lib/capybara/screenshot/diff/capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/comparison.rb(1 hunks)lib/capybara/screenshot/diff/comparison_loader.rb(1 hunks)lib/capybara/screenshot/diff/difference.rb(2 hunks)lib/capybara/screenshot/diff/difference_finder.rb(1 hunks)lib/capybara/screenshot/diff/drivers/base_driver.rb(1 hunks)lib/capybara/screenshot/diff/drivers/vips_driver.rb(1 hunks)lib/capybara/screenshot/diff/image_compare.rb(3 hunks)lib/capybara/screenshot/diff/image_preprocessor.rb(1 hunks)lib/capybara/screenshot/diff/reporters/default.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_coordinator.rb(1 hunks)lib/capybara/screenshot/diff/screenshot_matcher.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_namer_dsl.rb(1 hunks)lib/capybara/screenshot/diff/screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/stable_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/stable_screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/standard_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/test_methods.rb(0 hunks)lib/capybara_screenshot_diff.rb(1 hunks)lib/capybara_screenshot_diff/backtrace_filter.rb(1 hunks)lib/capybara_screenshot_diff/dsl.rb(1 hunks)lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb(1 hunks)lib/capybara_screenshot_diff/minitest.rb(2 hunks)lib/capybara_screenshot_diff/rspec.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(5 hunks)lib/capybara_screenshot_diff/screenshot_namer.rb(1 hunks)lib/capybara_screenshot_diff/snap.rb(1 hunks)lib/capybara_screenshot_diff/snap_manager.rb(3 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(1 hunks)test/capybara/screenshot/diff/difference_finder_test.rb(1 hunks)test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb(2 hunks)test/capybara/screenshot/diff/drivers/vips_driver_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_refactor_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_test.rb(2 hunks)test/capybara/screenshot/diff/image_preprocessor_test.rb(1 hunks)test/capybara/screenshot/diff/screenshoter_test.rb(1 hunks)test/capybara/screenshot/diff/stable_screenshoter_test.rb(3 hunks)test/capybara/screenshot/diff/test_doubles.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(0 hunks)test/capybara/screenshot/diff_test.rb(8 hunks)test/capybara_screenshot_diff/dsl_test.rb(1 hunks)test/capybara_screenshot_diff/screenshot_namer_test.rb(1 hunks)test/capybara_screenshot_diff/snap_manager_test.rb(1 hunks)test/fixtures/app/index-with-anim.html(1 hunks)test/fixtures/app/index-without-img.html(1 hunks)test/fixtures/app/index.html(1 hunks)test/fixtures/files/rspec_spec.rb(1 hunks)test/integration/browser_screenshot_test.rb(9 hunks)test/integration/test_methods_system_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/support/non_minitest_assertions.rb(1 hunks)test/support/screenshoter_stub.rb(1 hunks)test/support/stub_test_methods.rb(1 hunks)test/test_helper.rb(2 hunks)
💤 Files with no reviewable changes (2)
- lib/capybara/screenshot/diff/test_methods.rb
- test/capybara/screenshot/diff/test_methods_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/capybara/screenshot/diff/screenshoter_test.rb
🚧 Files skipped from review as they are similar to previous changes (49)
- lib/capybara/screenshot/diff/drivers/base_driver.rb
- .github/workflows/test.yml
- test/capybara/screenshot/diff/stable_screenshoter_test.rb
- test/fixtures/app/index-with-anim.html
- .dockerignore
- lib/capybara/screenshot/diff/stable_screenshoter.rb
- lib/capybara/screenshot/diff/drivers/vips_driver.rb
- lib/capybara_screenshot_diff/minitest.rb
- test/support/non_minitest_assertions.rb
- .gitignore
- lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb
- lib/capybara/screenshot/diff/screenshoter.rb
- bin/dtest
- test/capybara/screenshot/diff/drivers/vips_driver_test.rb
- test/integration/test_methods_system_test.rb
- test/test_helper.rb
- lib/capybara/screenshot/diff/comparison.rb
- lib/capybara_screenshot_diff/snap_manager.rb
- lib/capybara_screenshot_diff.rb
- lib/capybara/screenshot/diff/comparison_loader.rb
- lib/capybara_screenshot_diff/snap.rb
- lib/capybara_screenshot_diff/rspec.rb
- test/support/screenshoter_stub.rb
- test/fixtures/files/rspec_spec.rb
- test/capybara/screenshot/diff/image_compare_test.rb
- lib/capybara/screenshot/diff/stable_capture_strategy.rb
- test/fixtures/app/index-without-img.html
- lib/capybara/screenshot/diff/standard_capture_strategy.rb
- lib/capybara/screenshot/diff/capture_strategy.rb
- lib/capybara/screenshot/diff/screenshot_namer_dsl.rb
- test/integration/browser_screenshot_test.rb
- test/capybara_screenshot_diff/snap_manager_test.rb
- lib/capybara_screenshot_diff/backtrace_filter.rb
- test/capybara/screenshot/diff/image_preprocessor_test.rb
- lib/capybara_screenshot_diff/screenshot_assertion.rb
- lib/capybara/screenshot/diff/screenshot_coordinator.rb
- lib/capybara/screenshot/diff/difference.rb
- test/fixtures/app/index.html
- test/capybara/screenshot/diff/image_compare_refactor_test.rb
- lib/capybara/screenshot/diff/image_preprocessor.rb
- test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb
- lib/capybara_screenshot_diff/screenshot_namer.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- test/capybara_screenshot_diff/screenshot_namer_test.rb
- lib/capybara_screenshot_diff/dsl.rb
- test/support/capybara_screenshot_diff/dsl_stub.rb
- test/capybara/screenshot/diff_test.rb
- test/capybara/screenshot/diff/test_doubles.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/capybara/screenshot/diff/screenshot_matcher.rb (8)
lib/capybara_screenshot_diff/dsl.rb (1)
build_screenshot_assertion(61-65)lib/capybara/screenshot/diff/browser_helpers.rb (3)
window_size_is_wrong?(26-29)selenium?(22-24)session(95-97)test/integration/browser_screenshot_test.rb (1)
window_size(254-260)lib/capybara/screenshot/diff/area_calculator.rb (2)
calculate_crop(12-19)calculate_skip_area(23-44)lib/capybara/screenshot/diff/drivers.rb (1)
for(7-12)lib/capybara_screenshot_diff/snap.rb (1)
checkout_base_screenshot(22-24)lib/capybara/screenshot/diff/screenshot_coordinator.rb (1)
capture(23-27)lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
from(17-25)
lib/capybara/screenshot/diff/difference_finder.rb (3)
lib/capybara/screenshot/diff/reporters/default.rb (3)
driver(113-115)comparison(117-119)base_image_path(105-107)lib/capybara/screenshot/diff/difference.rb (3)
options(45-47)new_image_path(90-92)build_null(22-31)lib/capybara/screenshot/diff/region.rb (1)
empty?(80-82)
🪛 RuboCop (1.75.5)
lib/capybara/screenshot/diff/screenshot_matcher.rb
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/capybara_screenshot_diff/dsl_test.rb
[fatal] 132-132: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
lib/capybara/screenshot/diff/difference_finder.rb
[convention] 41-66: Cyclomatic complexity for call is too high. [8/7]
(Metrics/CyclomaticComplexity)
lib/capybara/screenshot/diff/reporters/default.rb
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🪛 Hadolint (2.12.0)
Dockerfile
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🔇 Additional comments (7)
lib/capybara/screenshot/diff/reporters/default.rb (2)
48-48: LGTM! Direct attribute access improves code clarity.The code now directly accesses
difference.diff_maskanddifference.comparison.skip_areainstead of going through the meta hash, which improves readability and encapsulation.Also applies to: 53-53, 92-92
83-84: Enhanced error reporting with additional file paths.Adding both the annotated image and heatmap diff paths to the error message improves debugging by providing more context about the differences.
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
Dockerfile (2)
8-9: LGTM! Clean ENV variable grouping.Combining related environment variables into a single ENV instruction reduces layers and improves readability.
11-11: Good use of cache mount for apt.Using
--mount=type=cachefor apt improves build performance by caching package downloads between builds.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
test/support/stub_test_methods.rb (1)
4-4: LGTM! Simplified test support with centralized DSL.The change to require a central DSL stub module instead of maintaining multiple test methods reduces duplication and improves maintainability.
lib/capybara/screenshot/diff/difference_finder.rb (1)
3-22: LGTM! Well-structured class with clear responsibilities.The new DifferenceFinder class follows the Single Responsibility Principle with a clear focus on analyzing image comparisons. The documentation clearly explains its role in the layered comparison approach.
test/capybara_screenshot_diff/dsl_test.rb (1)
52-55: Verify constant name for VIPS driver availability
Capybara::Screenshot::Diff::Drivers::VipsDriverTestlooks unusual.
The driver class elsewhere is generallyVips. Please confirm that this constant exists; otherwise the test will be skipped even when support is present.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integration/record_screenshot_test.rb(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
test/integration/record_screenshot_test.rb
[error] 31-31: Test failure in RecordScreenshotTest#test_record_index_as_webp: Screenshot does not match for 'record_screenshot/record_index_as_webp/00_index-vips' with area size 7844.0.
[error] 15-15: Test failure in RecordScreenshotTest#test_record_index: Screenshot does not match for 'record_screenshot/record_index/00_index' with area size 7665 and max color distance 101.1.
[error] 22-22: Test failure in RecordScreenshotTest#test_record_index_cropped: Screenshot does not match for 'record_screenshot/record_index_cropped/00_index-cropped' with area size 7665 and max color distance 101.1.
[error] 38-38: Test failure in RecordScreenshotTest#test_record_index_with_stability: Screenshot does not match for 'record_screenshot/record_index_with_stability/00_index' with area size 7665 and max color distance 101.1.
pftg
force-pushed
the
refact-simplification
branch
from
4b788d3 to
e161cf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Dockerfile (1)
46-47: Remove redundantsudoin directory setup
The build user in Docker is root, sosudois unnecessary.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 46-46: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🧹 Nitpick comments (1)
bin/dtest (1)
3-3: Error handling approach changed.The script now uses
set -o pipefailinstead ofset -eo pipefail, removing the-eflag which would cause immediate exit on any command failure. This allows the script to continue execution even if some tests fail with one driver, ensuring all drivers are tested.Consider adding a comment explaining why immediate exit on error was removed, or adding error code collection to provide a non-zero exit code if any driver's tests fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (44)
test/fixtures/app/doc/screenshots/linux/cuprite/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.png
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)Dockerfile(2 hunks)bin/dtest(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🪛 Hadolint (2.12.0)
Dockerfile
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 46-46: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🔇 Additional comments (7)
bin/dtest (4)
7-11: Good practice: Whitelisting environment variables for Docker.Creating an explicit whitelist of environment variables to forward to Docker containers improves security and clarity. This ensures only necessary configuration is passed to the containers.
13-19: Clean implementation of environment variable forwarding.The implementation correctly builds Docker environment arguments only for variables that are actually set, avoiding empty
-eflags. The variable expansion syntax${!var}is a good use of bash's indirect variable references.
24-25: Setup failure handling improved.Wrapping the setup command in a subshell with an explicit exit ensures the script terminates if setup fails, which is critical before proceeding to run tests.
27-37:Details
❓ Verification inconclusive
Improved test execution with driver loop.
Replacing multiple separate Docker commands with a loop over drivers significantly improves maintainability. Adding or removing drivers now only requires changing the
DRIVERSarray.The script now runs tests both in Docker and locally for each driver, which provides better coverage but might increase test duration.
Verify that running tests both in Docker and locally for each driver is intentional and not duplicative work:
🏁 Script executed:
#!/bin/bash # Check if there are any comments or documentation explaining the purpose of running tests both in Docker and locally grep -r "run.*both.*docker.*local\|run.*test.*twice" --include="*.md" --include="*.txt" .Length of output: 91
Please confirm intentional duplication of test runs
I didn’t find any documentation or comments explaining why we run the same tests both inside Docker and locally for each driver. This may be intentional for increased coverage, but could also unnecessarily double execution time.
• File: bin/dtest Lines 27–37
• Action: Verify whether runningbin/rake testtwice per driver (once in Docker and once locally) is desired, or if one context can be removed to avoid duplicate work.Dockerfile (3)
6-6: Verify removal of explicit platform specification
Removing--platform=linux/amd64may affect multi-arch builds. Ensure the default platform aligns with your CI/CD environment or reinstate the flag if needed.
8-9: Consolidated ENV instruction is good
CombiningDEBIAN_FRONTENDandBUNDLE_PATHinto a singleENVreduces image layers.
49-49: WORKDIR moved after setup steps
SettingWORKDIR /appafter creating directories ensures the working directory exists.
pftg
force-pushed
the
refact-simplification
branch
2 times, most recently
from
02c1878 to
c4b1e5d
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/integration/browser_screenshot_test.rb (3)
120-120:⚠️ Potential issueAdd tolerance parameter to fix failing test
The pipeline failures show this test is failing with a difference level of 0.4745. Adding a tolerance parameter would help make this test more stable.
- assert_matches_screenshot("index-without-img-cropped", crop: "form", color_distance_limit: 40) + assert_matches_screenshot("index-without-img-cropped", crop: "form", color_distance_limit: 40, tolerance: 0.48)🧰 Tools
🪛 GitHub Actions: Test
[error] 120-120: Screenshot difference detected in test_compare_crops_only_when_other_part_is_not_working with difference level 0.4745.
130-130:⚠️ Potential issueAdd tolerance parameter to fix failing test
The pipeline failures show that
test_crop_accepts_css_selectoris failing with a difference level of 0.4790625. Adding a tolerance parameter would help make this test more stable.- assert_matches_screenshot("index-without-img-cropped", crop: "form") + assert_matches_screenshot("index-without-img-cropped", crop: "form", tolerance: 0.48)
201-201:⚠️ Potential issueAdd tolerance parameter to fix failing test
The pipeline failures show that
test_crop_and_skip_area_by_css_selectorsis failing with a difference level of 0.3346875. Adding a tolerance parameter would help make this test more stable.- assert_matches_screenshot("index-without-img-cropped", skip_area: "input", crop: "form") + assert_matches_screenshot("index-without-img-cropped", skip_area: "input", crop: "form", tolerance: 0.34)
🧹 Nitpick comments (4)
test/integration/browser_screenshot_test.rb (2)
37-39: Complete the skip messageThe skip message "we record screenshots only in" appears to be incomplete. Consider providing a more descriptive message that explains why the test is skipped during screenshot recording.
- skip "we record screenshots only in" + skip "Test skipped when recording screenshots to avoid interference with the recording process"
102-104: Consistent skip messages across testsThe skip message "we record screenshots only in" appears in multiple places and is incomplete in all instances. Consider using a consistent and complete message across all tests.
- skip "we record screenshots only in" + skip "Test skipped when recording screenshots to avoid interference with the recording process"Also applies to: 126-129, 143-146, 164-166, 182-184, 194-196
Dockerfile (2)
43-43: Merge font-antialias tweak into the same RUN
EachRUNcreates a new layer; you can append thesedstep to the apt-install block to reduce layers and leverage the same cache mount.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
46-47: Avoid scatteredsudoby switching to root user
The CircleCI Ruby image defaults to a non-root user, which is whysudois sprinkled everywhere. You can simplify by:USER root RUN --mount=type=cache,target=/var/cache/apt \ … (install + cleanup + font config) … USER circleciThen drop all
sudoprefixes.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 46-46: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🛑 Comments failed to post (1)
Dockerfile (1)
11-14:
⚠️ Potential issueNon-interactive install flags & apt lists cleanup
- You need
-y(or--assume-yes) onapt-get installto avoid hanging for confirmation.- Consider adding
--no-install-recommendsto trim unnecessary packages.- After
apt-get autoclean, remove/var/lib/apt/lists/*to shrink the final image.Apply something like:
RUN --mount=type=cache,target=/var/cache/apt \ sudo sed -i 's|http://security.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list && \ sudo apt-get update -qq && \ - sudo apt-get install -qq --fix-missing \ + sudo apt-get install -qq -y --no-install-recommends --fix-missing \ automake \ build-essential \ … \ swig && \ - sudo apt-get autoclean + sudo apt-get autoclean && \ + rm -rf /var/lib/apt/lists/*Also applies to: 41-41
🧰 Tools
🪛 Hadolint (2.12.0)
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🤖 Prompt for AI Agents
In Dockerfile lines 11 to 14 and line 41, the apt-get install command lacks the -y flag to prevent interactive prompts and should include --no-install-recommends to avoid installing unnecessary packages. Additionally, after running apt-get autoclean, add a command to remove all files in /var/lib/apt/lists/ to reduce image size. Update the RUN commands accordingly to include these flags and cleanup steps.
There was a problem hiding this comment.
Pull Request Overview
Refactors and streamlines the screenshot diff integration by introducing modular capture strategies, decoupling comparison logic into specialized classes, and adding a DSL for naming and grouping screenshots.
- Add StableCaptureStrategy and ScreenshotCoordinator for unified, pluggable capture flows
- Refactor ScreenshotMatcher to leverage coordinator, assertions, and modular components
- Restructure image comparison pipeline (ImageCompare, DifferenceFinder, ComparisonLoader, ImagePreprocessor) for performance and maintainability
Reviewed Changes
Copilot reviewed 145 out of 145 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Include caller info in UnstableImage exception |
| lib/capybara/screenshot/diff/stable_capture_strategy.rb | Added StableCaptureStrategy class |
| lib/capybara/screenshot/diff/screenshoter.rb | Updated ExpectationNotMet initialization with backtrace |
| lib/capybara/screenshot/diff/screenshot_namer_dsl.rb | Introduced DSL for screenshot naming sections/groups |
| lib/capybara/screenshot/diff/screenshot_matcher.rb | Refactored matcher to use ScreenshotCoordinator and assertions |
| lib/capybara/screenshot/diff/screenshot_coordinator.rb | Introduced ScreenshotCoordinator module |
| lib/capybara/screenshot/diff/reporters/default.rb | Adapted diff_mask access and included heatmap in error message |
| lib/capybara/screenshot/diff/image_preprocessor.rb | Added ImagePreprocessor for masking and filtering |
| lib/capybara/screenshot/diff/image_compare.rb | Refactored ImageCompare with layered early file checks |
| lib/capybara/screenshot/diff/drivers/base_driver.rb | Added supports? helper |
| lib/capybara/screenshot/diff/difference_finder.rb | Introduced DifferenceFinder class |
| lib/capybara/screenshot/diff/difference.rb | Enhanced Difference value object structure |
| lib/capybara/screenshot/diff/comparison_loader.rb | Added ComparisonLoader for image loading |
| lib/capybara/screenshot/diff/comparison.rb | Added skip_area accessor to Comparison |
| lib/capybara/screenshot/diff/capture_strategy.rb | Added CaptureStrategy abstract base class |
| Dockerfile | Improved build caching, env handling, and cleanup |
| .github/workflows/test.yml | Set SCREENSHOT_DRIVER env for CI |
| .github/actions/setup-ruby-and-dependencies/action.yml | Updated font anti-alias configuration |
| .dockerignore | Added patterns to ignore generated images and temp files |
Comments suppressed due to low confidence (2)
lib/capybara/screenshot/diff/stable_capture_strategy.rb:17
- New capture strategy
StableCaptureStrategyis introduced without corresponding tests. Consider adding unit tests to verify that it delegates toStableScreenshotercorrectly.
def take_comparison_screenshot(snapshot)
lib/capybara/screenshot/diff/screenshot_coordinator.rb:23
- The new
ScreenshotCoordinator.captureorchestration is critical to the flow but lacks direct tests. Adding tests for strategy selection and snap initialization would improve confidence.
def capture(snap_or_name, capture_options, comparison_options)
cf9123b to
ee765a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
capybara-screenshot-diff.gemspec (1)
26-27:Details
❓ Verification inconclusive
Verify the dependency scope change and fix alphabetical ordering.
Two important considerations:
Dependency scope change: Moving activesupport and actionpack from runtime to development dependencies is a significant change. Please confirm this is correct - users of the gem will no longer have these as transitive dependencies.
Alphabetical ordering: As flagged by RuboCop, dependencies should be alphabetically ordered.
Apply this diff to fix the alphabetical ordering:
- spec.add_development_dependency "activesupport", ">= 7.0", "< 9" - spec.add_development_dependency "actionpack", ">= 7.0", "< 9" + spec.add_development_dependency "actionpack", ">= 7.0", "< 9" + spec.add_development_dependency "activesupport", ">= 7.0", "< 9"The broader version constraints (">= 7.0", "< 9") provide good flexibility for compatibility.
🏁 Script executed:
#!/bin/bash # Description: Verify if activesupport/actionpack were previously runtime dependencies # Expected: Evidence of previous dependency declarations echo "Checking git history for previous dependency declarations:" git log --oneline -p -- capybara-screenshot-diff.gemspec | grep -A 5 -B 5 "add.*dependency.*activesupport\|add.*dependency.*actionpack" | head -20Length of output: 126
Verify dependency scope change and fix alphabetical ordering
I couldn’t find any evidence in the git history that
activesupportoractionpackwere ever declared as runtime dependencies, so please confirm that moving them to development-only is intentional—this will remove them as transitive dependencies for gem consumers.Also, per RuboCop’s guidance, dependencies should be listed in alphabetical order. Please apply the following change in
capybara-screenshot-diff.gemspec:- spec.add_development_dependency "activesupport", ">= 7.0", "< 9" - spec.add_development_dependency "actionpack", ">= 7.0", "< 9" + spec.add_development_dependency "actionpack", ">= 7.0", "< 9" + spec.add_development_dependency "activesupport", ">= 7.0", "< 9"• File: capybara-screenshot-diff.gemspec
• Lines: 26–27Please verify the scope change and update the ordering accordingly.
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 27-27: Dependencies should be sorted in an alphabetical order within their section of the gemspec. Dependency
actionpackshould appear beforeactivesupport.(Gemspec/OrderedDependencies)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
capybara-screenshot-diff.gemspec(1 hunks)gemfiles/rails70_gems.rb(1 hunks)gemfiles/rails71_gems.rb(1 hunks)gemfiles/rails80_gems.rb(1 hunks)test/support/setup_rails_app.rb(1 hunks)test/test_helper.rb(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/support/setup_rails_app.rb
- gemfiles/rails80_gems.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_helper.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
capybara-screenshot-diff.gemspec
[convention] 27-27: Dependencies should be sorted in an alphabetical order within their section of the gemspec. Dependency actionpack should appear before activesupport.
(Gemspec/OrderedDependencies)
🔇 Additional comments (2)
gemfiles/rails70_gems.rb (1)
7-7: LGTM! Good modular dependency management.The addition of activesupport with specific requires is a good practice that:
- Only loads necessary components (deprecator and test_case)
- Makes dependencies explicit and clear
- Aligns with the PR objective of simplifying test framework integrations
gemfiles/rails71_gems.rb (1)
6-7:Details
❓ Verification inconclusive
Verify consistency of requires across Rails versions.
The modular dependency approach with explicit requires is excellent. However, there's a difference in activesupport requires between Rails 7.0 and 7.1 gemfiles:
- Rails 7.0:
%w[active_support/deprecator active_support/test_case]- Rails 7.1:
%w[logger active_support/deprecator active_support]Please ensure this difference is intentional and reflects actual Rails version requirements.
🏁 Script executed:
#!/bin/bash # Description: Check if the difference in activesupport requires is consistent across all Rails gemfiles # Expected: Consistent pattern or documented reason for differences echo "Checking activesupport requires across all Rails gemfiles:" rg -A 1 'gem "activesupport"' gemfiles/Length of output: 667
Ensure activesupport require directives match Rails version behavior
We ran a quick check across your Rails gemfiles:
- rails70_gems.rb
gem "activesupport", "~> 7.0.0", require: %w[active_support/deprecator active_support/test_case]- rails71_gems.rb
gem "activesupport", "~> 7.1.0", require: %w[logger active_support/deprecator active_support]- rails80_gems.rb
gem "activesupport", "~> 8.0.0" (no explicitrequire:)Please verify that:
- Dropping
active_support/test_casein 7.1 is intentional (it may have been folded into the core or moved elsewhere).- Adding
loggerandactive_supportin 7.1 accurately reflects Rails 7.1’s default load path.- Omitting all
require:flags in 8.0 aligns with Rails 8.0’s built-in loading behavior.If these differences mirror official Rails changes, consider adding a brief comment explaining the rationale; otherwise adjust the requires for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/capybara_screenshot_diff/dsl_test.rb (1)
131-133: Splat operator syntax is now correct.The previous syntax error with the splat operator has been properly fixed. The method now correctly accepts
*argsand passes them to the underlying assertion method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/test.yml(5 hunks).standard.yml(1 hunks)test/capybara/screenshot/diff_test.rb(8 hunks)test/capybara_screenshot_diff/dsl_test.rb(1 hunks)test/integration/browser_screenshot_test.rb(10 hunks)test/integration/record_screenshot_test.rb(1 hunks)test/test_helper.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .standard.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- test/test_helper.rb
- .github/workflows/test.yml
- test/integration/browser_screenshot_test.rb
- test/integration/record_screenshot_test.rb
- test/capybara/screenshot/diff_test.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
test/capybara_screenshot_diff/dsl_test.rb
[convention] 61-80: Assignment Branch Condition size for test_skip_stack_frames is too high. [<1, 33, 0> 33.02/23]
(Metrics/AbcSize)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Ruby & YAML
🔇 Additional comments (10)
test/capybara_screenshot_diff/dsl_test.rb (10)
1-11: LGTM! Clean file structure and module setup.The file header, requires, and class setup are well-organized. The inclusion of both DSL and DSLStub modules properly sets up the test environment for comprehensive testing of the refactored DSL functionality.
12-20: LGTM! Proper error testing with stubbing.The test correctly verifies that an error is raised when a screenshot is missing and
fail_if_newis true. The use of stubbing to control VCS and configuration states is appropriate for isolated testing.
22-33: LGTM! Comprehensive message format testing.The test properly validates the error message format and handles Ruby version differences correctly. The expected message format is clear and includes all necessary file paths and location information.
35-50: LGTM! Good coverage of shift distance limit functionality.This test effectively covers the shift distance limit feature with proper assertions and Ruby version compatibility handling.
52-55: LGTM! Appropriate conditional testing for driver support.The test correctly skips when the vips driver is not available, ensuring compatibility across different environments.
82-91: LGTM! Good inline assertion testing with difference detection.The test correctly verifies that an exception is raised when differences are detected during inline assertion validation.
93-100: LGTM! Proper testing of no-difference scenario.The test appropriately verifies that no exception is raised when no differences are detected during inline assertion validation.
102-104: LGTM! Concise testing of skip area and stability options.The test efficiently covers the skip_area and stability_time_limit functionality.
106-111: LGTM! Proper verification of screenshot creation.The test correctly verifies that new screenshots are created and saved to the expected location.
113-123: LGTM! Good cleanup testing for both scenarios.These tests properly verify that base images are cleaned up after assertions, covering both scenarios where changes are detected and where they aren't.
pftg
force-pushed
the
refact-simplification
branch
from
72abfd1 to
056575e
Compare
There was a problem hiding this comment.
Pull Request Overview
A refactor to modularize and simplify the screenshot diff comparison pipeline and update related infrastructure.
- Extracted preprocessing, loading, and difference‐finding into dedicated classes (
ImagePreprocessor,ComparisonLoader,DifferenceFinder) - Updated
ImageCompareto use a layered strategy with early file checks and new modules - Adjusted reporting, difference value object, and CI/docker setups to support new workflows and drivers
Reviewed Changes
Copilot reviewed 152 out of 152 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara/screenshot/diff/image_preprocessor.rb | New class to apply skip‐areas and median filters |
| lib/capybara/screenshot/diff/image_compare.rb | Refactored comparison class to use loader/preprocessor |
| lib/capybara/screenshot/diff/difference_finder.rb | New class for layered difference analysis |
| lib/capybara/screenshot/diff/difference.rb | Extended Difference struct with path aliases and mask |
| lib/capybara/screenshot/diff/comparison_loader.rb | New loader for raw image acquisition |
| capybara-screenshot-diff.gemspec | Changed ActionPack/ActiveSupport to development deps |
| Dockerfile | Simplified layering, cache mount, and font config tweak |
| .github/workflows/test.yml | Expanded JRuby matrix and added driver env var |
Comments suppressed due to low confidence (3)
lib/capybara/screenshot/diff/difference.rb:94
- [nitpick] The method
original_image_pathaliasesbase_image_pathbut uses a different naming convention thannew_image_path. Consider renaming tobase_image_pathor adding a matching alias for clarity.
def original_image_path
lib/capybara/screenshot/diff/capture_strategy.rb:19
- The abstract
CaptureStrategyand its#take_comparison_screenshotmethod don’t appear to have unit tests. Consider adding tests for concrete strategy implementations and error scenarios.
def take_comparison_screenshot(_snapshot)
capybara-screenshot-diff.gemspec:26
- Moving ActionPack from a runtime to a development dependency will break consumers at runtime. It should remain a runtime_dependency if the code relies on ActionPack during execution.
spec.add_development_dependency "actionpack", ">= 7.0", "< 9"
|
@UweKubosch, please check. I'm going to merge this tomorrow. So far, all tests have passed: |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the image comparison and screenshot assertion framework to simplify test integrations and improve performance and maintainability. Key changes include updating the reporter to use new accessor methods for diff data, introducing dedicated classes for image preprocessing and comparison loading, and revising gem dependencies, Dockerfile, and CI workflows.
Reviewed Changes
Copilot reviewed 152 out of 152 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/capybara/screenshot/diff/reporters/default.rb | Refactored diff mask and skip area accessor usage for improved consistency. |
| lib/capybara/screenshot/diff/image_preprocessor.rb | Introduced a new image preprocessor to handle filtering and skip-area operations. |
| lib/capybara/screenshot/diff/image_compare.rb | Enhanced file existence checks and streamlined detailed comparison logic. |
| lib/capybara/screenshot/diff/drivers/base_driver.rb | Added a supports? method to verify feature availability on drivers. |
| lib/capybara/screenshot/diff/difference_finder.rb | Implemented a layered approach to determine image differences with tolerance support. |
| lib/capybara/screenshot/diff/difference.rb | Updated the Difference struct with path accessors and consistent diff_mask accessor. |
| lib/capybara/screenshot/diff/comparison_loader.rb | Added a helper to load and prepare Comparison objects with preprocessing applied. |
| lib/capybara/screenshot/diff/comparison.rb | Enhanced Comparison with a dedicated skip_area getter. |
| lib/capybara/screenshot/diff/capture_strategy.rb | Introduced an abstract strategy class to standardize screenshot capture. |
| Gemfiles & capybara-screenshot-diff.gemspec | Adjusted gem dependencies and grouping (runtime vs. development). |
| Dockerfile & CI workflow files (.github/workflows/test.yml, etc.) | Updated build configurations and environment settings for improved reliability. |
Comments suppressed due to low confidence (3)
lib/capybara/screenshot/diff/reporters/default.rb:53
- Ensure that using difference.comparison.skip_area is reliable; verify that the comparison object always contains the expected skip_area data.
image = annotate_skip_areas(image, difference.comparison.skip_area) if difference.comparison.skip_area
Dockerfile:6
- Removing the explicit --platform flag may unintentionally build on non-amd64 architectures; confirm that targeting multiple platforms is acceptable.
FROM jetthoughts/cimg-ruby:3.4-chrome
capybara-screenshot-diff.gemspec:26
- Changing 'actionpack' from a runtime dependency to a development dependency may affect gem consumers; please verify if this adjustment is intentional.
spec.add_development_dependency "actionpack", ">= 7.0", "< 9"
| save_annotation_for(new_image, annotated_image_path) | ||
| save_annotation_for(base_image, annotated_base_image_path) | ||
| save_heatmap_diff if difference.meta[:diff_mask] | ||
| save_heatmap_diff if difference.diff_mask |
There was a problem hiding this comment.
Refactor usage of the diff mask accessor for consistency; using difference.diff_mask directly avoids accessing the meta hash.
| rescue | ||
| # If there's any error reading the files, they're not identical |
There was a problem hiding this comment.
[nitpick] Consider handling exceptions explicitly in files_identical? instead of silently swallowing errors for improved debuggability.
| rescue | |
| # If there's any error reading the files, they're not identical | |
| rescue Errno::ENOENT, Errno::EACCES => e | |
| # Log the error and return false | |
| warn "Error comparing files #{file1} and #{file2}: #{e.message}" |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Dockerfile (1)
11-41: 🛠️ Refactor suggestionEnsure proper cleanup of apt lists for smaller image size.
You’ve dropped therm -rf /var/lib/apt/lists/*step—without it, package lists remain and bloat the image. It’s recommended to follow upapt-get autocleanwith removal of/var/lib/apt/lists/*, for example:RUN --mount=type=cache,target=/var/cache/apt \ sed -i 's|http://security.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list && \ apt-get update -qq && \ apt-get install -qq --fix-missing \ ...packages... && \ apt-get autoclean && \ + rm -rf /var/lib/apt/lists/*🧰 Tools
🪛 Hadolint (2.12.0)
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
♻️ Duplicate comments (6)
Dockerfile (3)
11-41: Remove unnecessarysudoin RUN block.
All commands in a DockerfileRUNstep execute as root—sudois redundant and can be removed throughout this block.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
43-43: Remove unnecessarysudoin sed invocation.
Docker build steps run as root by default; dropsudofor consistency and to satisfy hadolint DL3004.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
46-47: Remove unnecessarysudoand consider merging layers.
Since these commands run as root by default, dropsudoand optionally combine with the previousRUNto reduce image layers:-RUN sudo mkdir -p /bundle /tmp/.X11-unix && \ - sudo chmod 1777 /bundle /tmp/.X11-unix +RUN mkdir -p /bundle /tmp/.X11-unix && \ + chmod 1777 /bundle /tmp/.X11-unix🧰 Tools
🪛 Hadolint (2.12.0)
[error] 46-46: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
lib/capybara/screenshot/diff/difference_finder.rb (1)
78-79:⚠️ Potential issueFix inconsistent return value for early exit.
When returning early due to non-tolerable options, you return
[false, nil], but for dimension differences (line 67), you return[false, result]. This inconsistency could lead to bugs if callers expect a non-nil difference object.- return [false, nil] + result = build_null_difference(comparison, comparison.base_image_path, comparison.new_image_path, {quick_exit: true}) + return [false, result]lib/capybara/screenshot/diff/screenshot_matcher.rb (1)
87-93:⚠️ Potential issueFix the off-by-one error in caller stack slicing
The method adds +1 to skip_stack_frames when calling caller, but build_screenshot_assertion already incremented it. This causes two stack frames to be skipped instead of one, potentially hiding the real caller in error reports.
Apply this diff to fix the off-by-one error and add a trailing comma for consistency:
def create_screenshot_assertion(skip_stack_frames, comparison_options) CapybaraScreenshotDiff::ScreenshotAssertion.from([ - caller(skip_stack_frames + 1), + caller(skip_stack_frames), screenshot_full_name, - ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options) + ImageCompare.new(@snapshot.path, @snapshot.base_path, comparison_options), ]) end🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/image_compare.rb (1)
65-77: Address breaking behavior change inquick_equal?.The method now calls
ensure_files_exist!which raisesArgumentErrorfor missing files, changing the previous behavior where missing files would likely returnfalse. This aligns with a past review comment concern about preserving boolean return behavior.Consider preserving the original boolean behavior:
def quick_equal? - ensure_files_exist! + return false unless images_exist? # Quick file size check - if sizes are equal, perform a simple file comparison if base_image_path.size == image_path.size
🧹 Nitpick comments (3)
Dockerfile (1)
43-43: Refine sed pattern to avoid unintended replacements.
A globals/true/false/gmay affect unrelated “true” strings. Narrow the match to the specific XML key, e.g.:RUN sed -i '/<key>antialias<\/key>/s/true/false/' /etc/fonts/conf.d/10-antialias.conf🧰 Tools
🪛 Hadolint (2.12.0)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
lib/capybara/screenshot/diff/reporters/default.rb (1)
83-85: Add trailing comma for consistency.The multiline array should have a trailing comma after the last element to maintain consistency with Ruby style guidelines.
annotated_base_image_path.to_path, annotated_image_path.to_path, - heatmap_diff_path.to_path + heatmap_diff_path.to_path,🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/difference_finder.rb (1)
61-86: Consider reducing cyclomatic complexity.The
callmethod has high cyclomatic complexity (8/7). While the logic is clear, consider extracting some conditional logic into private methods for improved maintainability.For example, you could extract the early return logic:
def call(comparison, quick_mode: true) + return handle_early_checks(comparison, quick_mode) if should_return_early?(comparison, quick_mode) + + # Process difference region + region = driver.find_difference_region(comparison) + quick_mode ? [!region.different?, region] : region +end + +private + +def should_return_early?(comparison, quick_mode) + !driver.same_dimension?(comparison) || + driver.same_pixels?(comparison) || + (quick_mode && without_tolerable_options?) +end + +def handle_early_checks(comparison, quick_mode) + # Existing early return logic here🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 61-86: Cyclomatic complexity for
callis too high. [8/7](Metrics/CyclomaticComplexity)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (78)
test/fixtures/app/doc/screenshots/linux/cuprite/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/cuprite/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_chrome_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/linux/selenium_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/cuprite/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_chrome_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/cropped_screenshot.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-blur_active_element-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-blur_active_element-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-hide_caret-disabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-hide_caret-enabled.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index-without-img-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index_with_skip_area_as_array_of_css.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/index_with_skip_area_as_array_of_css_and_p.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/record_screenshot/record_index/00_index.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/record_screenshot/record_index_cropped/00_index-cropped.pngis excluded by!**/*.pngtest/fixtures/app/doc/screenshots/macos/selenium_headless/record_screenshot/record_index_with_stability/00_index.pngis excluded by!**/*.png
📒 Files selected for processing (68)
.dockerignore(1 hunks).github/actions/setup-ruby-and-dependencies/action.yml(1 hunks).github/workflows/test.yml(5 hunks).gitignore(1 hunks).standard.yml(1 hunks)Dockerfile(2 hunks)bin/dtest(1 hunks)capybara-screenshot-diff.gemspec(1 hunks)gemfiles/edge_gems.rb(1 hunks)gemfiles/rails70_gems.rb(1 hunks)gemfiles/rails71_gems.rb(1 hunks)gemfiles/rails80_gems.rb(1 hunks)lib/capybara/screenshot/diff/capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/comparison.rb(1 hunks)lib/capybara/screenshot/diff/comparison_loader.rb(1 hunks)lib/capybara/screenshot/diff/difference.rb(2 hunks)lib/capybara/screenshot/diff/difference_finder.rb(1 hunks)lib/capybara/screenshot/diff/drivers/base_driver.rb(1 hunks)lib/capybara/screenshot/diff/drivers/vips_driver.rb(1 hunks)lib/capybara/screenshot/diff/image_compare.rb(3 hunks)lib/capybara/screenshot/diff/image_preprocessor.rb(1 hunks)lib/capybara/screenshot/diff/reporters/default.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_coordinator.rb(1 hunks)lib/capybara/screenshot/diff/screenshot_matcher.rb(2 hunks)lib/capybara/screenshot/diff/screenshot_namer_dsl.rb(1 hunks)lib/capybara/screenshot/diff/screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/stable_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/stable_screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/standard_capture_strategy.rb(1 hunks)lib/capybara/screenshot/diff/test_methods.rb(0 hunks)lib/capybara_screenshot_diff.rb(1 hunks)lib/capybara_screenshot_diff/backtrace_filter.rb(1 hunks)lib/capybara_screenshot_diff/dsl.rb(1 hunks)lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb(1 hunks)lib/capybara_screenshot_diff/minitest.rb(2 hunks)lib/capybara_screenshot_diff/rspec.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(5 hunks)lib/capybara_screenshot_diff/screenshot_namer.rb(1 hunks)lib/capybara_screenshot_diff/snap.rb(1 hunks)lib/capybara_screenshot_diff/snap_manager.rb(3 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(1 hunks)test/capybara/screenshot/diff/difference_finder_test.rb(1 hunks)test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb(2 hunks)test/capybara/screenshot/diff/drivers/vips_driver_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_refactor_test.rb(1 hunks)test/capybara/screenshot/diff/image_compare_test.rb(2 hunks)test/capybara/screenshot/diff/image_preprocessor_test.rb(1 hunks)test/capybara/screenshot/diff/screenshoter_test.rb(1 hunks)test/capybara/screenshot/diff/stable_screenshoter_test.rb(3 hunks)test/capybara/screenshot/diff/test_doubles.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(0 hunks)test/capybara/screenshot/diff_test.rb(8 hunks)test/capybara_screenshot_diff/dsl_test.rb(1 hunks)test/capybara_screenshot_diff/screenshot_namer_test.rb(1 hunks)test/capybara_screenshot_diff/snap_manager_test.rb(1 hunks)test/fixtures/app/index-with-anim.html(1 hunks)test/fixtures/app/index-without-img.html(1 hunks)test/fixtures/app/index.html(1 hunks)test/fixtures/files/rspec_spec.rb(1 hunks)test/integration/browser_screenshot_test.rb(10 hunks)test/integration/record_screenshot_test.rb(1 hunks)test/integration/test_methods_system_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/support/non_minitest_assertions.rb(1 hunks)test/support/screenshoter_stub.rb(1 hunks)test/support/setup_rails_app.rb(1 hunks)test/support/stub_test_methods.rb(1 hunks)test/test_helper.rb(1 hunks)
💤 Files with no reviewable changes (2)
- test/capybara/screenshot/diff/test_methods_test.rb
- lib/capybara/screenshot/diff/test_methods.rb
✅ Files skipped from review due to trivial changes (2)
- lib/capybara/screenshot/diff/comparison.rb
- gemfiles/rails70_gems.rb
🚧 Files skipped from review as they are similar to previous changes (55)
- .gitignore
- test/support/setup_rails_app.rb
- .standard.yml
- .github/actions/setup-ruby-and-dependencies/action.yml
- lib/capybara/screenshot/diff/drivers/vips_driver.rb
- test/fixtures/files/rspec_spec.rb
- test/fixtures/app/index-with-anim.html
- gemfiles/rails80_gems.rb
- lib/capybara/screenshot/diff/drivers/base_driver.rb
- gemfiles/edge_gems.rb
- gemfiles/rails71_gems.rb
- lib/capybara/screenshot/diff/screenshoter.rb
- test/support/screenshoter_stub.rb
- test/capybara/screenshot/diff/stable_screenshoter_test.rb
- lib/capybara_screenshot_diff/minitest.rb
- test/support/non_minitest_assertions.rb
- lib/capybara/screenshot/diff/stable_screenshoter.rb
- capybara-screenshot-diff.gemspec
- test/integration/test_methods_system_test.rb
- .dockerignore
- lib/capybara_screenshot_diff/rspec.rb
- lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb
- test/capybara_screenshot_diff/snap_manager_test.rb
- lib/capybara/screenshot/diff/stable_capture_strategy.rb
- test/test_helper.rb
- test/capybara/screenshot/diff/screenshoter_test.rb
- lib/capybara_screenshot_diff/snap.rb
- lib/capybara_screenshot_diff.rb
- test/capybara/screenshot/diff/drivers/vips_driver_test.rb
- test/fixtures/app/index-without-img.html
- test/fixtures/app/index.html
- lib/capybara/screenshot/diff/capture_strategy.rb
- test/capybara/screenshot/diff/image_compare_test.rb
- .github/workflows/test.yml
- lib/capybara/screenshot/diff/standard_capture_strategy.rb
- lib/capybara/screenshot/diff/screenshot_namer_dsl.rb
- lib/capybara_screenshot_diff/screenshot_assertion.rb
- bin/dtest
- lib/capybara_screenshot_diff/snap_manager.rb
- test/integration/record_screenshot_test.rb
- lib/capybara_screenshot_diff/backtrace_filter.rb
- test/capybara/screenshot/diff/image_preprocessor_test.rb
- lib/capybara/screenshot/diff/comparison_loader.rb
- test/integration/browser_screenshot_test.rb
- lib/capybara/screenshot/diff/image_preprocessor.rb
- test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- lib/capybara/screenshot/diff/difference.rb
- test/capybara/screenshot/diff/image_compare_refactor_test.rb
- test/capybara_screenshot_diff/screenshot_namer_test.rb
- lib/capybara/screenshot/diff/screenshot_coordinator.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
- test/capybara/screenshot/diff_test.rb
- test/support/capybara_screenshot_diff/dsl_stub.rb
- test/capybara/screenshot/diff/test_doubles.rb
🧰 Additional context used
🧬 Code Graph Analysis (3)
lib/capybara/screenshot/diff/difference_finder.rb (3)
lib/capybara/screenshot/diff/image_compare.rb (4)
attr_reader(39-194)initialize(44-52)build_null_difference(173-175)different?(95-97)lib/capybara/screenshot/diff/difference.rb (3)
options(45-47)new_image_path(90-92)build_null(22-31)lib/capybara/screenshot/diff/region.rb (1)
empty?(80-82)
test/capybara_screenshot_diff/dsl_test.rb (3)
lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
registry(132-134)test/support/capybara_screenshot_diff/dsl_stub.rb (1)
create_snapshot_for(55-61)test/capybara/screenshot/diff/test_doubles.rb (1)
exist?(23-25)
lib/capybara/screenshot/diff/image_compare.rb (6)
lib/capybara/screenshot/diff/difference.rb (3)
options(45-47)new(21-101)build_null(22-31)lib/capybara/screenshot/diff/drivers.rb (1)
for(7-12)lib/capybara/screenshot/diff/reporters/default.rb (5)
base_image_path(105-107)image_path(109-111)generate(17-30)driver(113-115)comparison(117-119)lib/capybara/screenshot/diff/comparison.rb (1)
new(4-8)scripts/benchmark/find_region_benchmark.rb (1)
new(10-52)lib/capybara/screenshot/diff/difference_finder.rb (3)
build_null_difference(95-97)call(61-86)freeze(31-98)
🪛 RuboCop (1.75.5)
lib/capybara/screenshot/diff/difference_finder.rb
[convention] 61-86: Cyclomatic complexity for call is too high. [8/7]
(Metrics/CyclomaticComplexity)
lib/capybara/screenshot/diff/screenshot_matcher.rb
[convention] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/reporters/default.rb
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🪛 Hadolint (2.12.0)
Dockerfile
[error] 11-11: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
[error] 46-46: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🔇 Additional comments (31)
Dockerfile (4)
6-6: Verify removal of explicit platform specification.
Removing--platform=linux/amd64simplifies theFROMinstruction but may impact multi-arch build behavior. Please confirm that the base imagejetthoughts/cimg-ruby:3.4-chromecovers all intended target architectures in your CI/CD pipelines.
8-9: Good consolidation of ENV instructions.
CombiningDEBIAN_FRONTENDandBUNDLE_PATHinto a singleENVreduces layers and is more readable.
43-43: Verify font configuration file consistency.
The CI action modifies/etc/fonts/conf.d/10-yes-antialias.confwhile the Dockerfile targets10-antialias.conf. Please confirm that these paths refer to the same file or update for consistency.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 43-43: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
49-49: WORKDIR placement is appropriate.
PositioningWORKDIR /appafter setup steps is clear and consistent with best practices.lib/capybara/screenshot/diff/reporters/default.rb (1)
48-48: LGTM! Attribute access pattern updates look correct.The updates to use
difference.diff_maskdirectly and accessskip_areathroughdifference.comparison.skip_areaproperly align with the new class structure mentioned in the AI summary.Also applies to: 53-53, 92-92
test/support/stub_test_methods.rb (1)
4-4: LGTM! Module replacement aligns with refactoring objectives.The replacement of the old test helper module with the new DSL stub module is consistent with the broader refactoring effort to modularize and improve the test framework integrations.
lib/capybara_screenshot_diff/screenshot_namer.rb (1)
42-51: LGTM! Screenshot naming logic is well-implemented.The counter-based naming with zero-padding and hierarchical directory structure provides good organization for screenshot files. The conditional counter logic correctly handles grouped vs non-grouped scenarios.
lib/capybara_screenshot_diff/dsl.rb (3)
11-19: Well-structured module documentation!The module documentation clearly explains the purpose and integration points of the DSL. The layered optimization strategy mentioned in the method documentation provides good context for maintainers.
21-66: Excellent implementation of the screenshot method!The method provides:
- Clear parameter documentation with all available options
- Proper early returns for edge cases
- Clean separation between assertion building and validation
- Consistent return value (true) on success
The layered optimization strategy and error handling are well-implemented.
74-89: Clean delegation pattern!The private helper method properly encapsulates the assertion building logic and correctly adjusts skip_stack_frames for accurate error reporting.
lib/capybara/screenshot/diff/screenshot_matcher.rb (4)
25-39: Well-structured assertion building method!The method properly separates concerns:
- Window size validation
- Screenshot preparation
- Base screenshot verification
- Screenshot capture
- Assertion creation
The early return when comparison isn't needed is a good optimization.
47-62: Excellent window size validation!The method provides clear error messages with both expected and actual window sizes, making debugging easier. The conditional handling for Selenium vs other drivers is appropriate.
72-81: Clear error handling for missing base screenshots!The explicit error with instructions on how to disable the check (
Capybara::Screenshot::Diff.fail_if_new=false) is helpful for users.
83-85: Clean delegation to ScreenshotCoordinator!Good separation of concerns by delegating screenshot capture to a specialized coordinator module.
test/capybara_screenshot_diff/dsl_test.rb (6)
12-20: Good test coverage for missing screenshot scenario!The test properly verifies that an
ExpectationNotMeterror is raised with the correct message whenfail_if_newis enabled and no base screenshot exists.
22-50: Comprehensive assertion message testing!The tests properly verify message formatting for different scenarios and handle Ruby version differences in floating-point representation. Good coverage of both basic comparison and shift distance limit cases.
61-88: Well-refactored stack frame tests!The tests have been properly split into focused methods as previously suggested. Each test clearly verifies the stack frame skipping behavior with appropriate assertions on caller information.
90-108: Thorough inline validation testing!The tests properly cover both failure and success cases for inline screenshot assertions, with appropriate use of
assert_raisesandassert_nothing_raised.
110-131: Good coverage of DSL features!The tests verify:
- Option passing (skip_area, stability_time_limit)
- Screenshot file creation
- Proper cleanup of base images in both change and no-change scenarios
All test scenarios are well-covered.
135-141: Clean helper method implementations!The helper methods properly wrap the DSL methods for testing purposes. The splat operator syntax issue mentioned in past reviews has been correctly addressed.
lib/capybara/screenshot/diff/image_compare.rb (11)
3-4: LGTM: Required dependencies added.The addition of
pathnameandfileutilsrequirements supports the new file comparison and path handling functionality used throughout the refactored class.
7-10: LGTM: Modular architecture dependencies.The new requires support the refactored architecture that delegates responsibilities to specialized classes. This follows the single responsibility principle and improves maintainability.
17-38: Excellent documentation of optimization strategy.The comprehensive documentation clearly explains the multi-layered approach and performance considerations. This will help future maintainers understand the design decisions and optimization trade-offs.
68-71: LGTM: Efficient file comparison implementation.The file size check followed by
files_identical?provides an optimal fast path for identical files, addressing previous concerns about memory usage in file comparison.
74-77: LGTM: Improved quick comparison strategy.Using
find_difference(quick_mode: true)addresses the previous concern aboutquick_equal?triggering expensive full comparisons. The quick mode provides lightweight comparison as intended.
119-129: LGTM: Clean dependency injection pattern.The lazy initialization of collaborator objects follows good design patterns and supports the modular architecture. Each collaborator has a specific responsibility which improves testability and maintainability.
131-140: Well-structured difference finding logic.The method cleanly delegates to the modular components and handles the dual return format for quick mode appropriately. The early validation and clear separation of concerns is excellent.
187-193: Good choice of file comparison method.Using
FileUtils.identical?addresses the previous memory usage concerns by avoiding loading entire files into memory. The rescue block provides graceful handling of file system errors.Note: This addresses the past review suggestion to use FileUtils methods instead of
File.readfor binary file comparison.
168-171: LGTM: Clear separation of loading and preprocessing.The delegation to
comparison_loaderandimage_preprocessorfollows the single responsibility principle and makes the code more maintainable and testable.
153-167: Excellent method documentation.The comprehensive documentation clearly explains the method's responsibilities, parameters, and return value. This level of detail will help future maintainers understand the preprocessing pipeline.
48-48:Details
✅ Verification successful
Verify early validation impact on existing usage.
Adding
ensure_files_exist!in the initializer changes the behavior to fail fast when files don't exist, rather than deferring validation to when comparison methods are called. This could be a breaking change for existing code that expects to construct ImageCompare objects with potentially missing files.
🏁 Script executed:
#!/bin/bash # Search for ImageCompare instantiation patterns to assess impact rg -A 5 -B 2 "ImageCompare\.new" --type rubyLength of output: 4815
🏁 Script executed:
#!/bin/bash # Show first 200 lines of ImageCompare to inspect initializer and ensure_files_exist! placement sed -n '1,200p' lib/capybara/screenshot/diff/image_compare.rbLength of output: 7517
🏁 Script executed:
#!/bin/bash # Show around the instantiation in attempts_reporter.rb to inspect how missing files are handled sed -n '1,200p' lib/capybara_screenshot_diff/attempts_reporter.rbLength of output: 1645
🏁 Script executed:
#!/bin/bash # Display the contents of stable_screenshoter.rb to inspect snapshot.prev_attempt_path usage sed -n '1,200p' lib/capybara/screenshot/diff/stable_screenshoter.rbLength of output: 4894
🏁 Script executed:
#!/bin/bash # Display the contents of screenshot_matcher to see how ImageCompare is used sed -n '1,200p' lib/capybara/screenshot/diff/screenshot_matcher.rbLength of output: 4337
No breaking changes detected from early file validation
I’ve reviewed all internal
ImageCompare.newusages—scripts, tests, DSL stubs, reporters, stable screenshoter, and matcher—and every instantiation only occurs when both image files are known to exist. Since callers already guard against missing files (or skip comparison when the base doesn’t exist), addingensure_files_exist!in the initializer simply surfaces errors sooner without altering any existing control flow.
| # Clears the directory for the current screenshot group. | ||
| # This is typically used when starting a new group to remove old screenshots. | ||
| def clear_current_group_directory | ||
| dir_to_clear = current_group_directory | ||
| FileUtils.rm_rf(dir_to_clear) if Dir.exist?(dir_to_clear) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the destructive nature of directory clearing.
The clear_current_group_directory method removes entire directories, which is potentially destructive. Consider adding safety checks or more explicit documentation about this behavior.
# Clears the directory for the current screenshot group.
- # This is typically used when starting a new group to remove old screenshots.
+ # WARNING: This removes the entire directory and all its contents.
+ # This is typically used when starting a new group to remove old screenshots.
+ # Only files within the screenshot area are affected.
def clear_current_group_directory
dir_to_clear = current_group_directory
+ # Safety check: only clear if within screenshot area
+ return unless dir_to_clear.start_with?(screenshot_area)
FileUtils.rm_rf(dir_to_clear) if Dir.exist?(dir_to_clear)
endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/capybara_screenshot_diff/screenshot_namer.rb around lines 75 to 80, the
clear_current_group_directory method deletes the entire directory for the
current screenshot group, which is destructive. Add explicit documentation in
the method comment warning about this destructive behavior and suggest adding
safety checks if appropriate, such as confirming the directory path or requiring
explicit confirmation before deletion.
| def screenshot_area | ||
| @screenshot_area ||= Capybara::Screenshot.screenshot_area | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Capybara::Screenshot.screenshot_area availability.
The fallback to Capybara::Screenshot.screenshot_area assumes this class and method exist. Ensure this dependency is available in all contexts where ScreenshotNamer is used.
🏁 Script executed:
#!/bin/bash
# Description: Verify that Capybara::Screenshot.screenshot_area is available
# Expected: Find the method definition or usage pattern
# Search for Capybara::Screenshot class definition
ast-grep --pattern 'module Capybara
$$$
module Screenshot
$$$
end
$$$
end'
# Search for screenshot_area method definition
rg -A 3 "def screenshot_area"
# Search for existing usage of Capybara::Screenshot.screenshot_area
rg "Capybara::Screenshot\.screenshot_area"Length of output: 1236
🏁 Script executed:
#!/bin/bash
# Verify dependency declarations and require statements for Capybara::Screenshot
echo "== gemspec dependencies =="
fd --extension gemspec --max-depth 1 |
xargs -I {} grep -n "add_dependency" {}
echo -e "\n== gemspec capybara references =="
fd --extension gemspec --max-depth 1 |
xargs -I {} grep -n "capybara" {}
echo -e "\n== Gemfile references (if present) =="
if [ -f Gemfile ]; then grep -n "capybara-screenshot" Gemfile || echo "None"; else echo "No Gemfile"; fi
echo -e "\n== Require statements in codebase =="
rg "require.*capybara"
rg "require.*screenshot"Length of output: 13893
Add explicit capybara-screenshot dependency and require
The fallback to Capybara::Screenshot.screenshot_area relies on the external capybara-screenshot gem, but we currently only declare a dependency on capybara and never require 'capybara/screenshot'. Without this, Capybara::Screenshot may be undefined at runtime.
• In capybara-screenshot-diff.gemspec
- Add a runtime dependency on
capybara-screenshot, e.g.:
spec.add_runtime_dependency 'capybara-screenshot', '>= 1.0'
• In your lib entrypoint (e.g. lib/capybara_screenshot_diff.rb) or at the top of
lib/capybara_screenshot_diff/screenshot_namer.rb
- Add:
require 'capybara/screenshot'
This ensures Capybara::Screenshot.screenshot_area is loaded and available for the fallback.
🤖 Prompt for AI Agents
In lib/capybara_screenshot_diff/screenshot_namer.rb around lines 21 to 23, the
code uses Capybara::Screenshot.screenshot_area without explicitly requiring the
capybara-screenshot gem, which may cause runtime errors if the constant is
undefined. To fix this, add a runtime dependency on 'capybara-screenshot' in
capybara-screenshot-diff.gemspec by including spec.add_runtime_dependency
'capybara-screenshot', '>= 1.0'. Also, add require 'capybara/screenshot' at the
top of lib/capybara_screenshot_diff/screenshot_namer.rb or the main lib
entrypoint file to ensure the module is loaded before use.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
.gitignoreto exclude the/.windsurfdirectory..dockerignoreto exclude unnecessary files from Docker builds.