test: parallelize tests #143
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a comprehensive refactor of the screenshot comparison and assertion infrastructure. It adds new modular classes for image preprocessing, comparison, and difference detection, and replaces the legacy test methods with a DSL-based approach. The assertion registry, naming, and error handling mechanisms are overhauled, and test coverage is expanded and reorganized, including new unit tests and helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant DSL as CapybaraScreenshotDiff::DSL
participant Namer as ScreenshotNamer
participant Matcher as ScreenshotMatcher
participant Coordinator as ScreenshotCoordinator
participant Strategy as CaptureStrategy
participant Loader as ComparisonLoader
participant Preprocessor as ImagePreprocessor
participant Finder as DifferenceFinder
Test->>DSL: screenshot(name, options)
DSL->>Namer: full_name(name)
DSL->>Matcher: build_screenshot_assertion(name, options)
Matcher->>Coordinator: capture(snap, capture_options, comparison_options)
Coordinator->>Strategy: take_comparison_screenshot(snap)
Strategy->>Loader: call(base_path, new_path, options)
Loader->>Preprocessor: process_comparison(comparison)
Preprocessor-->>Loader: preprocessed images
Loader-->>Strategy: Comparison object
Strategy-->>Coordinator: Screenshot taken
Coordinator-->>Matcher: Screenshot result
Matcher->>Finder: call(comparison, quick_mode)
Finder-->>Matcher: Difference object
Matcher-->>DSL: ScreenshotAssertion
DSL-->>Test: Boolean result or raises error
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (18)
lib/capybara/screenshot/diff/reporters/default.rb (1)
83-85: Consider adding trailing comma for consistency.The addition of
heatmap_diff_path.to_pathto the error message improves debugging capabilities by including the heatmap diff location.Consider adding a trailing comma for consistency with Ruby style guidelines:
[ "(#{difference.inspect})", 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, ].join(NEW_LINE)🧰 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/test_vcs.rb (2)
14-14: Remove debug print statement.The
ppstatement appears to be leftover debug code that should be removed before merging to production.- pp [root, screenshot_path, checkout_path]
18-18: Simplify file existence check.The condition
fixture_file && File.exist?(fixture_file)is redundant sinceFile.exist?handles nil gracefully.- if fixture_file && File.exist?(fixture_file) + if File.exist?(fixture_file)test/unit/snap_manager_test.rb (1)
56-62: Verify the test name matches its behavior.The test is named "overrides the file extension" but it doesn't actually test overriding - it tests the default PNG extension behavior. The test verifies proper file naming conventions including attempt path generation.
Consider renaming the test to better reflect what it actually tests:
-test "#snapshot overrides the file extension" do +test "#snapshot creates paths with PNG extension and proper naming" dolib/capybara_screenshot_diff/backtrace_filter.rb (1)
15-18: Consider anchoring the regex pattern to avoid false positives.The regex pattern
/gems\/(activesupport|minitest|railties)/could inadvertently filter out legitimate application code if the path contains these strings. For example, a project directory named "minitest-app" or a file path containing "gems/activesupport-extensions" would be filtered.Consider using a more specific pattern:
- .reject { |l| l =~ /gems\/(activesupport|minitest|railties)/ } + .reject { |l| l =~ /\/gems\/(activesupport|minitest|railties)-[\d.]+\// }This ensures we only match gem directories with version numbers.
Rakefile (1)
26-30: Consider documenting the coverage tool requirement.The coverage task assumes that test files will check for the
COVERAGEenvironment variable and enable coverage tools accordingly.Add a comment to clarify the coverage tool setup:
desc "Run all tests with coverage" task :coverage do + # Expects simplecov or similar to be configured in test_helper.rb ENV["COVERAGE"] = "true" Rake::Task["test"].invoke endtest/unit/test_driver_test.rb (1)
61-66: Add trailing comma for consistency.The multiline array should have a trailing comma after the last element for better consistency and cleaner diffs when adding new elements.
pixels = [ [255, 0, 0, 255], # Red [0, 255, 0, 255], # Green [0, 0, 255, 255], # Blue - [255, 255, 255, 255] # White + [255, 255, 255, 255], # White ]🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 65-65: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/unit/screenshot_namer_test.rb (1)
12-14: Consider performance optimization for teardown.The teardown method could be slightly optimized by removing the directory existence check since
FileUtils.remove_entryhandles non-existent paths gracefully.teardown do - FileUtils.remove_entry(@screenshot_area_root) if Dir.exist?(@screenshot_area_root) + FileUtils.remove_entry(@screenshot_area_root) rescue Errno::ENOENT + # Directory already removed, ignore endlib/capybara/screenshot/diff/image_preprocessor.rb (3)
23-29: Consider immutability for the comparison object.The
process_comparisonmethod modifies the comparison object in place by reassigning itsbase_imageandnew_imageattributes. This could lead to unexpected side effects if the same comparison object is used elsewhere.Consider returning a new comparison object or making the mutation explicit:
def process_comparison(comparison) - # Process both images - comparison.base_image = process_image(comparison.base_image, comparison.base_image_path) - comparison.new_image = process_image(comparison.new_image, comparison.new_image_path) - - comparison + # Process both images and return updated comparison + comparison.dup.tap do |updated_comparison| + updated_comparison.base_image = process_image(comparison.base_image, comparison.base_image_path) + updated_comparison.new_image = process_image(comparison.new_image, comparison.new_image_path) + end end
31-33: Inconsistent parameter usage in the call method.The
callmethod passesnilas the path parameter toprocess_image, which makes the median filter warning less informative when processing images directly.Consider either requiring paths or handling the nil case more gracefully:
-def call(images) - images.map { |image| process_image(image, nil) } -end +def call(images, paths = []) + images.map.with_index { |image, index| process_image(image, paths[index]) } +end
59-64: Improve warning message readability.The warning message construction spans multiple lines with string concatenation, which could be more readable.
Consider using a here document or format string for better readability:
def warn_about_skipped_median_filter(path) - warn( - "[capybara-screenshot-diff] Median filter has been skipped for #{path} " \ - "because it is not supported by #{driver.class}" - ) + warn("[capybara-screenshot-diff] Median filter has been skipped for #{path} " \ + "because it is not supported by #{driver.class}") endlib/capybara/screenshot/diff/difference.rb (1)
8-21: Remove duplicate class description.The class description "Represents a difference between two images" appears twice (lines 8 and 20), which is redundant.
# Represents a difference between two images # # This value object encapsulates the result of an image comparison operation. # It follows the Single Responsibility Principle by focusing solely on representing # the difference state, including: # - Whether images are different or equal # - Why they differ (dimensions, pixels, etc.) # - The specific region of difference # - Whether differences are tolerable based on configured thresholds # # As part of the layered comparison architecture, this class represents the final # output of the comparison process, containing all data needed for reporting. -# Represents a difference between two images class Difference < Struct.new(:region, :meta, :comparison, :failed_by, :base_image_path, :image_path, keyword_init: nil)test/support/capybara_screenshot_diff/dsl_stub.rb (1)
55-61: Consider adding error handling for snapshot creation.The
create_snapshot_formethod uses a time-based name fallback but doesn't handle potential errors fromset_test_images. Consider adding error handling or validation.def create_snapshot_for(expected, actual = nil, name: nil) actual ||= expected name ||= "#{actual}_#{Time.now.nsec}" - @manager.snapshot(name).tap do |snap| - set_test_images(snap, expected, actual) - end + @manager.snapshot(name).tap do |snap| + begin + set_test_images(snap, expected, actual) + rescue => e + Rails.logger.warn("Failed to set test images for snapshot #{name}: #{e.message}") if defined?(Rails) + raise + end + end endtest/integration/browser_screenshot_test.rb (1)
172-172: Very strict tolerance might cause test flakiness.The tolerance value of 0.001 in the "skip_area converts coordinates to be relative to cropped region" test is extremely strict. This might cause test flakiness due to minor rendering differences across environments.
Consider using a slightly higher tolerance (e.g., 0.01) to reduce potential flakiness while still ensuring the test validates the functionality:
- assert_matches_screenshot("index-cropped", skip_area: "#first-field", crop: "form", tolerance: 0.001) + assert_matches_screenshot("index-cropped", skip_area: "#first-field", crop: "form", tolerance: 0.01)lib/capybara_screenshot_diff/screenshot_namer.rb (1)
75-80: Consider adding safety measures for directory deletion.While the existence check is good,
FileUtils.rm_rfis a destructive operation. Consider adding:
- Validation that the path is within the expected screenshot area
- Logging of the deletion for audit purposes
- A safety check to prevent deletion of system directories
def clear_current_group_directory dir_to_clear = current_group_directory - FileUtils.rm_rf(dir_to_clear) if Dir.exist?(dir_to_clear) + if Dir.exist?(dir_to_clear) + # Ensure we're only deleting within the screenshot area + unless dir_to_clear.start_with?(screenshot_area) + raise ArgumentError, "Attempted to delete directory outside screenshot area: #{dir_to_clear}" + end + FileUtils.rm_rf(dir_to_clear) + end endlib/capybara/screenshot/diff/screenshot_matcher.rb (1)
87-93: Add trailing comma for consistency.The method logic is good, but adding a trailing comma would improve consistency with Ruby style guidelines.
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] 91-91: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/support/test_doubles.rb (1)
148-149: Clarify the dual naming for image paths.The fallback from
:image_pathto:new_image_pathsuggests inconsistent naming conventions. This could lead to confusion about which parameter name should be used.Consider standardizing on a single parameter name or documenting why both are needed:
-@new_image_path = options[:new_image_path] || options[:image_path] -@base_image_path = options[:base_image_path] +# If both names are needed, add a comment explaining why +# e.g., ":image_path for backward compatibility, :new_image_path for clarity" +@new_image_path = options[:new_image_path] || options[:image_path] +@base_image_path = options[:base_image_path]lib/capybara/screenshot/diff/image_compare.rb (1)
65-65: Remove redundant file existence check.The
ensure_files_exist!method is called both in the constructor (line 48) and inquick_equal?(line 65). Since files are validated during initialization, the second check is redundant.def quick_equal? - ensure_files_exist! - # Quick file size check - if sizes are equal, perform a simple file comparison if base_image_path.size == image_path.sizeAlso applies to: 79-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (85)
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!**/*.pngtest/fixtures/files/images/a.pngis excluded by!**/*.pngtest/fixtures/files/images/a_cropped.pngis excluded by!**/*.pngtest/fixtures/files/images/b.pngis excluded by!**/*.pngtest/fixtures/files/images/c.pngis excluded by!**/*.pngtest/fixtures/files/images/d.pngis excluded by!**/*.pngtest/fixtures/files/images/portrait.pngis excluded by!**/*.pngtest/fixtures/files/images/portrait_b.pngis excluded by!**/*.png
📒 Files selected for processing (86)
.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)Rakefile(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/test_vcs.rb(1 hunks)lib/capybara/screenshot/diff/vcs.rb(1 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/area_calculator_test.rb(0 hunks)test/capybara/screenshot/diff/difference_test.rb(0 hunks)test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb(0 hunks)test/capybara/screenshot/diff/image_compare_refactor_test.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/snap_manager_test.rb(0 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/rspec_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/support/test_doubles.rb(1 hunks)test/support/test_driver.rb(1 hunks)test/support/test_helpers.rb(1 hunks)test/system_test_case.rb(2 hunks)test/test_helper.rb(2 hunks)test/unit/area_calculator_test.rb(1 hunks)test/unit/comparison_loader_test.rb(1 hunks)test/unit/difference_finder_test.rb(1 hunks)test/unit/difference_test.rb(1 hunks)test/unit/drivers/chunky_png_driver_test.rb(1 hunks)test/unit/drivers/utils_test.rb(1 hunks)test/unit/drivers/vips_driver_test.rb(10 hunks)test/unit/dsl_test.rb(1 hunks)test/unit/image_compare_test.rb(4 hunks)test/unit/image_preprocessor_test.rb(1 hunks)test/unit/region_test.rb(4 hunks)test/unit/screenshot_namer_test.rb(1 hunks)test/unit/screenshot_test.rb(1 hunks)test/unit/screenshoter_test.rb(1 hunks)test/unit/snap_manager_test.rb(1 hunks)test/unit/stable_screenshoter_test.rb(6 hunks)test/unit/test_driver_test.rb(1 hunks)test/unit/vcs_test.rb(2 hunks)
💤 Files with no reviewable changes (6)
- test/capybara/screenshot/diff/difference_test.rb
- test/capybara/screenshot/diff/area_calculator_test.rb
- test/capybara_screenshot_diff/snap_manager_test.rb
- test/capybara/screenshot/diff/test_methods_test.rb
- test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb
- lib/capybara/screenshot/diff/test_methods.rb
🧰 Additional context used
🧬 Code Graph Analysis (15)
test/unit/region_test.rb (1)
lib/capybara/screenshot/diff/region.rb (1)
from_edge_coordinates(10-15)
lib/capybara/screenshot/diff/drivers/vips_driver.rb (2)
lib/capybara/screenshot/diff/region.rb (1)
to_top_left_corner_coordinates(21-23)lib/capybara/screenshot/diff/drivers/base_driver.rb (1)
dimension(30-32)
lib/capybara/screenshot/diff/comparison.rb (1)
lib/capybara/screenshot/diff/difference.rb (1)
options(45-47)
lib/capybara_screenshot_diff/snap.rb (2)
lib/capybara_screenshot_diff/snap_manager.rb (1)
abs_path_for(27-29)lib/capybara_screenshot_diff/screenshot_namer.rb (1)
full_name(42-51)
lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb (1)
lib/capybara_screenshot_diff/backtrace_filter.rb (1)
filtered(14-18)
test/unit/snap_manager_test.rb (4)
test/test_helper.rb (1)
fixture_image_path_from(80-82)lib/capybara_screenshot_diff/snap_manager.rb (1)
provision_snap_with(36-40)lib/capybara_screenshot_diff/screenshot_namer.rb (1)
full_name(42-51)lib/capybara_screenshot_diff/snap.rb (1)
next_attempt_path!(35-40)
lib/capybara/screenshot/diff/screenshot_namer_dsl.rb (2)
lib/capybara/screenshot/diff/os.rb (1)
name(10-16)lib/capybara_screenshot_diff/screenshot_namer.rb (2)
section(27-30)group(34-37)
lib/capybara/screenshot/diff/standard_capture_strategy.rb (1)
lib/capybara/screenshot/diff/reporters/default.rb (1)
driver(113-115)
lib/capybara/screenshot/diff/test_vcs.rb (1)
lib/capybara/screenshot/diff/vcs.rb (3)
checkout_vcs(12-14)checkout_vcs(16-77)checkout_vcs(17-23)
lib/capybara_screenshot_diff/snap_manager.rb (1)
lib/capybara_screenshot_diff/snap.rb (2)
delete!(16-20)attr_reader(4-54)
test/integration/browser_screenshot_test.rb (5)
lib/capybara/screenshot/diff/difference.rb (1)
tolerance(49-51)lib/capybara/screenshot/diff/reporters/default.rb (1)
driver(113-115)lib/capybara_screenshot_diff/minitest.rb (1)
before_teardown(37-46)lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
assertions_present?(100-102)test/test_helper.rb (1)
persist_comparisons?(64-66)
lib/capybara/screenshot/diff/difference.rb (2)
lib/capybara/screenshot/diff/reporters/default.rb (2)
comparison(117-119)base_image_path(105-107)lib/capybara/screenshot/diff/difference_finder.rb (1)
freeze(31-98)
lib/capybara/screenshot/diff/difference_finder.rb (3)
lib/capybara/screenshot/diff/image_compare.rb (2)
build_null_difference(173-175)different?(95-97)lib/capybara/screenshot/diff/difference.rb (2)
options(45-47)build_null(22-31)lib/capybara/screenshot/diff/region.rb (1)
empty?(80-82)
test/support/test_driver.rb (1)
lib/capybara/screenshot/diff/reporters/default.rb (2)
base_image(97-99)new_image(101-103)
test/support/test_doubles.rb (2)
lib/capybara/screenshot/diff/reporters/default.rb (1)
comparison(117-119)lib/capybara/screenshot/diff/difference.rb (1)
options(45-47)
🪛 RuboCop (1.75.5)
lib/capybara/screenshot/diff/reporters/default.rb
[convention] 84-84: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/unit/test_driver_test.rb
[convention] 65-65: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara/screenshot/diff/difference_finder.rb
[convention] 61-86: Cyclomatic complexity for call is too high. [8/7]
(Metrics/CyclomaticComplexity)
test/support/test_driver.rb
[warning] 42-48: Call super to initialize state of the parent class.
(Lint/MissingSuper)
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)
[error] 46-46: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🔇 Additional comments (164)
test/fixtures/app/index-with-anim.html (1)
7-7: Approve: Added consistent font-family for visual tests
The newfont-familyrule ensures that this fixture renders with the same typography as the other fixtures, improving consistency in screenshot comparisons.test/fixtures/app/index.html (4)
3-3: Approve: Reordered viewport meta attributes for consistency
Switching tocontent="…" name="viewport"matches the attribute order used in other fixtures without affecting rendering.
4-8: Approve: Added inline CSS for body font-family
Including the same prioritized font-family list here keeps all fixtures visually aligned during regression tests.
15-15: Approve: Reordered input attributes for consistent markup
Movingidandstylebeforetypeandvaluemirrors the ordering in related fixtures and does not change functionality.
20-20: Approve: Reordered second input attributes for consistency
This aligns the attribute sequence with the first input and other fixtures.test/fixtures/app/index-without-img.html (4)
3-3: Approve: Reordered viewport meta attributes for consistency
Matching the attribute order used in other fixtures ensures uniform parsing and easier maintenance.
4-8: Approve: Added inline CSS for body font-family
This addition standardizes the font rendering across all HTML fixtures for visual regression testing.
14-14: Approve: Reordered first input attributes for consistent markup
Adjusts the attribute order to match other fixtures without altering behavior.
19-19: Approve: Reordered second input attributes for consistency
Ensures all form elements follow the same attribute sequencing for readability and maintenance..gitignore (2)
1-1: LGTM! Standard editor backup file pattern.Adding the
*~pattern to ignore editor backup files (commonly created by Emacs and other editors) is a good practice to keep the repository clean.
4-4: LGTM! Appropriate IDE directory exclusion.Adding
/.windsurfto ignore the Windsurf AI code editor directory follows standard practices for excluding IDE-specific files and directories.gemfiles/rails71_gems.rb (1)
6-7: LGTM! Improved dependency granularity and explicit loading.Splitting the gem declaration and adding explicit
requireoptions is a good practice that:
- Provides better control over which components are loaded
- Improves modularity by separating ActiveSupport from ActionPack
- Aligns with similar changes across other Rails gemfiles
- Can potentially improve performance by loading only needed components
The explicit require lists are appropriate for the typical usage patterns of these gems.
.standard.yml (1)
5-5:Details
✅ Verification successful
LGTM! Appropriate Ruby version for JRuby compatibility.
Downgrading to Ruby 3.1 to support JRuby 9.4 is reasonable for cross-platform compatibility. This ensures Standard Ruby's linting rules work consistently across different Ruby implementations.
Verify that this Ruby version change doesn't negatively impact linting for other Ruby implementations used in the project:
🏁 Script executed:
#!/bin/bash # Description: Check if the project supports multiple Ruby versions and verify compatibility # Expected: Find Ruby version specifications in CI configs and other files echo "Checking Ruby version specifications across the project..." rg -g "!vendor" -g "!tmp" "ruby.*version|RUBY_VERSION" --type yaml --type rb -A 2 -B 2 echo -e "\nChecking GitHub Actions workflow files for Ruby matrix..." fd -t f -e yml -e yaml . .github/workflows/ --exec grep -l "ruby" {} \; --exec cat {} \;Length of output: 6930
Approve Ruby version update
The.standard.ymlchange lockingruby_versionto 3.1 aligns Standard RB’s target syntax with JRuby 9.4 compatibility. Our CI lint job runs on Ruby 3.3 and the test matrix covers Ruby 3.2–3.4 plus JRuby 9.4/10.0/HEAD. Standard RB uses the host Ruby runtime; the config’s target version only drives lint rules—tests remain unaffected. No further action required.gemfiles/edge_gems.rb (1)
7-7: LGTM! Consistent dependency management across gemfiles.Adding
activesupportalongsideactionpackfrom the Rails edge repository maintains consistency with the explicit dependency separation implemented in other gemfiles. This ensures comprehensive testing against the latest Rails development versions..github/actions/setup-ruby-and-dependencies/action.yml (1)
39-40: LGTM! Ensures consistent font rendering for screenshot tests.Disabling font antialiasing is a good practice for screenshot testing, especially when parallelizing tests across different environments. This prevents rendering variations that could cause false positives in image comparisons.
lib/capybara/screenshot/diff/drivers/base_driver.rb (1)
34-36: LGTM! Clean feature detection implementation.The
supports?method provides a standardized way for drivers to declare feature support, enabling conditional logic in image preprocessing and difference detection components. Therespond_to?approach is appropriate for this use case.lib/capybara/screenshot/diff/comparison.rb (1)
5-7: LGTM! Clean accessor method for skip area functionality.The
skip_areamethod provides standardized access to skip area configuration, enabling consistent usage across image preprocessing and difference detection components.capybara-screenshot-diff.gemspec (1)
26-27:Details
✅ Verification successful
Verify that moving actionpack to development dependency won't break existing functionality.
Moving actionpack from runtime to development dependency could be a breaking change if any runtime code depends on it. Please ensure that:
- No runtime code in the gem requires actionpack functionality
- Existing users won't experience breakage after this change
- The gem still works correctly in applications that don't have actionpack installed
🏁 Script executed:
#!/bin/bash # Description: Check if any runtime code depends on actionpack # Expected: No actionpack usage in lib/ directory runtime code echo "Searching for actionpack usage in lib/ directory:" rg -i "actionpack|action_pack" lib/ echo "Searching for Rails-specific classes that might depend on actionpack:" rg -i "(ActionController|ActionView|Rails\.application)" lib/ echo "Checking require statements for actionpack:" rg "require.*action" lib/Length of output: 484
No runtime ActionPack usage detected – moving to development dependency is safe.
Ripgrep searches in
lib/found no references toActionPack,ActionController,ActionView, or other Rails-specific classes. The gem’s runtime code does not require ActionPack, so this change will not break existing users.test/support/non_minitest_assertions.rb (1)
9-9:Details
✅ Verification successful
Verify DSL module migration consistency.
The migration from
Capybara::Screenshot::Diff::TestMethodstoCapybaraScreenshotDiff::DSLlooks correct and aligns with the broader refactoring described in the summary.Let me verify that all references to the old module have been updated consistently:
🏁 Script executed:
#!/bin/bash # Description: Verify all references to TestMethods have been migrated to DSL # Expected: No remaining references to the old TestMethods module echo "Checking for any remaining references to old TestMethods module:" rg "Capybara::Screenshot::Diff::TestMethods" --type ruby echo -e "\nChecking for new DSL module usage:" rg "CapybaraScreenshotDiff::DSL" --type rubyLength of output: 2663
All old
TestMethodsreferences removed and new DSL usage is consistent.No remaining references to
Capybara::Screenshot::Diff::TestMethods, andCapybaraScreenshotDiff::DSL(andDSLStub) are used uniformly across tests and libraries.gemfiles/rails80_gems.rb (1)
6-6:Details
✅ Verification successful
LGTM! Verify consistency across Rails gemfiles.
The addition of
activesupportwith appropriate version constraint for Rails 8.0 is correct.Let me verify that activesupport declarations are consistent across all Rails gemfiles:
🏁 Script executed:
#!/bin/bash # Description: Verify activesupport gem declarations across all Rails gemfiles # Expected: Consistent patterns across different Rails versions echo "Checking activesupport declarations in all Rails gemfiles:" fd "rails.*_gems.rb" gemfiles/ --exec grep -H "activesupport" echo -e "\nChecking version patterns:" fd "rails.*_gems.rb" gemfiles/ --exec grep -H "gem.*activesupport"Length of output: 922
activesupport version constraints are consistent across Rails gemfiles
All three gemfiles declare activesupport with the same “~> major.minor.0” pattern:
• gemfiles/rails70_gems.rb:
gem "activesupport", "~> 7.0.0"
• gemfiles/rails71_gems.rb:gem "activesupport", "~> 7.1.0"
• gemfiles/rails80_gems.rb:gem "activesupport", "~> 8.0.0"No further changes are needed.
lib/capybara/screenshot/diff/drivers/vips_driver.rb (1)
43-43: LGTM! Minor formatting improvement.The indentation adjustment improves code formatting consistency with no functional changes.
gemfiles/rails70_gems.rb (1)
7-7:Details
✅ Verification successful
Verify the need for selective ActiveSupport requires.
The addition looks correct for Rails 7.0, but the selective
requireoptions differ from other Rails versions. Please verify if this selective loading is necessary for Rails 7.0 compatibility or test parallelization.Let me check the require patterns across different Rails versions and understand why selective requiring is used:
🏁 Script executed:
#!/bin/bash # Description: Compare activesupport require patterns across Rails versions # Expected: Understanding of why selective requires are used for Rails 7.0 echo "Checking activesupport require patterns across all Rails gemfiles:" fd "rails.*_gems.rb" gemfiles/ --exec grep -H -A1 -B1 "activesupport" echo -e "\nChecking if these specific modules are used in tests:" rg "ActiveSupport::Deprecator|ActiveSupport::TestCase" test/ --type rubyLength of output: 3386
Selective ActiveSupport requires are valid for Rails 7.0
The explicitrequire: %w[active_support/deprecator active_support/test_case]ingemfiles/rails70_gems.rbaligns with your test suite’s heavy use ofActiveSupport::TestCaseand ensures necessary deprecation support, while keeping load-time to a minimum. No further changes required.test/fixtures/files/rspec_spec.rb (3)
20-20: LGTM: Path standardization improves test consistency.The change from Rails.root-based path to
ActiveSupport::TestCase.file_fixture_path / "app"aligns with the broader test infrastructure standardization mentioned in the AI summary.
24-24: LGTM: Explicit tolerance configuration enhances clarity.Setting the tolerance explicitly to
0.5makes the test configuration more transparent and maintainable.
28-28:Details
✅ Verification successful
Verify DSL module transition consistency across codebase.
The change from
Capybara::Screenshot::Diff::TestMethodstoCapybaraScreenshotDiff::DSLrepresents a significant API change. Ensure all references to the old module have been updated consistently.
🏁 Script executed:
#!/bin/bash # Description: Verify all references to TestMethods have been replaced with DSL # Expected: No references to TestMethods should remain echo "Searching for remaining TestMethods references:" rg -n "TestMethods" --type ruby echo -e "\nSearching for DSL usage patterns:" rg -n "CapybaraScreenshotDiff::DSL" --type ruby echo -e "\nSearching for old module pattern:" rg -n "Capybara::Screenshot::Diff::TestMethods" --type rubyLength of output: 2941
Module reference update verified across codebase
No remaining references to
Capybara::Screenshot::Diff::TestMethods; all usages now point toCapybaraScreenshotDiff::DSL.test/unit/region_test.rb (1)
7-7: LGTM: Excellent test description improvements.The updated test method names are significantly more descriptive and clearly communicate what each test verifies:
- Method under test is explicit (e.g.,
#move_by,#find_intersect_with)- Expected behavior is clearly stated
- Follows Ruby testing best practices
These improvements enhance test maintainability and make the test suite more self-documenting.
Also applies to: 16-16, 26-26, 44-44, 48-48, 53-53, 58-58
lib/capybara/screenshot/diff/stable_screenshoter.rb (1)
103-103: LGTM: Enhanced exception handling with call stack context.The explicit instantiation of
CapybaraScreenshotDiff::UnstableImagewith thecallerparameter provides valuable debugging context. This aligns with the broader error handling improvements mentioned in the AI summary and will make troubleshooting unstable screenshot issues more effective.lib/capybara/screenshot/diff/screenshoter.rb (1)
89-89: LGTM: Consistent exception handling improvement.The explicit instantiation of
CapybaraScreenshotDiff::ExpectationNotMetwith thecallerparameter maintains consistency with the similar enhancement instable_screenshoter.rb. This provides better debugging context when images fail to load within the timeout period..dockerignore (1)
1-12: LGTM! Well-structured Docker ignore patterns.The
.dockerignorefile correctly excludes development directories and temporary files that shouldn't be included in Docker builds. The screenshot report file patterns are comprehensive, covering both PNG and WebP formats with various suffixes (attempt, diff, base).lib/capybara_screenshot_diff/rspec.rb (1)
30-30: Excellent improvement to preserve backtrace information.The explicit backtrace preservation ensures that the original stack trace context is maintained when re-raising the exception as an RSpec expectation failure. This will significantly help with debugging failed screenshot comparisons.
test/support/screenshoter_stub.rb (1)
14-14: Good addition for filename normalization.The regex pattern correctly removes numeric suffixes (e.g., "_123") from filenames before file extensions. This appears to support test parallelization by normalizing screenshot names to find the correct source images, which aligns with the PR's objective.
test/integration/rspec_test.rb (1)
7-7: Much more descriptive test method name.The updated test name clearly communicates what the test validates, making it easier to understand test results and failures. This improves test maintainability and readability.
lib/capybara_screenshot_diff/snap.rb (1)
52-52: LGTM! Improved pattern precision for attempt files.The change from
attempt_*toattempt_[0-9][0-9]makes the glob pattern more precise and consistent with the actual file naming convention used innext_attempt_path!(line 37) which usessprintf(".attempt_%02i.#{format}", @attempts_count). This ensures only valid two-digit attempt files are matched, preventing potential issues with files that have similar but different naming patterns.test/integration/test_methods_system_test.rb (1)
18-19: LGTM! Correct migration to new DSL modules.The replacement of
TestMethodsandTestMethodsStubwithCapybaraScreenshotDiff::DSLandCapybaraScreenshotDiff::DSLStubaligns with the broader refactoring to a DSL-based approach mentioned in the AI summary. This maintains the same functionality while using the updated module structure.lib/capybara/screenshot/diff/stable_capture_strategy.rb (1)
1-24: LGTM! Well-designed capture strategy implementation.The
StableCaptureStrategyclass follows excellent design patterns:
- Strategy Pattern: Properly inherits from
CaptureStrategybase class- Delegation: Cleanly delegates screenshot capture to
StableScreenshoter- Separation of Concerns: Constructor handles initialization, capture method focuses on delegation
- Clear Documentation: Good class-level comment explaining the stabilization behavior
The implementation is clean, focused, and integrates well with the broader capture strategy framework.
lib/capybara_screenshot_diff/error_with_filtered_backtrace.rb (1)
1-16: LGTM! Excellent enhancement for error debugging.The
ErrorWithFilteredBacktraceclass provides a valuable improvement to error reporting by automatically filtering out library-specific backtrace entries. Key strengths:
- Better UX: Filters out noise from backtraces (library code, specific gems) to focus on user code
- Proper Inheritance: Correctly inherits from
StandardErrorand callssuper(message)- Clean Design: Uses dependency injection with
BacktraceFilterfor modularity- Good Documentation:
@privateannotations clearly indicate internal APIThe integration with
BacktraceFilter.filtered(which removes entries fromactivesupport,minitest,railtiesgems and library directory) will make error messages much more actionable for developers.test/support/setup_rails_app.rb (2)
6-7: LGTM: Rails 7.0 compatibility improvements.The explicit requires for "logger" and "action_controller" are necessary for Rails 7.0+ compatibility where these dependencies may not be automatically loaded.
11-11: LGTM: Improved configurability with class attribute accessor.Converting from a class method to
cattr_accessormakes therootpath configurable, which is beneficial for testing scenarios where different root paths might be needed. The default value maintains backward compatibility.test/system_test_case.rb (2)
19-19: LGTM: Consistent use of test fixture paths.The change from
Rails.roottoActiveSupport::TestCase.file_fixture_path / "app"aligns with using proper test fixture locations for screenshot storage during tests.
68-68: LGTM: Updated to use new VCS abstraction pattern.The change from
Vcs.restore_git_revisiontoVcs::GitVcs.new.restore_git_revisionaligns with the pluggable VCS provider pattern introduced in this refactor. UsingCapybara::Screenshot.rootinstead ofRails.rootmaintains consistency with the path changes.lib/capybara/screenshot/diff/standard_capture_strategy.rb (1)
1-25: LGTM: Well-implemented capture strategy pattern.This implementation follows good object-oriented design principles:
- Clear single responsibility (standard screenshot capture)
- Proper inheritance from base
CaptureStrategyclass- Clean delegation to
Screenshoterinstance- Appropriate use of dependency injection via options
The code is readable and follows the strategy pattern correctly for modularizing screenshot capture logic.
test/integration/record_screenshot_test.rb (3)
7-8: LGTM: Updated to use new screenshot namer DSL.The change from instance variables to using
CapybaraScreenshotDiff.screenshot_namer.sectionand.groupaligns with the new centralized screenshot naming architecture. The conditional logic ensures these are only set when not already configured.
10-12: LGTM: Driver-specific tolerance configuration.Setting different tolerance values based on the driver type (0.035 for :vips, 0.7 for others) accounts for the different precision characteristics of various screenshot comparison drivers. This helps ensure appropriate test sensitivity.
14-17: LGTM: Proper test cleanup and isolation.The teardown method properly resets
blur_active_elementand restores the original tolerance, ensuring test isolation and preventing state leakage between tests. This is essential for reliable parallel test execution.lib/capybara/screenshot/diff/screenshot_namer_dsl.rb (1)
1-36: LGTM! Clean and well-structured DSL module.The module provides a clear interface for managing screenshot naming conventions with proper documentation and consistent method signatures. The delegation to a global screenshot namer instance follows a good separation of concerns pattern.
test/unit/screenshoter_test.rb (1)
8-10: Good refactoring supporting test parallelization.The change from
ActionDispatch::IntegrationTesttoActiveSupport::TestCasealigns well with the PR objective to parallelize tests. Unit tests typically have better parallelization characteristics than integration tests. The DSL module updates are consistent with the broader refactoring effort.lib/capybara/screenshot/diff/reporters/default.rb (3)
48-48: Improved encapsulation with proper accessor method.Good change from accessing
difference.meta[:diff_mask]to using the properdifference.diff_maskaccessor method. This improves encapsulation and consistency.
53-53: Better encapsulation for skip area access.The change to access skip area through
difference.comparison.skip_areainstead of directly on the difference object improves the object model's consistency and encapsulation.
92-92: Consistent use of accessor method.Good change to use
difference.diff_maskinstead of accessing the meta hash directly, maintaining consistency with the other accessor changes in this file.lib/capybara_screenshot_diff.rb (4)
8-8: Good alignment with DSL refactoring.The change from requiring
test_methodstoscreenshot_namer_dslaligns with the broader refactoring effort to replace the old test methods with the new DSL-based approach.
12-12: Improved error handling infrastructure.The addition of the filtered backtrace error handling requirement sets up better error reporting capabilities throughout the system.
15-15: Enhanced error reporting with filtered backtraces.The change to inherit from
ErrorWithFilteredBacktraceinstead ofStandardErrorshould improve the debugging experience by providing cleaner, more relevant stack traces.
21-21: Good addition of specific error class.The new
WindowSizeMismatchErrorclass provides better error specificity and inherits from the improved backtrace filtering base class, enhancing both error clarity and debugging experience.test/unit/comparison_loader_test.rb (1)
1-61: LGTM! Comprehensive test suite for ComparisonLoader.The test suite follows Rails testing conventions well and provides good coverage of the
ComparisonLoaderfunctionality. The tests verify:
- Correct instantiation and attribute assignment
- Image loading order preservation
- Option forwarding
- Driver interaction
The use of test doubles is appropriate for isolating the unit under test.
lib/capybara/screenshot/diff/capture_strategy.rb (1)
6-26: Excellent abstract base class design following the Strategy pattern.The class design is well-structured with:
- Clear documentation explaining its purpose
- Proper initialization of option hashes
- Abstract method with appropriate
NotImplementedError- Private accessors for subclass use
This provides a solid foundation for concrete capture strategy implementations.
test/unit/difference_test.rb (1)
8-18:Details
✅ Verification successful
Verify test logic - empty different_dimensions array should not indicate different images.
The test setup creates a
Differencewith{different_dimensions: []}(empty array) but expectsdifferent?andfailed?to returntrue. This seems counterintuitive - an empty array typically indicates no differences were found.Please verify this is the correct behavior or if the test setup should use a non-empty array to indicate actual dimension differences.
🏁 Script executed:
#!/bin/bash # Description: Check the Difference class implementation to understand the logic for different? and failed? methods # Search for the Difference class and its different? and failed? methods ast-grep --pattern 'class Difference $$$ def different? $$$ end $$$ def failed? $$$ end $$$ end' # Also search for any logic related to different_dimensions rg -A 5 -B 5 "different_dimensions"Length of output: 4603
🏁 Script executed:
#!/bin/bash # Locate the Ruby file defining the Difference class file=$(rg -l "class Difference" -t rb) # Print the file path and first 200 lines to view class implementation echo "Found Difference class in: $file" sed -n '1,200p' "$file" # Show the definitions of different? and failed? with surrounding context grep -R -n -C3 "def different?" "$file" grep -R -n -C3 "def failed?" "$file"Length of output: 335
🏁 Script executed:
#!/bin/bash # Locate the file defining the Difference class without restricting file types file=$(rg -l "class Difference" .) echo "Difference class found in: $file" echo "----- Class Implementation -----" sed -n '1,200p' "$file" echo echo "----- #different? Definition -----" grep -R -n -C3 "def different?" "$file" echo echo "----- #failed? Definition -----" grep -R -n -C3 "def failed?" "$file"Length of output: 2574
🏁 Script executed:
#!/bin/bash # Confirm the Difference class file exists ls -l lib/capybara/screenshot/diff/difference.rb # Display the start of the file to locate method definitions sed -n '1,200p' lib/capybara/screenshot/diff/difference.rb # Show the definitions and bodies of different? and failed? methods grep -n -C3 "def different?" lib/capybara/screenshot/diff/difference.rb grep -n -C3 "def failed?" lib/capybara/screenshot/diff/difference.rbLength of output: 3012
Test logic is correct given current implementation
The
Difference#failed?method simply checks whetherfailed_byis non-nil/truthy, and#different?returnstruewheneverfailed?istrue. In your test you’ve passed afailed_byhash withdifferent_dimensions: [], which is indeed truthy—so bothfailed?anddifferent?returntrueas expected.If you want the test to more closely mirror how dimension failures are generated in
DifferenceFinder(which uses{ different_dimensions: true }), you could replace the empty array with a boolean or even a non‐empty array. But functionally, your existing setup correctly simulates “images have different dimensions.”lib/capybara/screenshot/diff/comparison_loader.rb (1)
6-11: Excellent documentation and design approach.The class documentation clearly explains the single responsibility and coordination with other components. The adherence to SRP is commendable and will improve maintainability.
test/unit/screenshot_test.rb (3)
7-7: Appropriate test class inheritance change.Changing from
ActionDispatch::IntegrationTesttoActiveSupport::TestCaseis appropriate for these unit tests since they're testing path handling functionality rather than requiring integration test features like HTTP requests or controller actions.
8-8: Improved test method naming enhances readability.The change from
def test_namestyle totest "description"style makes the test intentions much clearer and more readable. This is a good practice for test documentation.Also applies to: 12-12, 18-18
25-25: Good defensive programming in cleanup.The conditional cleanup
Capybara::Screenshot.root = @orig_root if @orig_rootproperly handles the case where the original root might be nil, preventing potential issues.test/unit/snap_manager_test.rb (5)
7-13: Excellent test setup and teardown structure.The setup creates a temporary directory and the teardown properly cleans up using the manager's cleanup method. This ensures test isolation and prevents test pollution.
15-23: Good test coverage for default provisioning behavior.This test properly verifies that
provision_snap_withcopies files to the snap path (actual version by default) and that the base path doesn't exist when not explicitly created.
25-33: Comprehensive testing of version-specific provisioning.Testing the
:baseversion explicitly ensures the versioning system works correctly and files are placed in the right locations based on the version parameter.
35-43: Good test for snapshot tracking functionality.This test verifies that the manager properly tracks created snapshots, which is important for cleanup and management functionality.
45-54: Important edge case testing for attempt versions.Testing that attempt versions are tracked but handled differently from regular snapshots is crucial for the snapshot management logic.
lib/capybara_screenshot_diff/minitest.rb (3)
42-42: Improved backtrace preservation enhances debugging.Setting the backtrace to the original exception's backtrace (
e.backtrace) instead of clearing it preserves valuable debugging information. This aligns with the error handling improvements mentioned in the AI summary.
40-43: Consistent error handling pattern with proper context preservation.The error handling properly converts
CapybaraScreenshotDiff::ExpectationNotMettoMinitest::Assertionwhile preserving the backtrace, which should improve the debugging experience for test failures.
25-25:Details
✅ Verification successful
Verify the stack frame adjustment is correct.
The change from
skip_stack_frames + 3toskip_stack_frames + 1reduces the number of skipped frames by 2. This should provide more relevant stack trace information for debugging.Please verify that this change provides the correct stack trace depth for error reporting:
🏁 Script executed:
#!/bin/bash # Description: Search for other stack frame handling patterns in the codebase to verify consistency # Search for skip_stack_frames usage patterns rg "skip_stack_frames" -A 2 -B 2 # Search for similar error handling patterns rg "super.*skip_stack_frames" -A 2 -B 2Length of output: 6713
Stack frame adjustment is consistent and correct.
I’ve verified all
skip_stack_framesusage across the codebase:
- lib/capybara_screenshot_diff/minitest.rb now does
super(..., skip_stack_frames: skip + 1, …), matching:
- lib/capybara_screenshot_diff/dsl.rb (
skip + 1)- lib/capybara/screenshot/diff/screenshot_matcher.rb (
build_screenshot_assertionadds +1, thencaller(skip + 1))- README.md examples (
skip_stack_frames: 1)- RSpec fixture in test/fixtures/files/rspec_spec.rb (
skip_stack_frames: 1)All integrations uniformly add one frame at each wrapper, delivering the intended depth (and reducing the over-skipping from the previous +3). No further changes required.
lib/capybara_screenshot_diff/backtrace_filter.rb (1)
5-9: Well-designed constant and constructor implementation.The
LIB_DIRECTORYconstant correctly identifies the library root, and the trailingFile::SEPARATORensures accurate path matching in thestart_with?check. The optional constructor parameter provides good flexibility for testing.test/support/stub_test_methods.rb (1)
4-4: LGTM! Clean refactoring.The module extraction to
capybara_screenshot_diff/dsl_stubimproves code organization by separating test support utilities into dedicated modules.Rakefile (1)
14-24: Well-structured test task separation.The separate
test:unitandtest:integrationtasks enable targeted test runs and support the PR's parallelization objective. This follows Rails testing conventions and improves developer workflow.lib/capybara_screenshot_diff/snap_manager.rb (4)
14-14: Good architectural improvement: Snapshot trackingAdding a Set to track created snapshots enables more precise cleanup operations compared to the previous directory-based approach.
18-20: Clean implementation of snapshot registrationThe use of
tapto register the snapshot in the tracking set while returning the snapshot object is idiomatic and maintains the method's interface contract.
48-51: Excellent refactor: Granular cleanup vs directory removalThis change from broad directory removal to targeted snapshot cleanup provides better control and reduces the risk of accidentally removing unrelated files. The approach aligns well with the new snapshot tracking architecture.
70-70: Good exposure of snapshots collectionThe
attr_readerprovides appropriate read-only access to the snapshots collection for external inspection without exposing mutation capabilities.lib/capybara/screenshot/diff/screenshot_coordinator.rb (4)
11-27: Excellent coordinator pattern implementationThis module provides a clean abstraction for screenshot capture strategy selection. The unified
captureAPI simplifies the interface while the internal strategy selection logic is well-encapsulated.
30-33: Clean strategy selection logicThe strategy selection based on
stability_time_limitpresence is straightforward and follows a clear conditional pattern that's easy to understand and maintain.
37-44: Robust snapshot handlingThe
ensure_snapmethod properly handles both snapshot objects and string names, providing flexibility in the API while ensuring consistent snapshot object usage downstream.
35-46: Proper encapsulation with private methodsThe use of
private_class_methodcorrectly encapsulates helper methods while keeping the public API minimal and focused..github/workflows/test.yml (4)
24-24: Good performance optimization for JRubyEnabling the JRuby thread pool should improve performance and reduce test execution times, especially beneficial for the parallelized test scenarios mentioned in the PR title.
48-49: Explicit driver configuration for minimal setupSetting the
SCREENSHOT_DRIVERexplicitly ensures consistent test behavior across different environments and makes the test requirements more transparent.
92-92: Timeout adjustments reflect JRuby characteristicsThe increased timeouts (20 minutes for matrix, 7 minutes for retry) acknowledge JRuby's slower startup and execution times. These adjustments should reduce false failures due to timeouts.
Also applies to: 135-135
96-96: Improved specificity with explicit JRuby versionsUsing specific JRuby versions (
jruby-9.4,jruby-10.0) instead of genericjrubyprovides better reproducibility and clearer compatibility constraints.Also applies to: 109-109
Dockerfile (5)
8-10: Good consolidation of environment variablesCombining the
ENVdeclarations into a single instruction reduces the number of image layers and improves build efficiency.
11-41: Excellent build optimization with cache mountThe
--mount=type=cachedirective will significantly speed up subsequent builds by caching the apt package downloads. The consolidation of apt commands into a singleRUNinstruction also reduces image layers.🧰 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)
12-12: Repository URL change for consistencySwitching from the security repository to the main archive URL should provide more consistent package availability and reduce potential download issues.
43-43: Smart fix for consistent font renderingDisabling font antialiasing ensures consistent screenshot rendering across different environments, which is crucial for screenshot comparison tests.
🧰 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: Efficient directory setupCombining the directory creation and permission setting into a single command reduces image layers while maintaining the necessary permissions for the application.
🧰 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)
test/capybara/screenshot/diff/image_compare_refactor_test.rb (1)
1-61: Well-structured test suite with comprehensive coverage.This test file provides excellent coverage of the refactored
ImageCompareclass with clear, focused test cases for each method. The test structure follows Ruby testing best practices and includes both positive and negative test scenarios.However, there's a discrepancy between the PR title "test: parallelize tests" and the actual content, which appears to be a comprehensive refactor of screenshot comparison infrastructure rather than test parallelization.
Likely an incorrect or invalid review comment.
test/unit/vcs_test.rb (1)
8-8: Good refactoring to unit test approach.Changing from
ActionDispatch::IntegrationTesttoActiveSupport::TestCaseis appropriate since VCS operations are better suited for unit testing rather than integration testing.test/unit/test_driver_test.rb (1)
1-78: Comprehensive test coverage for fake test driver.This test suite provides excellent coverage of the
FakeTestDriverandTestImagefunctionality with clear, focused test cases. The tests properly validate both the driver configuration methods and image manipulation capabilities.🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 65-65: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/unit/screenshot_namer_test.rb (1)
1-163: Excellent comprehensive test suite with thorough edge case coverage.This test file demonstrates exceptional quality with:
- Comprehensive coverage of all
ScreenshotNamerfunctionality- Proper handling of edge cases (nil, empty strings)
- Clear test naming and organization
- Appropriate setup/teardown with temporary directory management
- Good test isolation and independence
test/unit/area_calculator_test.rb (2)
1-76: Excellent test coverage and organization.The test suite demonstrates good practices with:
- Clear nested test classes for logical grouping
- Comprehensive edge case coverage
- Proper use of both Region objects and array coordinates
- Good assertion patterns
65-71:Details
✅ Verification successful
Verify consistency of return values for edge cases.
The test expects
nilwhenskip_areasis not provided, but other tests expect empty arrays. This inconsistency could indicate unclear API contract.Please verify that returning
nilfor missing skip areas is the intended behavior rather than an empty array, which would be more consistent with the other edge case test.
🏁 Script executed:
#!/bin/bash # Description: Check AreaCalculator implementation to verify return value consistency # Expected: Find the actual implementation to confirm nil vs empty array behavior ast-grep --pattern 'class AreaCalculator $$$ def calculate_skip_area $$$ end $$$ end'Length of output: 4375
Nil return for missing skip areas is intentional
The implementation of
calculate_skip_areaexplicitly returnsnilwhen@skip_areais not set and returns an empty array when@skip_areais provided (even if it’s empty). The test at lines 65–71 correctly reflects this API contract, so no changes are needed.lib/capybara/screenshot/diff/difference.rb (2)
22-31: Good addition of the build_null factory method.The
build_nullclass method provides a clean way to create default difference instances with proper metadata. The frozen return value is a good practice for immutability.
89-91: Clean accessor method for diff_mask.The
diff_maskmethod provides a clean interface to access the diff mask from the meta hash.test/unit/image_preprocessor_test.rb (2)
47-67: Excellent test coverage for median filtering.The test properly:
- Skips when VIPS is not available
- Verifies both driver calls and return values
- Checks the correct window size parameter
- Tests both images in the array
29-45: Good test for skip area functionality.The test thoroughly verifies:
- Correct application of black boxes to both images
- Proper region parameters passed to driver
- Expected return values
bin/dtest (3)
3-3: Good addition for robust script execution.Adding
set -o pipefailensures that pipeline failures are properly detected and the script exits with the correct error code.
8-19: Well-implemented environment variable filtering.The whitelist approach for environment variables is a good security practice that prevents unintended environment leakage into Docker containers. The dynamic building of Docker arguments is clean and efficient.
25-25: Proper error handling for setup failure.The explicit exit with status 1 on setup failure ensures that the script fails fast and clearly indicates setup issues.
test/capybara/screenshot/diff_test.rb (7)
27-27: Good refactoring to use new DSL stub module.The replacement of the old
TestMethodsStubwithCapybaraScreenshotDiff::DSLStubaligns with the broader refactoring effort and consolidates test utilities.
44-48: Correct migration to screenshot_namer properties.The changes from instance variables
@screenshot_groupand@screenshot_sectiontoscreenshot_namer.groupandscreenshot_namer.sectionproperties correctly reflect the new architecture using theScreenshotNamerclass.Also applies to: 52-66
151-151: Additional mock expectations for different? method.The added expectations for
different?method calls suggest the comparison logic now calls this method multiple times. Ensure this aligns with the actual implementation to prevent test flakiness.Also applies to: 157-157, 184-185, 191-191
201-201: Consistent module inclusion across test classes.The
ScreenshotFormatTestclass now also includes the newDSLStubmodule, maintaining consistency with the refactoring.
233-243: Well-designed helper methods for test utilities.The addition of
screenshot_dir,screenshot_namer, andbuild_full_namehelper methods provides clean abstractions for accessing screenshot functionality in tests.
30-30:Details
✅ Verification successful
Verify persist_comparisons? method availability.
The teardown now uses
persist_comparisons?method, but ensure this method is available in the test context.
🏁 Script executed:
#!/bin/bash # Description: Find the definition of persist_comparisons? method # Expected: Should find the method definition and confirm it's accessible in test context rg -A 3 -B 3 "def persist_comparisons" --type rbLength of output: 77
🏁 Script executed:
#!/bin/bash # Search for any occurrence or definition of persist_comparisons in Ruby files rg -n "persist_comparisons" --glob '*.rb'Length of output: 396
persist_comparisons? is defined and available in the test context
The
persist_comparisons?method is defined intest/test_helper.rb(line 64) and is required by all tests. No changes needed.
107-107:Details
✅ Verification successful
Verify the corrected naming logic.
The test now expects
"c/b/00_a"instead of"c/b/01_a". This suggests the naming logic changed from 1-based to 0-based indexing. Please verify this is the intended behavior and consistent with the actualScreenshotNamerimplementation.
🏁 Script executed:
#!/bin/bash # Description: Check the ScreenshotNamer implementation for naming logic # Expected: Confirm the indexing starts from 0, not 1 ast-grep --pattern 'class ScreenshotNamer { $$$ full_name($$$) { $$$ } $$$ }'Length of output: 91
🏁 Script executed:
#!/bin/bash # Search for the ScreenshotNamer class and relevant methods rg -n "ScreenshotNamer" . rg -n "class ScreenshotNamer" . rg -n "def full_name" . rg -n "def build_full_name" .Length of output: 1384
🏁 Script executed:
#!/bin/bash # Display the implementation of ScreenshotNamer and its tests to verify indexing logic echo "===== lib/capybara_screenshot_diff/screenshot_namer.rb (lines 1-200) =====" cat -n lib/capybara_screenshot_diff/screenshot_namer.rb | sed -n '1,200p' echo "" echo "===== test/unit/screenshot_namer_test.rb (lines 1-200) =====" cat -n test/unit/screenshot_namer_test.rb | sed -n '1,200p'Length of output: 11439
Naming logic confirmed as 0-based indexing
The
reset_group_countermethod initializes the counter to 0 whenever a group is set, andfull_nameformats the first invocation as00_<name>. This aligns with the updated test expecting"c/b/00_a". No changes are needed.test/support/capybara_screenshot_diff/dsl_stub.rb (3)
1-16: Well-structured test support module.The use of
ActiveSupport::Concernand proper setup/teardown lifecycle management creates a clean and reusable test utility. The SnapManager initialization and screenshoter stubbing are appropriately scoped.
18-23: Clean image comparison helper method.The
make_comparisonmethod provides a convenient interface for creatingImageCompareobjects with sensible defaults and flexible options.
35-48: Effective stubbing mechanism with struct.The
ImageCompareStubstruct andbuild_image_compare_stubmethod provide a clean way to create mock objects for testing. Theequalparameter correctly sets bothquick_equal?anddifferent?properties with inverse logic.test/integration/browser_screenshot_test.rb (6)
9-11: Driver-specific tolerance configuration is well-implemented.Setting different tolerance values based on the diff driver (:vips vs others) acknowledges the different precision characteristics of the drivers. The values (0.035 for vips, 0.13 for others) appear reasonable for image comparison tolerance.
22-22: Consistent use of persist_comparisons? helper.The replacement of environment variable checks with the
persist_comparisons?helper method maintains consistency with the test helper utility and centralizes the logic.
37-39: Appropriate conditional test skipping for screenshot recording.The conditional skipping of tests that modify page content during screenshot recording prevents interference with the recording process. This is a good practice for maintaining clean baseline screenshots.
Also applies to: 102-104, 126-129, 143-146, 164-167, 182-185, 194-197
44-44: Explicit tolerance control in critical tests.Setting
tolerance: nilin specific tests that expect differences or require precise comparison is appropriate for ensuring test reliability.Also applies to: 172-172, 232-232
138-140: Good test coverage for skip_area functionality.The new tests verify that
skip_areaaccepts both arrays of CSS selectors and coordinate arrays, providing comprehensive coverage of the feature.
277-277: Improved error message formatting.Adding an extra newline before the error message list improves readability of test failure output.
lib/capybara_screenshot_diff/screenshot_namer.rb (2)
14-23: Clean initialization and lazy loading pattern.The constructor properly initializes all instance variables, and the
screenshot_areamethod implements a good lazy initialization pattern.
39-51: Well-implemented naming logic with proper counter handling.The method correctly formats the counter with zero-padding before incrementing, and safely constructs the path using
File.join.lib/capybara/screenshot/diff/difference_finder.rb (1)
9-30: Excellent documentation of the comparison process.The detailed step-by-step documentation clearly explains the comparison logic and performance characteristics. This will be very helpful for future maintainers.
test/unit/dsl_test.rb (1)
1-11: Excellent test coverage and structure.The test suite provides comprehensive coverage of the DSL functionality including error handling, message generation, caller stack frame management, delayed execution, and cleanup behavior. The test patterns are well-structured and use appropriate stubbing mechanisms.
Also applies to: 12-142
test/test_helper.rb (2)
51-56: Great parallel test isolation setup.The parallel test setup ensures proper isolation by creating separate Rails root directories and Capybara screenshot roots for each test process. This prevents test interference in parallel execution.
39-41: Improved VCS configuration and cleanup.The centralized FIXTURES_PATH constant and TestVcs provider setup enhance test organization. The updated teardown using SnapManager.cleanup! provides more targeted cleanup compared to removing entire directories.
Also applies to: 61-62
test/support/test_driver.rb (1)
9-36: Excellent test support infrastructure.The
TestImageandFakeTestDriverclasses provide a clean abstraction for testing image comparison logic without relying on actual image files. The fluent interface for configuring driver behavior and the helper methods inActiveSupport::TestCasemake these classes easy to use in tests.Also applies to: 39-95, 101-113
test/unit/drivers/utils_test.rb (1)
11-11: Improved test method naming for better readability.The updated test method names using the
#method_nameconvention and more descriptive phrasing make the test suite more readable and self-documenting. These changes follow good testing practices.Also applies to: 18-18, 26-26, 34-34, 42-42, 48-48
test/support/test_helpers.rb (4)
8-14: LGTM! Clean path helper implementation.The path helper methods provide a clean abstraction for accessing root and test fixture directories. Using
@root ||=for memoization andPathnamefor path handling is appropriate.
17-41: Well-designed assertion helpers for test isolation.The assertion methods in the
Assertionsmodule provide clear, specific checks for image comparison operations. The descriptive error messages and consistent parameter naming (with sensible defaults) will help with debugging failed tests. This supports test parallelization by providing isolated assertions for driver behavior.
44-55: Driver setup method handles optional parameters well.The
setup_driver_resultsmethod properly handles optional parameters with sensible defaults and conditional assignment. Theunless same_pixels.nil?check ensures thatfalsevalues are properly set while avoiding overriding withnil.
58-74: Test data factory methods promote test isolation.The factory methods in
TestDatamodule create isolated test objects which is crucial for parallel test execution. The clear parameter documentation and sensible defaults make these methods easy to use across different test scenarios.test/unit/image_compare_test.rb (2)
15-16: Good refactoring to use ActiveSupport::TestCase for unit tests.Changing from
ActionDispatch::IntegrationTesttoActiveSupport::TestCaseis appropriate for unit tests and supports better test parallelization. The switch toCapybaraScreenshotDiff::DSLStubaligns with the new DSL-based approach.
18-32: Improved test naming and organization.The test method names are now more descriptive and follow good conventions (e.g.,
#initialize creates instance with...). Using themake_comparisonhelper method promotes consistency across tests.lib/capybara/screenshot/diff/vcs.rb (5)
10-14: Clean provider pattern implementation.The
mattr_accessor :providerand delegation toprovider.checkout_vcscreates a clean pluggable architecture while maintaining the same public interface. This supports better testing and flexibility.
16-23: Well-structured GitVcs class encapsulation.Moving the VCS logic into a dedicated
GitVcsclass improves organization and testability. The method signature remains the same, preserving backward compatibility.
25-27: Clean SVN detection method.The
svn?method provides a clear, testable way to detect SVN repositories. Using(root / ".svn").exist?is more readable than the original implementation.
31-54: Git restoration logic preserved with better structure.The Git restoration logic maintains all original functionality including LFS support and error handling, but is now properly encapsulated in an instance method. The error handling and cleanup logic is preserved.
83-83: Provider initialization follows Rails conventions.Initializing the default provider at the bottom of the file follows Rails conventions and ensures the provider is available when the module is loaded.
test/unit/stable_screenshoter_test.rb (5)
8-9: Appropriate test class refactoring for unit testing.Changing from
ActionDispatch::IntegrationTesttoActiveSupport::TestCaseis correct for unit tests and improves test parallelization. The switch toCapybaraScreenshotDiff::DSLStubaligns with the new DSL infrastructure.
20-35: Well-structured test with proper mock usage.The test properly uses mocks to verify retry behavior without actual screenshot operations. The mock expectations clearly define the expected sequence of calls for stable screenshot detection.
37-47: Good argument validation tests.The tests for
nilparameter validation ensure proper error handling. Testing bothwaitandstability_time_limitparameters separately provides good coverage of edge cases.
49-69: Comprehensive cleanup verification test.The test properly verifies that temporary files are cleaned up after successful comparison. The assertions check both file existence and cleanup behavior, which is important for preventing test pollution in parallel execution.
71-117: Thorough exception handling test with proper cleanup.The test comprehensively covers the unstable image scenario with proper mock setup, exception verification, and cleanup validation. The
ensureblock prevents test pollution, which is crucial for parallel test execution.🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 82-82: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara_screenshot_diff/dsl.rb (5)
4-8: LGTM! Well-organized imports for the refactored DSL.The new require statements properly import all necessary dependencies for the enhanced screenshot comparison infrastructure.
11-19: Excellent documentation and module organization!The comprehensive documentation clearly explains the module's purpose and design. Including
ScreenshotNamerDSLprovides a clean separation of naming concerns.
21-66: Well-structured screenshot method with excellent documentation!The implementation demonstrates:
- Comprehensive parameter documentation with clear descriptions
- Defensive programming with early returns
- Clean separation between immediate and delayed validation
- Consistent boolean return values
This is a significant improvement over the previous implementation.
68-70: Good backward compatibility support!The alias ensures existing test suites can continue using
assert_matches_screenshotwithout modification.
72-89: Clean helper method with proper delegation!The private method provides good encapsulation and properly handles stack frames for accurate error reporting.
lib/capybara/screenshot/diff/screenshot_matcher.rb (7)
9-9: LGTM! Proper module dependency.The screenshot_coordinator require supports the extracted capture logic.
25-39: Excellent refactoring with clear separation of concerns!The method now has:
- Better error handling with explicit checks
- Clear flow with helper methods
- Performance optimization with early return when comparison isn't needed
- Proper stack frame handling for error reporting
47-62: Excellent error handling for window size mismatches!The method provides:
- Clear error messages showing expected vs actual sizes
- Helpful guidance for users on how to fix the issue
- Proper handling of Selenium-specific behavior
64-70: Clean option preparation logic!Good use of
AreaCalculatorto handle crop and skip area calculations, with proper driver configuration.
72-81: Great error handling for missing base screenshots!The method provides clear feedback when base screenshots are missing and includes helpful guidance on how to disable the check if needed.
83-85: Clean delegation to ScreenshotCoordinator!Good separation of concerns by delegating capture logic to a dedicated coordinator.
95-109: Well-organized option extraction!The method cleanly separates capture-related options from comparison options, making the code more maintainable.
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 107-107: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
lib/capybara_screenshot_diff/screenshot_assertion.rb (5)
33-39: Excellent addition of validate! for immediate validation!The method provides a clean way to perform immediate validation with proper exception handling and caller context for better error messages.
84-89: Good integration of ScreenshotNamer into AssertionRegistry!The screenshot namer is properly initialized and encapsulated within the registry.
104-114: Improved error handling in verify method!The changes provide:
- Defensive checks for active state
- Better error aggregation with all failure messages
- Proper caller context from the first failed assertion
122-122: Proper lifecycle management for screenshot_namer!The screenshot namer is correctly reset when the registry is cleared, and the delegator provides clean API access.
Also applies to: 141-141
1-145: Consider updating the PR title to reflect the actual changes.The PR title "test: parallelize tests" doesn't accurately reflect the extensive refactoring of the screenshot comparison infrastructure. These changes appear to be architectural improvements rather than test parallelization. Consider a more descriptive title like "refactor: modernize screenshot comparison infrastructure with DSL and improved error handling".
test/support/test_doubles.rb (2)
7-26: LGTM!The
TestPathtest double is well-structured and provides the necessary interface for simulating file paths in tests.
98-123: LGTM!The
TestPreprocessortest double correctly simulates the preprocessor interface with proper call tracking.test/unit/drivers/chunky_png_driver_test.rb (1)
1-155: Excellent test coverage and organization!The test suite is well-structured with:
- Clear separation of concerns (QuickEqualTest, DifferentTest, ColorDistanceTest, HelpersTest)
- Comprehensive coverage of edge cases including dimension changes, color distances, and tolerance thresholds
- Good test naming that clearly describes expected behavior
test/unit/drivers/vips_driver_test.rb (1)
17-17: Great improvement to test naming consistency!The updated test names are much more descriptive and clearly state the expected behavior. The change from
TestMethodsStubtoCapybaraScreenshotDiff::DSLStubaligns with the broader refactoring effort.Also applies to: 33-214
lib/capybara/screenshot/diff/image_compare.rb (2)
17-38: Excellent documentation of the optimization strategy!The detailed explanation of the multi-layered approach provides valuable context for understanding the performance optimizations and helps future maintainers.
64-77: Well-optimized quick comparison implementation!The layered approach with file size check followed by byte-by-byte comparison before falling back to image analysis provides excellent performance optimization for the common case of identical files.
| # frozen_string_literal: true | ||
|
|
There was a problem hiding this comment.
Add missing require statement for FileUtils.
The code uses FileUtils.cp but doesn't require the FileUtils library.
# frozen_string_literal: true
+require "fileutils"
+
module Capybara📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # frozen_string_literal: true | |
| # frozen_string_literal: true | |
| require "fileutils" | |
| module Capybara |
🤖 Prompt for AI Agents
In lib/capybara/screenshot/diff/test_vcs.rb at the beginning of the file (lines
1-2), add a require statement for the FileUtils library. This will ensure that
the FileUtils.cp method used later in the code is available and prevents runtime
errors due to the missing library.
| def call(base_path, new_path, options = {}) | ||
| # Load the raw images | ||
| base_img, new_img = @driver.load_images(base_path, new_path) | ||
|
|
||
| # Create a preliminary comparison with raw images | ||
| # This is used for enhanced preprocessing that needs context | ||
| Comparison.new(new_img, base_img, options, @driver, new_path, base_path) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review parameter ordering and add error handling.
There's an inconsistency in parameter ordering that could lead to confusion:
- Method signature:
call(base_path, new_path, options = {}) - Driver call:
@driver.load_images(base_path, new_path) - Comparison constructor:
Comparison.new(new_img, base_img, options, @driver, new_path, base_path)
The method accepts base_path first, but the Comparison constructor expects new_img first. This could indicate a logical inconsistency.
Additionally, there's no error handling if @driver.load_images fails or returns invalid data.
Consider adding error handling and verifying parameter consistency:
def call(base_path, new_path, options = {})
# Load the raw images
- base_img, new_img = @driver.load_images(base_path, new_path)
+ begin
+ base_img, new_img = @driver.load_images(base_path, new_path)
+ rescue StandardError => e
+ raise "Failed to load images: #{e.message}"
+ end
+
+ raise "Driver returned nil images" if base_img.nil? || new_img.nil?
# Create a preliminary comparison with raw images
# This is used for enhanced preprocessing that needs context
Comparison.new(new_img, base_img, options, @driver, new_path, base_path)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def call(base_path, new_path, options = {}) | |
| # Load the raw images | |
| base_img, new_img = @driver.load_images(base_path, new_path) | |
| # Create a preliminary comparison with raw images | |
| # This is used for enhanced preprocessing that needs context | |
| Comparison.new(new_img, base_img, options, @driver, new_path, base_path) | |
| end | |
| def call(base_path, new_path, options = {}) | |
| # Load the raw images | |
| begin | |
| base_img, new_img = @driver.load_images(base_path, new_path) | |
| rescue StandardError => e | |
| raise "Failed to load images: #{e.message}" | |
| end | |
| raise "Driver returned nil images" if base_img.nil? || new_img.nil? | |
| # Create a preliminary comparison with raw images | |
| # This is used for enhanced preprocessing that needs context | |
| Comparison.new(new_img, base_img, options, @driver, new_path, base_path) | |
| end |
🤖 Prompt for AI Agents
In lib/capybara/screenshot/diff/comparison_loader.rb around lines 22 to 29, the
call method has inconsistent parameter ordering between loading images and
creating the Comparison object, which can cause confusion or bugs. Ensure the
order of base and new images is consistent throughout by either swapping
parameters in the Comparison.new call or adjusting the load_images call
accordingly. Additionally, add error handling around @driver.load_images to
catch failures or invalid returns, raising an appropriate error or handling the
case gracefully before proceeding to create the Comparison object.
Rakefile
Outdated
| t.libs << "test" | ||
| t.libs << "lib" | ||
| t.test_files = FileList["test/**/*_test.rb"] | ||
| t.test_files = FileList["test/unit/**/*_test.rb", "test/integration/**/*_test.rb"] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if system tests should be included in the main test task.
The AI summary mentions system tests, but the main test task only includes unit and integration tests. This might be intentional for parallelization, but it's worth confirming.
Run the following script to check for system test files:
🏁 Script executed:
#!/bin/bash
# Description: Check if system test files exist that might need to be included in the test task
# Search for system test files
fd -t f "_test\.rb$" test/system
# Also check if there are any test files outside unit/integration directories
fd -t f "_test\.rb$" test | grep -v -E "test/(unit|integration)/"Length of output: 300
Include non-unit/integration tests in the main test task or add a dedicated system test task
We discovered two test files under test/capybara/... that aren’t picked up by the current t.test_files pattern. Please confirm whether these are intended as system tests and either include them in the main task or define a separate test:system (or test:capybara) task.
Tests currently not covered:
- test/capybara/screenshot/diff/image_compare_refactor_test.rb
- test/capybara/screenshot/diff_test.rb
Suggested update to Rakefile:
- t.test_files = FileList["test/unit/**/*_test.rb", "test/integration/**/*_test.rb"]
+ t.test_files = FileList[
+ "test/unit/**/*_test.rb",
+ "test/integration/**/*_test.rb",
+ "test/capybara/**/*_test.rb"
+ ]Additionally, for the new coverage task:
- Verify that your coverage tool (e.g. SimpleCov) is installed before running.
- Document the
coveragetask in your README (usage, prerequisites).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.test_files = FileList["test/unit/**/*_test.rb", "test/integration/**/*_test.rb"] | |
| t.test_files = FileList[ | |
| "test/unit/**/*_test.rb", | |
| "test/integration/**/*_test.rb", | |
| "test/capybara/**/*_test.rb" | |
| ] |
🤖 Prompt for AI Agents
In Rakefile at line 11, the main test task only includes unit and integration
tests, missing system tests located in test/capybara. Confirm if these are
system tests and either add their path pattern to t.test_files to include them
in the main test task or create a separate test:system or test:capybara task
specifically for these tests. Also, ensure the coverage task checks for the
coverage tool's installation before running and document its usage and
prerequisites in the README.
test/unit/vcs_test.rb
Outdated
| test "#restore_git_revision checks out and verifies the original screenshot" do | ||
| screenshot_path = ActiveSupport::TestCase.file_fixture_path / "files/images/a.png" | ||
|
|
||
| base_screenshot_path = Pathname.new(@base_screenshot.path) | ||
| assert Vcs.restore_git_revision(screenshot_path, base_screenshot_path, root: Screenshot.root) | ||
|
|
||
| assert Vcs::GitVcs.new.restore_git_revision(screenshot_path, base_screenshot_path, root: ActiveSupport::TestCase.file_fixture_path) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the new VCS API is properly implemented.
The change from Vcs.restore_git_revision to Vcs::GitVcs.new.restore_git_revision suggests a shift from class methods to instance methods. Ensure that all other VCS usage throughout the codebase has been updated consistently.
🏁 Script executed:
#!/bin/bash
# Description: Verify all VCS usage has been updated to use the new instance-based API
# Expected: Should find only new instance-based usage patterns
echo "Searching for old class method usage patterns..."
rg "Vcs\.restore_git_revision" --type ruby
echo "Searching for new instance method usage patterns..."
rg "Vcs::GitVcs\.new" --type ruby
echo "Searching for any remaining direct Vcs module method calls..."
rg "Vcs\.[a-z_]+" --type ruby -A 2Length of output: 3050
Update remaining VCS API usages to instance-based calls
We’ve verified that all restore_git_revision calls have been switched to Vcs::GitVcs.new…, but there are still a few spots using the old module methods:
• test/unit/dsl_test.rb
– Replace stubs on the module (Capybara::Screenshot::Diff::Vcs.stub(:checkout_vcs, …)) with stubs on the provider instance, e.g.:
ruby Capybara::Screenshot::Diff::Vcs.provider.stub(:checkout_vcs, false) do … end
(or use allow_any_instance_of(Capybara::Screenshot::Diff::Vcs::GitVcs) if you prefer RSpec syntax)
• lib/capybara_screenshot_diff/snap_manager.rb
– Change any direct calls to
ruby Capybara::Screenshot::Diff::Vcs.checkout_vcs(root, path, as_path)
into instance calls, for example:
ruby Capybara::Screenshot::Diff::Vcs.provider.checkout_vcs(path, root: root, target: as_path)
Once these are updated, all remaining VCS usage will be consistently instance-based.
🤖 Prompt for AI Agents
In test/unit/vcs_test.rb around lines 22 to 27, the test has been updated to use
the new instance-based VCS API by calling Vcs::GitVcs.new.restore_git_revision
instead of the old class method. However, other parts of the codebase still use
the old module method calls on Vcs. To fix this, update all remaining Vcs API
usages to instance-based calls: in test/unit/dsl_test.rb, replace stubs on the
Vcs module with stubs on the provider instance or use instance-based stubbing;
in lib/capybara_screenshot_diff/snap_manager.rb, replace direct calls to
Capybara::Screenshot::Diff::Vcs.checkout_vcs with calls on the provider
instance, passing arguments as keyword parameters. This ensures consistent use
of the new instance-based VCS API throughout the codebase.
| # As part of the layered comparison architecture, this class represents the final | ||
| # output of the comparison process, containing all data needed for reporting. | ||
| # Represents a difference between two images | ||
| class Difference < Struct.new(:region, :meta, :comparison, :failed_by, :base_image_path, :image_path, keyword_init: nil) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify keyword_init usage.
Setting keyword_init: nil is unusual. Typically this would be keyword_init: true to enable keyword arguments or omitted entirely for positional arguments.
Please verify the intended behavior for the Struct initialization. The nil value may not provide the expected keyword argument behavior.
🏁 Script executed:
#!/bin/bash
# Description: Check how Difference.new is called throughout the codebase
# Expected: Find usage patterns to determine if keyword or positional arguments are expected
rg -A 3 "Difference\.new"Length of output: 2276
Remove unnecessary keyword_init: nil
The keyword_init: nil option is effectively a no-op (treated as false), and all Difference.new calls in the codebase use positional arguments. It’s misleading and can be removed. If you intended to support keyword initialization, change it to keyword_init: true and update all callers accordingly.
• Location to update:
- lib/capybara/screenshot/diff/difference.rb:
- class Difference < Struct.new(:region, :meta, :comparison, :failed_by, :base_image_path, :image_path, keyword_init: nil) + class Difference < Struct.new(:region, :meta, :comparison, :failed_by, :base_image_path, :image_path)
• Call sites (no changes needed if you keep positional args):
- test/unit/difference_test.rb
- lib/capybara/screenshot/diff/drivers/vips_driver.rb
- lib/capybara/screenshot/diff/drivers/chunky_png_driver.rb
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Difference < Struct.new(:region, :meta, :comparison, :failed_by, :base_image_path, :image_path, keyword_init: nil) | |
| class Difference < Struct.new(:region, :meta, :comparison, :failed_by, :base_image_path, :image_path) |
🤖 Prompt for AI Agents
In lib/capybara/screenshot/diff/difference.rb at line 21, the struct Difference
is defined with keyword_init: nil, which is unnecessary and misleading since all
calls to Difference.new use positional arguments. Remove the keyword_init: nil
option entirely to clarify that the struct uses positional arguments. No changes
are needed in the call sites.
test/support/test_driver.rb
Outdated
| def initialize(base_image: nil, new_image: nil) | ||
| @base_image = base_image | ||
| @new_image = new_image | ||
| @dimension_result = true | ||
| @pixel_result = true | ||
| @difference_region = nil | ||
| end |
There was a problem hiding this comment.
Add super call to properly initialize parent class.
The FakeTestDriver inherits from BaseDriver but doesn't call super in its initialize method, which could lead to incomplete initialization of the parent class state.
def initialize(base_image: nil, new_image: nil)
+ super()
@base_image = base_image
@new_image = new_image
@dimension_result = true
@pixel_result = true
@difference_region = nil
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def initialize(base_image: nil, new_image: nil) | |
| @base_image = base_image | |
| @new_image = new_image | |
| @dimension_result = true | |
| @pixel_result = true | |
| @difference_region = nil | |
| end | |
| def initialize(base_image: nil, new_image: nil) | |
| super() | |
| @base_image = base_image | |
| @new_image = new_image | |
| @dimension_result = true | |
| @pixel_result = true | |
| @difference_region = nil | |
| end |
🧰 Tools
🪛 RuboCop (1.75.5)
[warning] 42-48: Call super to initialize state of the parent class.
(Lint/MissingSuper)
🤖 Prompt for AI Agents
In test/support/test_driver.rb around lines 42 to 48, the initialize method of
FakeTestDriver does not call super, which means the parent class BaseDriver's
initialization is skipped. Modify the initialize method to include a call to
super with appropriate arguments to ensure the parent class is properly
initialized before setting subclass-specific instance variables.
test/unit/image_compare_test.rb
Outdated
| class IntegrationRegressionTest < ActionDispatch::IntegrationTest | ||
| include TestMethodsStub | ||
| include CapybaraScreenshotDiff::DSLStub |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify inheritance consistency for IntegrationRegressionTest.
The IntegrationRegressionTest class still inherits from ActionDispatch::IntegrationTest but includes CapybaraScreenshotDiff::DSLStub. This seems inconsistent with the unit test class changes above. Verify if this class should also inherit from ActiveSupport::TestCase for consistency.
🏁 Script executed:
#!/bin/bash
# Description: Check if IntegrationRegressionTest uses any integration-specific functionality
# that would require ActionDispatch::IntegrationTest inheritance
# Search for integration test methods in the class
rg -A 10 -B 5 'class IntegrationRegressionTest.*ActionDispatch::IntegrationTest' test/unit/image_compare_test.rb
# Check if the class uses any ActionDispatch::IntegrationTest specific methods
rg 'get\s|post\s|put\s|delete\s|patch\s|head\s|options\s|follow_redirect|request\.|response\.' test/unit/image_compare_test.rbLength of output: 870
Change IntegrationRegressionTest to inherit from ActiveSupport::TestCase
This test class never exercises any ActionDispatch::IntegrationTest-specific behavior (no HTTP verbs, no request/response, etc.) and simply uses the DSL stub. For consistency with the other unit tests, switch its base class.
• File: test/unit/image_compare_test.rb
Lines: ~90
- class IntegrationRegressionTest < ActionDispatch::IntegrationTest
+ class IntegrationRegressionTest < ActiveSupport::TestCase
include CapybaraScreenshotDiff::DSLStub📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class IntegrationRegressionTest < ActionDispatch::IntegrationTest | |
| include TestMethodsStub | |
| include CapybaraScreenshotDiff::DSLStub | |
| class IntegrationRegressionTest < ActiveSupport::TestCase | |
| include CapybaraScreenshotDiff::DSLStub |
🤖 Prompt for AI Agents
In test/unit/image_compare_test.rb around lines 90 to 91, the
IntegrationRegressionTest class currently inherits from
ActionDispatch::IntegrationTest but does not use any integration-specific
features. To maintain consistency with other unit tests and because it only
includes the DSL stub, change its inheritance to ActiveSupport::TestCase instead
of ActionDispatch::IntegrationTest.
lib/capybara/screenshot/diff/vcs.rb
Outdated
| # frozen_string_literal: true | ||
|
|
||
| require_relative "os" | ||
| require_relative "test_vcs" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify usage of required test_vcs file.
The file requires test_vcs but doesn't appear to use it directly in this file. Ensure this requirement is necessary or if it should be moved to where TestVcs is actually used.
🏁 Script executed:
#!/bin/bash
# Description: Check where TestVcs is used to verify if the require is needed here
# Search for TestVcs usage in the codebase
rg -i 'TestVcs|test_vcs' --type rb
# Check if TestVcs is used in this file specifically
rg -i 'TestVcs' lib/capybara/screenshot/diff/vcs.rbLength of output: 115
Let's locate the test_vcs.rb file and see where (and if) TestVcs is referenced across the codebase:
🏁 Script executed:
#!/bin/bash
# 1. Find all occurrences of test_vcs.rb
files=$(find . -type f -name 'test_vcs.rb')
echo "Found test_vcs.rb files:"
echo "$files"
# 2. Preview first 50 lines of each test_vcs.rb
for f in $files; do
echo "===== Preview: $f ====="
head -n 50 "$f"
done
# 3. Search for any usage of the TestVcs constant in Ruby files
rg 'TestVcs' -n --glob '*.rb'Length of output: 1412
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' lib/capybara/screenshot/diff/vcs.rbLength of output: 2793
🏁 Script executed:
#!/bin/bash
# Display the first 100 lines of test/test_helper.rb to inspect require statements
sed -n '1,100p' test/test_helper.rbLength of output: 2733
Remove unnecessary test_vcs require
The Vcs module only ever uses the GitVcs class; it never references TestVcs internally. Since your tests already do:
require "capybara/screenshot/diff/test_vcs"
Capybara::Screenshot::Diff::Vcs.provider = Capybara::Screenshot::Diff::TestVcs.new(...)you can drop the require_relative "test_vcs" from your production code to avoid loading test helpers in normal use.
- lib/capybara/screenshot/diff/vcs.rb: remove the line
- require_relative "test_vcs"
🤖 Prompt for AI Agents
In lib/capybara/screenshot/diff/vcs.rb at line 4, the require_relative
"test_vcs" is unnecessary because the Vcs module does not use TestVcs
internally; it only uses GitVcs. Remove this require line to avoid loading test
helper code in production.
| def is_a?(klass) | ||
| return @is_vips_driver if klass == Drivers::VipsDriver | ||
| super | ||
| end |
There was a problem hiding this comment.
Avoid overriding core Ruby methods is_a? and class.
Overriding these fundamental Ruby methods can lead to unexpected behavior and break assumptions in code that uses type checking or introspection. This approach is fragile and could cause hard-to-debug issues.
Consider using a different approach such as:
- Duck typing - implement the necessary methods without faking the type
- Use a proper test double framework that handles type simulation safely
- Create a base interface that both real and test drivers implement
-def is_a?(klass)
- return @is_vips_driver if klass == Drivers::VipsDriver
- super
-end
+# Instead, consider implementing necessary VipsDriver methods
+# or using respond_to? for capability checking
-# Return Object to avoid infinite recursion
-def class
- Object
-end
+# Remove this override - let Ruby handle class properlyAlso applies to: 92-95
🤖 Prompt for AI Agents
In test/support/test_doubles.rb around lines 51 to 54 and 92 to 95, avoid
overriding core Ruby methods like is_a? and class as it can cause unexpected
behavior and break type checking. Instead, refactor the test doubles to use duck
typing by implementing only the necessary methods without faking the object's
type. Alternatively, use a test double framework that safely simulates types or
define a shared interface that both real and test driver classes implement to
ensure compatibility without overriding core methods.
| # Check if files are identical by content | ||
| def files_identical?(file1, file2) | ||
| # Compare file contents | ||
| FileUtils.identical?(file1, file2) | ||
| rescue | ||
| # If there's any error reading the files, they're not identical | ||
| false | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more specific error handling.
The broad rescue clause could hide unexpected errors. Consider catching specific exceptions that are expected when files can't be read.
def files_identical?(file1, file2)
# Compare file contents
FileUtils.identical?(file1, file2)
-rescue
+rescue Errno::ENOENT, Errno::EACCES, Errno::EISDIR => e
# If there's any error reading the files, they're not identical
false
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check if files are identical by content | |
| def files_identical?(file1, file2) | |
| # Compare file contents | |
| FileUtils.identical?(file1, file2) | |
| rescue | |
| # If there's any error reading the files, they're not identical | |
| false | |
| end | |
| # Check if files are identical by content | |
| def files_identical?(file1, file2) | |
| # Compare file contents | |
| FileUtils.identical?(file1, file2) | |
| rescue Errno::ENOENT, Errno::EACCES, Errno::EISDIR => e | |
| # If there's any error reading the files, they're not identical | |
| false | |
| end |
🤖 Prompt for AI Agents
In lib/capybara/screenshot/diff/image_compare.rb around lines 186 to 193, the
files_identical? method uses a broad rescue clause that can mask unexpected
errors. Replace the generic rescue with specific exception handling for expected
file reading errors, such as Errno::ENOENT and Errno::EACCES, to ensure only
anticipated exceptions are caught while allowing others to propagate.
pftg
force-pushed
the
test-parallelize-tests
branch
2 times, most recently
from
a4eb1b4 to
7b72c89
Compare
pftg
force-pushed
the
test-parallelize-tests
branch
from
7b72c89 to
a0dca0d
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
Chores