refact: makes tests consistent #142
Conversation
WalkthroughThis update restructures and enhances the test suite for the Capybara Screenshot Diff project. It introduces new unit test files under Changes
Sequence Diagram(s)sequenceDiagram
participant Rake
participant TestRunner
participant UnitTests
participant IntegrationTests
participant Coverage
Rake->>TestRunner: rake test
TestRunner->>IntegrationTests: Run all integration tests
Rake->>TestRunner: rake test:unit
TestRunner->>UnitTests: Run all unit tests
Rake->>Coverage: rake coverage
Coverage->>TestRunner: Set ENV["COVERAGE"] = "true"
Coverage->>TestRunner: Run all tests
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
pftg
force-pushed
the
refact-consistent-tests
branch
from
33ead33 to
30cc085
Compare
pftg
force-pushed
the
refact-consistent-tests
branch
from
30cc085 to
1409810
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
pftg
force-pushed
the
refact-consistent-tests
branch
from
1409810 to
160fe9e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR standardizes test setup and naming across the suite, consolidates helper modules, and refines the Rake test tasks for more granular invocation.
- Swaps
require_relativepaths for centralizedsupport/helpers and doubles - Renames test cases to use
#method_namestyle and updates working directories - Introduces shared helper modules and scopes tests under
test/unitandtest/integration
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/unit/image_preprocessor_test.rb | Updated requires/includes, renamed test descriptions |
| test/unit/image_compare_test.rb | Renamed tests, fixed image fixture path resolution |
| test/unit/dsl_test.rb | Renamed test names to #screenshot style |
| test/test_helper.rb | Added TestHelpers includes and expanded load path setup |
| test/support/test_helpers.rb | New helper module for assertions and driver setup |
| Rakefile | Refined default and namespaced test tasks, added coverage task |
Comments suppressed due to low confidence (2)
Rakefile:11
- The default Rake test task now excludes tests under
test/capybara; consider includingtest/**/*_test.rbor adding a pattern fortest/capybara/**/*_test.rbto avoid missing any tests.
t.test_files = FileList["test/unit/**/*_test.rb", "test/integration/**/*_test.rb"]
test/unit/dsl_test.rb:83
- [nitpick] The regex
%r{/dsl_test.rb}is quite broad and may match unintended frames; adding the specific test method name or line number would make this assertion more precise.
assert_match(
%r{/dsl_test.rb},
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/unit/image_preprocessor_test.rb (1)
50-50: Use consistent driver variable naming.This test uses a local
drivervariable rather than the instance@driverused elsewhere, which can be confusing. Consider consistently using@driveror local variables across all tests.
🧹 Nitpick comments (2)
test/support/test_helpers.rb (2)
41-45: Consider simplifying the conditional logic.The conditional assignments could be streamlined for better readability.
def setup_driver_results(driver, same_dimension: true, same_pixels: nil, difference_region: nil) driver.same_dimension_result = same_dimension - driver.same_pixels_result = same_pixels unless same_pixels.nil? - driver.difference_region_result = difference_region if difference_region + driver.same_pixels_result = same_pixels if same_pixels + driver.difference_region_result = difference_region if difference_region end
54-56: Consider making the factory method more flexible.The direct instantiation limits flexibility for testing different driver implementations.
Consider making this more flexible by accepting a driver class parameter:
-def create_test_driver(is_vips: false, images: nil) - Capybara::Screenshot::Diff::TestDoubles::TestDriver.new(is_vips, images) +def create_test_driver(is_vips: false, images: nil, driver_class: Capybara::Screenshot::Diff::TestDoubles::TestDriver) + driver_class.new(is_vips, images) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
Rakefile(2 hunks)test/capybara/screenshot/diff/area_calculator_test.rb(0 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(0 hunks)test/capybara/screenshot/diff/difference_finder_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/support/test_helpers.rb(1 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(9 hunks)test/unit/dsl_test.rb(9 hunks)test/unit/image_compare_test.rb(5 hunks)test/unit/image_preprocessor_test.rb(2 hunks)test/unit/region_test.rb(4 hunks)test/unit/rspec_test.rb(1 hunks)test/unit/screenshot_namer_test.rb(3 hunks)test/unit/screenshot_test.rb(1 hunks)test/unit/stable_screenshoter_test.rb(3 hunks)test/unit/vcs_test.rb(1 hunks)
💤 Files with no reviewable changes (5)
- test/capybara/screenshot/diff/area_calculator_test.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
- test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb
- test/capybara/screenshot/diff/difference_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/capybara/screenshot/diff/image_compare_refactor_test.rb (3)
test/test_helper.rb (1)
include(31-71)lib/capybara/screenshot/diff/reporters/default.rb (1)
comparison(117-119)lib/capybara/screenshot/diff/image_compare.rb (2)
processed(111-115)reporter(103-105)
test/unit/dsl_test.rb (2)
test/unit/drivers/chunky_png_driver_test.rb (5)
test(14-24)test(26-85)test(87-130)test(132-137)make_comparison(139-142)test/support/test_doubles.rb (1)
exist?(23-25)
🔇 Additional comments (50)
test/support/test_helpers.rb (2)
1-6: LGTM! Good module structure and organization.The TestHelpers module is well-organized with clear separation of concerns and proper inclusion of dependencies.
8-32: Well-documented assertion methods with consistent API.The assertion methods follow a consistent pattern with good documentation and descriptive error messages. The default parameter of
times = 1is sensible for most use cases.test/unit/vcs_test.rb (1)
22-22: Excellent improvement to test method naming.The new test name is much more descriptive and clearly indicates both the method being tested (
#restore_git_revision) and the expected behavior. This aligns well with the PR objective of making tests consistent.test/unit/rspec_test.rb (1)
7-7: Great improvement to test method naming for clarity.The new test name clearly describes the integration being tested and the expected outcome, making it much more descriptive than the previous generic name.
test/test_helper.rb (4)
29-29: Good integration of the new test helpers.Requiring the new test_helpers support file enables the use of the helper modules throughout the test suite.
32-35: Excellent modular inclusion of test helpers.Including the three TestHelpers modules separately maintains clear separation of concerns while making all helper methods available to test cases. The descriptive comment helps clarify the purpose of this section.
49-49: Improved skip message clarity.The updated skip message is more descriptive and clearly indicates how to enable the optional tests.
53-54: Good encapsulation by making helper method private.Making
fixture_image_path_fromexplicitly private improves encapsulation and clearly indicates this is an internal helper method.test/unit/difference_test.rb (1)
8-18:✅ Verification successful
Verify the test logic for empty different_dimensions array.
The test setup creates a
Differenceinstance withdifferent_dimensions: [](empty array), but then asserts that both#different?and#failed?returntrue. This seems counterintuitive - typically an empty difference would suggest the images are identical.Please verify that this is the intended behavior of the
Differenceclass. If not, consider updating the test setup to use actual difference data that would logically result intruefor both predicates.
🏁 Script executed:
#!/bin/bash # Description: Verify the Difference class implementation to understand the expected behavior with empty different_dimensions # Search for the Difference class implementation ast-grep --pattern 'class Difference $$$ end' # Search for the different? method implementation ast-grep --pattern 'def different? $$$ end' # Search for the failed? method implementation ast-grep --pattern 'def failed? $$$ end'Length of output: 6495
🏁 Script executed:
#!/bin/bash # Search for references to different_dimensions in the repository rg -n 'different_dimensions'Length of output: 774
No changes needed—tests align with implementation
The
Differenceclass treats any non-nilfailed_byvalue (even an empty array) as a failure, anddifferent?short-circuitsblank? || tolerable?, leavingfailed?to drive the result. Your tests correctly asserttruefor both predicates withdifferent_dimensions: [].test/unit/drivers/vips_driver_test.rb (1)
33-201: Excellent improvement to test method naming.The test method renamings significantly improve clarity by explicitly describing the expected behavior. The new names follow a consistent pattern of
#method_name behavior_description, making it much easier to understand what each test is verifying.For example:
"#different? returns false when comparing identical images"is much clearer than generic descriptions"#quick_equal? respects color_distance_limit when above difference threshold"clearly describes both the method and the condition being testedThese changes enhance test maintainability and readability without altering any test logic.
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 131-131: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 144-144: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
test/unit/comparison_loader_test.rb (1)
1-61: Well-structured unit test suite with comprehensive coverage.This new test file demonstrates excellent testing practices:
- Good test organization: Clear setup method with appropriate test fixtures
- Comprehensive coverage: Tests verify return types, attribute assignment, option forwarding, and driver interactions
- Proper isolation: Uses TestDoubles for mocking dependencies
- Clear naming: Test method names clearly describe the behavior being verified
- Focused testing: Each test has a single responsibility and clear assertions
The test suite thoroughly validates the
ComparisonLoaderfunctionality while maintaining good separation of concerns.test/unit/image_compare_test.rb (3)
18-87: Improved test method naming enhances clarity.The test method renamings follow the same excellent pattern seen in other test files, making the test intent much clearer. Names like
"#initialize creates instance with chunky_png driver by default"and"#different? with vips driver generates annotated diff images"clearly communicate what each test is verifying.
98-98: Good fix for incorrect directory path.The path correction from
"../../../images"to"../images"appears to fix an incorrect relative path. This change aligns the path with the actual directory structure relative to the test file location.
95-134: Clear and descriptive test method naming.The renaming of these integration test methods improves readability by clearly stating the expected behavior:
"identical images are quick_equal and not different across all drivers"and"different images are not quick_equal and are marked as different".test/unit/region_test.rb (1)
7-61: Excellent test method naming improvements!The test method names have been consistently updated to follow Ruby testing conventions and provide clear descriptions of the expected behavior. The use of
#for instance methods and.for class methods, along with descriptive outcomes, significantly improves test readability and maintainability.Rakefile (3)
11-11: Good test file organization.The explicit specification of test directories (
test/unit/**/*_test.rb,test/integration/**/*_test.rb) provides clearer control over which tests are executed and aligns with the new test structure.
14-24: Excellent addition of granular test tasks.The separate
test:unitandtest:integrationtasks provide valuable flexibility for developers to run specific test suites independently, which is especially useful during development and CI workflows.
26-30: Useful coverage task addition.The coverage task provides a convenient way to run all tests with coverage enabled, making it easy to generate coverage reports for the entire test suite.
test/unit/screenshot_test.rb (1)
8-26: Great modernization of test syntax!The conversion from old-style
def test_*methods to the moderntest "description" doblock style significantly improves readability. The test descriptions are clear and descriptive.The conditional cleanup on line 25 (
if @orig_root) is a good defensive programming practice that prevents potential errors if the original root was never set.test/unit/area_calculator_test.rb (1)
1-77: Excellent comprehensive test suite for AreaCalculator!This new test file demonstrates excellent testing practices:
- Well-organized structure: The nested test classes (
CalculateSkipAreaTest,InitializationTest,EdgeCaseTest) logically group related functionality- Comprehensive coverage: Tests handle normal operation, edge cases, different input formats, and error conditions
- Clear test descriptions: Each test clearly states what behavior is being verified
- Proper assertions: Uses appropriate assertion methods and validates expected behavior
- Consistent style: Follows the modern test block style used throughout the refactored test suite
The tests properly verify:
- Area calculation with and without overlaps
- Handling of different input types (Region objects vs arrays)
- Edge cases like empty arrays and nil values
- Type conversions and coordinate validation
test/unit/image_preprocessor_test.rb (4)
4-6: LGTM: Improved require structure and test helpers.The updates to use absolute require paths and switch from
TestDoublestoTestHelpersalign well with the test consistency goals of this PR.
12-12: LGTM: Consistent use of test helpers and instance variables.Good refactoring to use
TestHelpersand the@driverinstance variable created in setup for better test consistency.Also applies to: 16-16
19-27: LGTM: Improved test naming and consistency.The test name is now more descriptive and the consistent use of
@driverthroughout the test improves readability and maintainability.
69-87: LGTM: Improved test clarity and consistency.The test name is more descriptive and the consistent use of
@driverthroughout maintains the pattern established in other tests.test/unit/screenshot_namer_test.rb (1)
80-80: LGTM: Improved test naming for clarity.The updated test names are more descriptive and clearly communicate what each test verifies. This enhances readability and aligns with the PR's goal of making tests consistent.
Also applies to: 108-108, 115-115, 151-151
test/capybara/screenshot/diff/image_compare_refactor_test.rb (4)
5-6: LGTM: Improved require structure and test helpers.The switch to absolute require paths and the inclusion of
TestHelpersinstead ofTestDoublesmaintains consistency with other refactored test files.Also applies to: 13-13
15-24: LGTM: Excellent test separation and clarity.Breaking the combined tests into focused individual tests significantly improves test clarity. Each test now has a single responsibility, making failures easier to diagnose and understand.
26-35: LGTM: Clear test structure with descriptive naming.The grouping comments and descriptive test names make it easy to understand what functionality is being tested. The separation of positive and negative test cases is a best practice.
37-57: LGTM: Comprehensive test coverage with good organization.The tests cover both the
dimensions_changed?method and the reporter configuration. The addition of the explicitprocessedcall and the verification of the default reporter type adds valuable test coverage.test/unit/drivers/utils_test.rb (1)
11-11: LGTM: Improved test naming with consistent convention.The updated test names follow a consistent
#method_namestyle and clearly describe the conditions under which drivers are included or excluded. This enhances test readability and aligns perfectly with the PR's consistency goals.Also applies to: 18-18, 26-26, 34-34, 42-42, 48-48
test/unit/stable_screenshoter_test.rb (4)
20-20: Excellent test naming improvement.The renamed test method is much more descriptive and clearly indicates the expected behavior of retrying until images stabilize across iterations.
37-37: Great improvement in test specificity.The updated test names now explicitly state that
ArgumentErroris raised when parameters arenil, making the test expectations much clearer than the previous generic "any error" descriptions.Also applies to: 43-43
49-49: Clear and descriptive test naming.The new test name effectively communicates the cleanup behavior after successful comparison, improving test documentation.
71-71: Precise exception type specification.The updated test name now clearly indicates that
UnstableImageexception is raised when the stability timeout is reached, which is much more specific and helpful than the previous naming.test/unit/drivers/chunky_png_driver_test.rb (5)
11-13: Good test organization and setup.The use of
ActiveSupport::TestCaseas the base class and inclusion of the DSL stub is appropriate for unit testing.
14-24: Excellent use of nested test classes.The
QuickEqualTestnested class provides good organization and isolation for testing the quick equality functionality.
38-48: Comprehensive difference detection testing.The test effectively validates both the difference detection logic and the generation of annotated comparison images, including proper file existence checks.
87-130: Thorough color distance testing.The
ColorDistanceTestclass provides excellent coverage of color distance thresholds and edge cases, including minimal color differences.
139-151: Well-structured helper methods.The helper methods
make_comparison,sample_region, andload_test_imageprovide good test utilities and maintain clean test organization.test/unit/difference_finder_test.rb (5)
10-12: Appropriate test inheritance and mixins.Using
ActionDispatch::IntegrationTestas the base class and including both DSL stub and test doubles provides good testing infrastructure.
14-42: Well-structured initialization testing.The
InitializationTestclass properly tests the constructor with good use of test doubles and clear assertions for driver and options assignment.
56-65: Excellent quick mode testing with proper assertions.The test effectively validates both the return value and the difference object, while also asserting that the expected driver methods are called.
115-124: Comprehensive full mode dimension testing.The test properly validates the behavior when image dimensions differ, including checking the failed status and verifying method call counts.
162-170: Good helper method organization.The helper methods are appropriately placed at the class level and provide clean test setup utilities.
test/unit/dsl_test.rb (6)
12-12: Excellent test name improvement.The new test name clearly describes the error condition (missing screenshot + fail_if_new true) and the expected behavior, making the test much more self-documenting.
22-22: Great improvement in test specificity.The updated test names now clearly indicate what aspect of the error message is being tested (basic mismatch vs. shift distance inclusion), improving test documentation significantly.
Also applies to: 35-35
52-52: Clear driver option testing description.The new test name effectively communicates that it's testing driver options support for image comparison functionality.
61-61: Descriptive stack frame testing names.Both test names now clearly describe the different skip_stack_frames behavior and what should appear in the caller, making the test intentions much clearer.
Also applies to: 74-74
90-90: Precise delayed option testing descriptions.The test names now clearly distinguish between error and success cases when using
delayed: false, improving test readability and understanding.Also applies to: 101-101
110-110: Comprehensive and descriptive test naming.All these test names are now much more specific about what functionality they're testing (option acceptance, file creation, cleanup behavior), greatly improving the test suite's self-documentation.
Also applies to: 114-114, 121-121, 127-127
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/unit/diff_test.rb (1)
143-143: Consider updating nested classes for consistency.While the main
DiffTestclass has been updated to inherit fromActiveSupport::TestCase, the nested classesSampleMiniTestCaseandScreenshotFormatTeststill inherit fromActionDispatch::IntegrationTest. For full consistency with the refactoring objectives, consider whether these should also be updated.If these nested classes don't require integration test capabilities, apply this diff:
- class SampleMiniTestCase < ActionDispatch::IntegrationTest + class SampleMiniTestCase < ActiveSupport::TestCase - class ScreenshotFormatTest < ActionDispatch::IntegrationTest + class ScreenshotFormatTest < ActiveSupport::TestCaseHowever, if they specifically need integration test functionality, the current approach is acceptable.
Also applies to: 195-195
test/fixtures/rspec_spec.rb (2)
8-11: Consider simplifying the conditional require pattern.The conditional loading based on
SCREEN_SIZEconstant seems unnecessary and could make the test dependencies unclear. Consider either requiring all dependencies unconditionally or using a more explicit setup pattern.-unless defined?(SCREEN_SIZE) - require "test_helper" - require "support/setup_capybara_drivers" -end +require "test_helper" +require "support/setup_capybara_drivers"
42-44: Consider making the conflict test more comprehensive.While this test ensures no conflicts with standard RSpec methods, it's quite minimal. Consider testing additional RSpec functionality to ensure broader compatibility.
it "does not conflicts with rspec methods" do expect { raise StandardError }.to raise_error(StandardError) + expect(true).to be_truthy + expect([1, 2, 3]).to include(2) + expect { sleep 0.01 }.not_to raise_error end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
test/fixtures/comparisons/a-and-b.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-b.heatmap.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-c.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/b-and-a.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/c-and-a.diff.pngis excluded by!**/*.png
📒 Files selected for processing (9)
test/capybara/screenshot/diff/image_compare_refactor_test.rb(0 hunks)test/fixtures/rspec_spec.rb(1 hunks)test/integration/rspec_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/test_helper.rb(2 hunks)test/unit/diff_test.rb(1 hunks)test/unit/dsl_test.rb(8 hunks)test/unit/image_compare_test.rb(6 hunks)test/unit/vcs_test.rb(2 hunks)
💤 Files with no reviewable changes (1)
- test/capybara/screenshot/diff/image_compare_refactor_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/support/capybara_screenshot_diff/dsl_stub.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- test/test_helper.rb
- test/unit/vcs_test.rb
- test/unit/image_compare_test.rb
- test/unit/dsl_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/fixtures/rspec_spec.rb (10)
lib/capybara/screenshot/diff/browser_helpers.rb (1)
resize_to(14-20)test/integration/browser_screenshot_test.rb (1)
window_size(255-261)lib/capybara/screenshot/diff/reporters/default.rb (1)
driver(113-115)lib/capybara/screenshot/diff/difference.rb (1)
tolerance(49-51)test/support/test_doubles.rb (1)
class(93-95)test/unit/diff_test.rb (1)
include(143-161)test/unit/vcs_test.rb (1)
include(8-31)test/unit/dsl_test.rb (1)
include(8-142)test/unit/image_compare_test.rb (3)
include(15-88)include(90-139)include(141-188)test/test_helper.rb (1)
include(31-71)
🔇 Additional comments (8)
test/unit/diff_test.rb (1)
9-9:❓ Verification inconclusive
LGTM! Superclass change aligns with test refactoring objectives.
The change from
ActionDispatch::IntegrationTesttoActiveSupport::TestCaseappropriately moves this test class from integration-style to unit-style testing, which aligns with the PR's goal of making tests consistent. The test methods focus on unit-level functionality of the screenshot diff module rather than full integration scenarios.However, please verify that all test functionality remains intact after this change, as there may be subtle differences in test lifecycle or available helpers between the two superclasses.
🏁 Script executed:
#!/bin/bash # Description: Run the specific test file to ensure all tests still pass # Expected result: All tests should pass successfully ruby -Itest test/unit/diff_test.rbLength of output: 839
Approve superclass change but ensure consistency and verify tests
- The change from
ActionDispatch::IntegrationTesttoActiveSupport::TestCaseintest/unit/diff_test.rbcorrectly reflects the shift from integration-style to unit-style testing.- For consistency, consider updating the nested test classes in this file:
SampleMiniTestCaseScreenshotFormatTest
to also inherit fromActiveSupport::TestCaseif they don’t rely on integration helpers.- The sandbox run failed with a missing
rackuperror. Please verify in your local Rails environment that all tests still pass, for example:# In your project root bundle install RAILS_ENV=test bundle exec rails test test/unit/diff_test.rbtest/integration/rspec_test.rb (2)
7-7: Excellent improvement to test naming.The new test name is much more descriptive and clearly communicates what the test is verifying. This follows good testing practices and improves maintainability.
13-13: Good use of Rails test helper.Using
file_fixtureinstead of manual path joining is the Rails-idiomatic way to handle fixture files. This is more robust and handles path resolution correctly.test/fixtures/rspec_spec.rb (5)
14-25: Comprehensive setup configuration looks good.The before block properly configures all necessary Capybara and screenshot settings for the tests. The configuration is thorough and covers driver setup, paths, and tolerance settings.
27-29: Good test for DSL inclusion verification.This test correctly verifies that the DSL module is properly included in the RSpec context, which is essential for the gem's functionality.
36-40: Excellent test for custom matcher functionality.This test validates the custom
match_screenshotmatcher with options, which is a key feature of the gem. The test properly passes options likeskip_stack_framesanddriver.
24-24:✅ Verification successful
Verify the tolerance value is appropriate.
The tolerance value of 0.5 seems quite high for screenshot comparisons. This might allow significant visual differences to pass undetected.
🏁 Script executed:
#!/bin/bash # Check what tolerance values are used in other test files to ensure consistency rg "tolerance.*=" --type rb -A 2 -B 2Length of output: 66
🏁 Script executed:
#!/bin/bash # Search for tolerance assignments across all Ruby files grep -R -n "tolerance *= *" --include="*.rb" -C2 .Length of output: 3413
No change needed: fixture’s tolerance is intentionally context-specific
The
0.5value intest/fixtures/files/rspec_spec.rb:24is only used to drive the fixture’s diff example. Other tests deliberately pick different tolerances (e.g. 0.035, 0.13, 0.7 ornil) based on driver and test purpose, so there’s no requirement to align the fixture’s value with those defaults.
31-34:✅ Verification successful
Verify screenshot cleanup after test execution.
The test creates a screenshot but doesn't explicitly verify cleanup. Ensure that the test framework or teardown properly cleans up generated screenshots.
🏁 Script executed:
#!/bin/bash # Check if there are teardown mechanisms for cleaning up screenshots in RSpec tests rg -A 10 -B 5 "teardown|after.*do" test/fixtures/rspec_spec.rb test/support/ test/test_helper.rbLength of output: 4103
🏁 Script executed:
#!/bin/bash # Output the contents of non_minitest_assertions.rb to understand teardown integration sed -n '1,200p' test/support/non_minitest_assertions.rbLength of output: 544
🏁 Script executed:
#!/bin/bash sed -n '1,200p' test/fixtures/rspec_spec.rbLength of output: 1380
🏁 Script executed:
#!/bin/bash # Display contents of stub_test_methods.rb to verify RSpec teardown integration sed -n '1,200p' test/support/stub_test_methods.rbLength of output: 170
🏁 Script executed:
#!/bin/bash sed -n '1,200p' test/support/screenshoter_stub.rbLength of output: 849
Teardown cleanup verified
Existing teardown hooks clean up generated screenshots after each test run:
- In Minitest (test/test_helper.rb):
teardown do CapybaraScreenshotDiff::SnapManager.cleanup! unless persist_comparisons? end- In RSpec (test/support/capybara_screenshot_diff/dsl_stub.rb):
teardown do @manager.cleanup! Capybara::Screenshot::Diff.screenshoter = Capybara::Screenshot::Screenshoter CapybaraScreenshotDiff.reset endNo further action required.
pftg
force-pushed
the
refact-consistent-tests
branch
from
341ed2b to
70d7a3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/unit/drivers/chunky_png_driver_test.rb (2)
118-129: Consider improving test isolation and avoiding direct file system dependencies.This test directly manipulates a ChunkyPNG image and saves it to the file system, which could lead to test coupling and potential race conditions in parallel test execution.
Consider using a more isolated approach:
-test "#max_color_distance detects minimal color differences between images" do - a_img = ChunkyPNG::Image.from_blob(File.binread("#{TEST_IMAGES_DIR}/a.png")) - a_img[9, 6] += 0x010000 - - comp = make_comparison(:a, :b) - other_img_filename = comp.image_path - a_img.save(other_img_filename) - - comp.different? - - assert_includes comp.error_message, "1" -end +test "#max_color_distance detects minimal color differences between images" do + # Create a test image fixture with minimal differences instead + # or use a dedicated test image pair designed for this scenario + comp = make_comparison(:a, :minimal_diff_image) + comp.different? + + assert_includes comp.error_message, "1" +end
139-150: Consider consolidating duplicate helper methods.The
make_comparison,sample_region, andload_test_imagehelper methods are specific to this test class but could potentially be shared across test files.Consider moving common test helpers to the
TestHelpersmodule mentioned in the test support files to promote reusability across the test suite.test/unit/difference_finder_test.rb (1)
14-16: Remove redundant include statements in nested test classes.The nested test classes are including
CapybaraScreenshotDiff::DSLStubandTestDoublesmodules that are already included in the parent class, making these includes redundant.class InitializationTest < self - include CapybaraScreenshotDiff::DSLStub - include TestDoublesApply the same change to
QuickModeTestandFullModeTestclasses.Also applies to: 44-46, 103-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (90)
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/comparisons/a-and-b.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-b.heatmap.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-c.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/b-and-a.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/c-and-a.diff.pngis excluded by!**/*.pngtest/fixtures/images/a.pngis excluded by!**/*.pngtest/fixtures/images/a_cropped.pngis excluded by!**/*.pngtest/fixtures/images/b.pngis excluded by!**/*.pngtest/fixtures/images/c.pngis excluded by!**/*.pngtest/fixtures/images/d.pngis excluded by!**/*.pngtest/fixtures/images/portrait.pngis excluded by!**/*.pngtest/fixtures/images/portrait_b.pngis excluded by!**/*.png
📒 Files selected for processing (30)
.gitignore(1 hunks)Rakefile(2 hunks)scripts/benchmark/find_region_benchmark.rb(1 hunks)test/capybara/screenshot/diff/area_calculator_test.rb(0 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(0 hunks)test/capybara/screenshot/diff/difference_finder_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(0 hunks)test/integration/rspec_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/support/test_helpers.rb(1 hunks)test/test_helper.rb(3 hunks)test/unit/area_calculator_test.rb(1 hunks)test/unit/comparison_loader_test.rb(1 hunks)test/unit/diff_test.rb(9 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(8 hunks)test/unit/image_compare_test.rb(6 hunks)test/unit/image_preprocessor_test.rb(2 hunks)test/unit/region_test.rb(4 hunks)test/unit/screenshot_namer_test.rb(3 hunks)test/unit/screenshot_test.rb(1 hunks)test/unit/screenshoter_test.rb(1 hunks)test/unit/stable_screenshoter_test.rb(4 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/image_compare_refactor_test.rb
- test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- scripts/benchmark/find_region_benchmark.rb
- test/unit/screenshot_namer_test.rb
🚧 Files skipped from review as they are similar to previous changes (17)
- test/integration/rspec_test.rb
- test/unit/drivers/vips_driver_test.rb
- test/support/capybara_screenshot_diff/dsl_stub.rb
- test/unit/vcs_test.rb
- test/unit/image_preprocessor_test.rb
- test/unit/image_compare_test.rb
- Rakefile
- test/unit/diff_test.rb
- test/unit/region_test.rb
- test/unit/difference_test.rb
- test/test_helper.rb
- test/unit/screenshot_test.rb
- test/support/test_helpers.rb
- test/unit/area_calculator_test.rb
- test/unit/drivers/utils_test.rb
- test/unit/stable_screenshoter_test.rb
- test/unit/dsl_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/unit/drivers/chunky_png_driver_test.rb (6)
lib/capybara/screenshot/diff/image_compare.rb (2)
quick_equal?(64-77)reporter(103-105)test/support/test_doubles.rb (1)
exist?(23-25)lib/capybara/screenshot/diff/reporters/default.rb (4)
base_image_path(105-107)image_path(109-111)save(72-74)driver(113-115)test/test_helper.rb (1)
assert_same_images(62-65)lib/capybara/screenshot/diff/difference.rb (1)
options(45-47)test/support/capybara_screenshot_diff/dsl_stub.rb (1)
create_snapshot_for(57-63)
test/unit/difference_finder_test.rb (2)
lib/capybara/screenshot/diff/difference.rb (2)
failed?(41-43)equal?(37-39)test/support/test_helpers.rb (4)
setup_driver_results(41-45)assert_dimension_check_called(12-15)assert_pixel_check_called(20-23)assert_difference_region_called(28-31)
🔇 Additional comments (4)
test/unit/screenshoter_test.rb (1)
8-8: Excellent refactoring to improve test consistency and isolation.The change from
ActionDispatch::IntegrationTesttoActiveSupport::TestCasealigns perfectly with the PR's objective to make tests consistent. This unit test approach is well-suited for testing theScreenshoterclass functionality in isolation, with proper mocking and stubbing replacing the integration setup. The tests should run faster while maintaining good coverage of the core functionality.test/unit/comparison_loader_test.rb (1)
1-61: Well-structured unit test file with good test isolation.The test file demonstrates excellent practices:
- Proper use of test doubles for isolation
- Clear test method names that describe the behavior being tested
- Good setup with instance variables
- Focused assertions that verify specific behaviors
The tests effectively cover the key responsibilities of
ComparisonLoader#call: returning aComparisonwith correct attributes, loading images in proper order, forwarding options, and using the driver correctly.test/unit/drivers/chunky_png_driver_test.rb (1)
1-155: Comprehensive test coverage with good organization.The test file provides excellent coverage of the ChunkyPNG driver functionality:
- Well-organized nested test classes for different aspects
- Good test names that clearly describe expected behavior
- Proper assertions for both success and failure cases
- Tests cover edge cases like dimension mismatches and color distance limits
test/unit/difference_finder_test.rb (1)
1-174: Well-structured unit tests with good coverage of different modes.The test file effectively covers the
DifferenceFinderfunctionality:
- Clear separation between initialization, quick mode, and full mode testing
- Good use of test doubles for isolation
- Proper assertions for both boolean results and object types
- Tests verify that driver methods are called appropriately
pftg
force-pushed
the
refact-consistent-tests
branch
from
70d7a3f to
19bde22
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR restructures the test suite for consistency, cleans up old test files, and updates benchmark paths and Rake tasks to reflect the new directory layout.
- Removed legacy tests under
test/capybara/screenshot/diff/in favor of a unifiedtest/unit/test/integrationstructure - Updated the benchmark’s image directory to
test/fixtures/images - Split Rake tasks into separate
test:unit,test:integration, and added acoveragetask
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/capybara/screenshot/diff/*.rb | Deleted old diff-related tests (migrated to new test structure) |
| scripts/benchmark/find_region_benchmark.rb | Updated TEST_IMAGES_DIR to use test/fixtures/images |
| Rakefile | Changed default :test file list, added test:unit, test:integration, and a coverage task |
Comments suppressed due to low confidence (3)
test/capybara/screenshot/diff/image_compare_refactor_test.rb:1
- This test file was deleted; ensure its scenarios are migrated into the new
test/unitortest/integrationdirectories so coverage is not lost.
-
Rakefile:11
- [nitpick] The default task now only includes unit and integration tests; confirm that all relevant test types are covered or intentionally excluded.
t.test_files = FileList["test/unit/**/*_test.rb", "test/integration/**/*_test.rb"]
scripts/benchmark/find_region_benchmark.rb:11
- The fixtures path was updated; verify any documentation or CI scripts referencing
test/imagesare also updated to avoid broken paths.
TEST_IMAGES_DIR = Pathname.new(File.expand_path("../../test/fixtures/images", __dir__))
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/unit/difference_finder_test.rb (1)
35-42: Remove duplicate helper method implementations.The
setup_test_comparisonmethod is duplicated across multiple nested test classes and at the class level. This violates the DRY principle and makes maintenance harder.Consolidate the duplicate helper methods by keeping a single implementation at the main test class level and removing all duplicate implementations from the nested classes at lines 90-96, 147-153, and 160-166.
🧹 Nitpick comments (1)
test/unit/drivers/chunky_png_driver_test.rb (1)
1-156: Comprehensive test suite with excellent coverage!This test file provides thorough coverage of the ChunkyPNGDriver functionality with well-organized nested test classes. The structure effectively groups related tests and covers important scenarios including:
- Image equality checks with various tolerance settings
- Difference detection with dimension and pixel variations
- Color distance calculations across different image comparisons
- Edge cases like single-pixel differences and threshold enforcement
The use of helper methods and integration with DSLStub provides realistic test scenarios.
Consider splitting this comprehensive test suite into smaller, more focused files if it grows further. While the current organization with nested classes works well, separate files for QuickEqual, Different, ColorDistance, and Helpers functionality could improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
test/fixtures/comparisons/a-and-b.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-b.heatmap.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-c.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/b-and-a.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/c-and-a.diff.pngis excluded by!**/*.pngtest/fixtures/images/a.pngis excluded by!**/*.pngtest/fixtures/images/a_cropped.pngis excluded by!**/*.pngtest/fixtures/images/b.pngis excluded by!**/*.pngtest/fixtures/images/c.pngis excluded by!**/*.pngtest/fixtures/images/d.pngis excluded by!**/*.pngtest/fixtures/images/portrait.pngis excluded by!**/*.pngtest/fixtures/images/portrait_b.pngis excluded by!**/*.png
📒 Files selected for processing (31)
.gitignore(1 hunks)Rakefile(2 hunks)scripts/benchmark/find_region_benchmark.rb(1 hunks)test/capybara/screenshot/diff/area_calculator_test.rb(0 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(0 hunks)test/capybara/screenshot/diff/difference_finder_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(0 hunks)test/integration/rspec_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/support/test_helpers.rb(1 hunks)test/system_test_case.rb(1 hunks)test/test_helper.rb(3 hunks)test/unit/area_calculator_test.rb(1 hunks)test/unit/comparison_loader_test.rb(1 hunks)test/unit/diff_test.rb(9 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(8 hunks)test/unit/image_compare_test.rb(6 hunks)test/unit/image_preprocessor_test.rb(2 hunks)test/unit/region_test.rb(4 hunks)test/unit/screenshot_namer_test.rb(3 hunks)test/unit/screenshot_test.rb(1 hunks)test/unit/screenshoter_test.rb(1 hunks)test/unit/stable_screenshoter_test.rb(4 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/image_compare_refactor_test.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
✅ Files skipped from review due to trivial changes (3)
- test/system_test_case.rb
- test/unit/screenshot_namer_test.rb
- test/support/test_helpers.rb
🚧 Files skipped from review as they are similar to previous changes (19)
- .gitignore
- test/unit/screenshoter_test.rb
- scripts/benchmark/find_region_benchmark.rb
- test/unit/region_test.rb
- test/integration/rspec_test.rb
- test/unit/difference_test.rb
- test/unit/vcs_test.rb
- test/support/capybara_screenshot_diff/dsl_stub.rb
- Rakefile
- test/unit/image_preprocessor_test.rb
- test/unit/drivers/vips_driver_test.rb
- test/unit/area_calculator_test.rb
- test/test_helper.rb
- test/unit/image_compare_test.rb
- test/unit/diff_test.rb
- test/unit/drivers/utils_test.rb
- test/unit/screenshot_test.rb
- test/unit/stable_screenshoter_test.rb
- test/unit/dsl_test.rb
🔇 Additional comments (2)
test/unit/comparison_loader_test.rb (1)
1-61: Excellent unit test implementation!This test file demonstrates solid testing practices:
- Clean structure with proper namespacing and module organization
- Effective use of test doubles for dependency isolation
- Comprehensive coverage of the
ComparisonLoader#callmethod functionality- Clear, descriptive test names that document expected behavior
- Proper assertions validating both return values and method interactions
The tests cover all critical aspects: object instantiation, attribute assignment, image loading order, and option forwarding.
test/unit/difference_finder_test.rb (1)
14-174: Well-structured test organization with comprehensive coverage.The nested test class approach effectively organizes tests by functionality (Initialization, QuickMode, FullMode). The test coverage is thorough, validating both success and failure scenarios across different modes of operation.
The use of test doubles provides good isolation, and the assertions properly verify both return values and method call expectations.
pftg
force-pushed
the
refact-consistent-tests
branch
2 times, most recently
from
98647b2 to
5789467
Compare
pftg
force-pushed
the
refact-consistent-tests
branch
from
5789467 to
33255cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/unit/image_preprocessor_test.rb (1)
14-18: Consider consistent driver variable usage.The setup method uses
@driver = create_test_driveras an instance variable, but some tests create localdrivervariables (e.g., line 51). For consistency and to avoid confusion, consider using the instance variable@driverthroughout all tests.- @driver = create_test_driver(is_vips: true) + # Use existing @driver and recreate if needed for VIPS-specific tests + @driver = create_test_driver(is_vips: true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
test/fixtures/comparisons/a-and-b.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-b.heatmap.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/a-and-c.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/b-and-a.diff.pngis excluded by!**/*.pngtest/fixtures/comparisons/c-and-a.diff.pngis excluded by!**/*.pngtest/fixtures/images/a.pngis excluded by!**/*.pngtest/fixtures/images/a_cropped.pngis excluded by!**/*.pngtest/fixtures/images/b.pngis excluded by!**/*.pngtest/fixtures/images/c.pngis excluded by!**/*.pngtest/fixtures/images/d.pngis excluded by!**/*.pngtest/fixtures/images/portrait.pngis excluded by!**/*.pngtest/fixtures/images/portrait_b.pngis excluded by!**/*.png
📒 Files selected for processing (32)
Rakefile(1 hunks)scripts/benchmark/find_region_benchmark.rb(1 hunks)test/capybara/screenshot/diff/area_calculator_test.rb(0 hunks)test/capybara/screenshot/diff/comparison_loader_test.rb(0 hunks)test/capybara/screenshot/diff/difference_finder_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(0 hunks)test/capybara/screenshot/diff/image_preprocessor_test.rb(0 hunks)test/integration/rspec_test.rb(1 hunks)test/support/capybara_screenshot_diff/dsl_stub.rb(1 hunks)test/support/screenshoter_stub.rb(1 hunks)test/support/test_helpers.rb(1 hunks)test/system_test_case.rb(1 hunks)test/test_helper.rb(3 hunks)test/unit/area_calculator_test.rb(1 hunks)test/unit/comparison_loader_test.rb(1 hunks)test/unit/diff_test.rb(9 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(8 hunks)test/unit/image_compare_test.rb(6 hunks)test/unit/image_preprocessor_test.rb(1 hunks)test/unit/region_test.rb(4 hunks)test/unit/screenshot_namer_test.rb(3 hunks)test/unit/screenshot_test.rb(1 hunks)test/unit/screenshoter_test.rb(1 hunks)test/unit/stable_screenshoter_test.rb(4 hunks)test/unit/vcs_test.rb(2 hunks)
💤 Files with no reviewable changes (7)
- test/capybara/screenshot/diff/difference_test.rb
- test/capybara/screenshot/diff/area_calculator_test.rb
- test/capybara/screenshot/diff/image_compare_refactor_test.rb
- test/capybara/screenshot/diff/image_preprocessor_test.rb
- test/capybara/screenshot/diff/drivers/chunky_png_driver_test.rb
- test/capybara/screenshot/diff/comparison_loader_test.rb
- test/capybara/screenshot/diff/difference_finder_test.rb
✅ Files skipped from review due to trivial changes (2)
- test/system_test_case.rb
- test/unit/screenshot_namer_test.rb
🚧 Files skipped from review as they are similar to previous changes (19)
- test/unit/screenshoter_test.rb
- scripts/benchmark/find_region_benchmark.rb
- test/integration/rspec_test.rb
- Rakefile
- test/unit/region_test.rb
- test/unit/drivers/vips_driver_test.rb
- test/unit/comparison_loader_test.rb
- test/unit/vcs_test.rb
- test/unit/difference_test.rb
- test/support/capybara_screenshot_diff/dsl_stub.rb
- test/unit/image_compare_test.rb
- test/test_helper.rb
- test/unit/screenshot_test.rb
- test/unit/diff_test.rb
- test/support/test_helpers.rb
- test/unit/drivers/utils_test.rb
- test/unit/stable_screenshoter_test.rb
- test/unit/area_calculator_test.rb
- test/unit/dsl_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/unit/drivers/chunky_png_driver_test.rb (6)
test/test_helper.rb (1)
assert_same_images(62-65)test/unit/drivers/vips_driver_test.rb (3)
test(169-214)make_comparison(159-162)sample_region(164-166)test/support/capybara_screenshot_diff/dsl_stub.rb (2)
make_comparison(21-25)create_snapshot_for(57-63)lib/capybara/screenshot/diff/image_compare.rb (2)
quick_equal?(64-77)reporter(103-105)test/support/test_doubles.rb (2)
different?(133-135)exist?(23-25)lib/capybara/screenshot/diff/reporters/default.rb (4)
base_image_path(105-107)image_path(109-111)save(72-74)driver(113-115)
test/unit/difference_finder_test.rb (8)
test/unit/comparison_loader_test.rb (2)
include(10-57)setup(13-19)test/unit/dsl_test.rb (1)
include(8-155)test/support/capybara_screenshot_diff/dsl_stub.rb (1)
setup(7-11)test/unit/difference_test.rb (1)
setup(7-19)lib/capybara/screenshot/diff/reporters/default.rb (2)
driver(113-115)base_image_path(105-107)lib/capybara/screenshot/diff/difference.rb (4)
options(45-47)failed?(41-43)equal?(37-39)new_image_path(90-92)test/support/test_helpers.rb (4)
setup_driver_results(41-45)assert_dimension_check_called(12-15)assert_pixel_check_called(20-23)assert_difference_region_called(28-31)test/support/test_doubles.rb (1)
call(110-114)
🔇 Additional comments (5)
test/unit/drivers/chunky_png_driver_test.rb (1)
1-155: Excellent test structure and comprehensive coverage!This test suite is well-organized with clear separation of concerns using nested test classes. The tests cover all major functionality of the ChunkyPNG driver including:
- Quick equality checks with color distance limits
- Difference detection with various tolerance settings
- Color distance calculations and limits
- Shift distance limits and dimension changes
- Annotated image generation and file handling
The helper methods are appropriately defined and the test assertions are comprehensive. The use of fixtures and test stubs follows good testing practices.
test/support/screenshoter_stub.rb (1)
16-16: Good defensive programming practice!Adding
FileUtils.mkdir_p(path.dirname)ensures the directory structure exists before attempting to copy the file, preventing potential errors during test execution when directories might not exist.test/unit/difference_finder_test.rb (3)
119-129: Helper methods are properly consolidated.The helper methods
setup_test_comparisonandcreate_finderare now properly defined at the main class level, avoiding the duplication issue mentioned in previous reviews. This follows the DRY principle and improves maintainability.
42-74: Comprehensive quick mode testing with good assertion coverage.The quick mode tests properly verify both the return values and the driver method calls. The tests cover all scenarios:
- Exact matches with dimension and pixel checks
- Tolerance-based matching with difference objects
- Pixel differences beyond tolerance
The assertions verify both the boolean results and the presence/absence of difference objects as expected.
86-116: Well-structured full mode testing.The full mode tests appropriately verify that:
- Failed differences are returned for dimension mismatches
- Equal results are returned for exact matches
- Difference objects are returned for pixel differences
The use of
assert_instance_of Differenceand predicate methods likefailed?andequal?provides clear assertions about the expected behavior.
| warning_output = capture_io do | ||
| preprocessor = ImagePreprocessor.new(@driver, options) | ||
| result = preprocessor.call(@test_images) | ||
|
|
||
| assert_equal @test_images, result | ||
| assert_empty @driver.filter_calls | ||
| end | ||
|
|
||
| assert_match expected_warning, warning_output.join | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify the capture_io usage and warning assertion.
The test captures IO to verify warning messages, but the assertion assert_match expected_warning, warning_output.join might not work as expected since capture_io returns an array of [stdout, stderr], and warnings typically go to stderr.
Consider updating the assertion to properly handle the captured output:
- warning_output = capture_io do
+ stdout, stderr = capture_io do
preprocessor = ImagePreprocessor.new(@driver, options)
result = preprocessor.call(@test_images)
assert_equal @test_images, result
assert_empty @driver.filter_calls
end
- assert_match expected_warning, warning_output.join
+ assert_match expected_warning, stderr📝 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.
| warning_output = capture_io do | |
| preprocessor = ImagePreprocessor.new(@driver, options) | |
| result = preprocessor.call(@test_images) | |
| assert_equal @test_images, result | |
| assert_empty @driver.filter_calls | |
| end | |
| assert_match expected_warning, warning_output.join | |
| end | |
| stdout, stderr = capture_io do | |
| preprocessor = ImagePreprocessor.new(@driver, options) | |
| result = preprocessor.call(@test_images) | |
| assert_equal @test_images, result | |
| assert_empty @driver.filter_calls | |
| end | |
| assert_match expected_warning, stderr | |
| end |
🤖 Prompt for AI Agents
In test/unit/image_preprocessor_test.rb around lines 79 to 88, the test uses
capture_io which returns an array of [stdout, stderr], but the assertion checks
the combined output with join, which may not correctly capture warnings sent to
stderr. Update the assertion to check only the stderr part of the captured
output by asserting that expected_warning matches warning_output[1], ensuring
the warning message is properly verified.
Summary by CodeRabbit
Tests
Chores