refact: support different tests frameworks #139
Conversation
|
Warning Rate limit exceeded@pftg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (21)
WalkthroughThis set of changes introduces a new centralized mechanism for managing screenshot assertions and their verification in the Capybara screenshot diffing workflow. A new Changes
Sequence Diagram(s)sequenceDiagram
participant TestFramework as Test Framework (RSpec/Minitest/etc)
participant DSL as CapybaraScreenshotDiff::DSL
participant Assertion as ScreenshotAssertion
participant Registry as AssertionRegistry
participant Diff as CapybaraScreenshotDiff
TestFramework->>DSL: screenshot(name, **args)
DSL->>Assertion: ScreenshotAssertion.new(name, **args)
DSL->>Diff: add_assertion(assertion)
Diff->>Registry: add_assertion(assertion)
Note right of Registry: Assertion stored for later verification
TestFramework->>Diff: verify
Diff->>Registry: verify(assertions)
Registry->>Assertion: validate (for each assertion)
Assertion->>Assertion: assert_image_not_changed
Assertion-->>Registry: error message (if diff detected)
Registry-->>Diff: Raise ExpectationNotMet if any errors
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
A refactor to support different test frameworks by centralizing screenshot assertions and unifying error handling and window resizing logic.
- Centralizes screenshot assertion storage and error verification across tests.
- Updates and unifies browser window resize logic in test frameworks.
- Refactors tests to replace instance variable tracking with a centralized assertions registry.
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/system_test_case.rb | Updated screenshot root/save_path and refactored rollback_comparison_runtime_files. |
| test/support/stub_test_methods.rb | Changed snapshot manager initialization for consistency with new screenshot root. |
| test/support/non_minitest_assertions.rb | Updated browser resize and teardown hooks using the new DSL. |
| test/integration/browser_screenshot_test.rb | Replaced instance-based screenshot assertions with a centralized assertions registry and updated error types. |
| test/fixtures/files/rspec_spec.rb | Adjusted screenshot save_path and root values for consistency. |
| test/capybara/screenshot/screenshot_test.rb | Modified root path usage. |
| test/capybara/screenshot/diff_test.rb | Updated error types and assertions for stability checks. |
| test/capybara/screenshot/diff/test_methods_test.rb | Replaced instance screenshot arrays with assertions registry and added debug print statements in ScreenshotFormatTest. |
| lib/capybara_screenshot_diff/screenshot_assertion.rb | Introduced new screenshot assertion and registry classes for centralized error management. |
| lib/capybara_screenshot_diff/rspec.rb | Updated DSL inclusion and teardown hooks for RSpec tests. |
| lib/capybara_screenshot_diff/minitest.rb | Adjusted screenshot hook to increment assertions and rescue errors properly. |
| lib/capybara_screenshot_diff/dsl.rb | Introduced new DSL methods wrapping screenshot calls. |
| lib/capybara_screenshot_diff/cucumber.rb | Updated resize logic for Cucumber tests. |
| lib/capybara_screenshot_diff.rb | Refactored error classes into a new hierarchy. |
| lib/capybara/screenshot/diff/test_methods.rb | Major refactor on job scheduling and error raising for screenshot verifications. |
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Changed error raising from fail to an exception with a new error type. |
| lib/capybara/screenshot/diff/browser_helpers.rb | Added a helper to conditionally resize the window. |
Comments suppressed due to low confidence (2)
test/integration/browser_screenshot_test.rb:21
- The error message 'Not processed screenshots' may be too vague for diagnosing failures. Consider expanding the message to include context or details to help identify which assertions failed.
raise "Not processed screenshots"
test/capybara/screenshot/diff/test_methods_test.rb:191
- Debug print statements (such as puts and pp) in the ScreenshotFormatTest setup and teardown may clutter test output and should be removed or gated behind a debug flag before merging to production.
puts "ScreenshotFormatTest#setup assertions: #{CapybaraScreenshotDiff.assertions}"
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
lib/capybara/screenshot/diff/test_methods.rb (1)
145-147:⚠️ Potential issueNumbered block parameters (
_1) require Ruby ≥ 2.7If the gem still claims compatibility with Ruby 2.6 or earlier, this line will cause a syntax error.
To keep backward-compatibility use an explicit parameter:- raise CapybaraScreenshotDiff::ExpectationNotMet.new(error_msg).tap { _1.set_backtrace(backtrace) } + raise CapybaraScreenshotDiff::ExpectationNotMet.new(error_msg).tap { |err| err.set_backtrace(backtrace) }
🧹 Nitpick comments (8)
lib/capybara_screenshot_diff/dsl.rb (1)
12-13: Minor but safe: ensurealias_methodis defined only once.Modules can be included multiple times (e.g. via
extend).
Usingalias_methodat the top level is usually fine, but if the module is re-included the alias will raiseNameError.
Guarding with:alias_method :_screenshot, :screenshot unless method_defined?(:_screenshot)prevents accidental double definition.
Not critical, yet avoids surprising load-order errors.lib/capybara_screenshot_diff/minitest.rb (1)
37-45: Minor: you already incrementedself.assertions; no need to create an extraMinitest::Assertion.Inside the teardown you build a dummy
Minitest::Assertionand push it intofailures; this double-counts for the statistics (assertionscounter was bumped in the test, then again here).
Instead, re-raise the converted exception so Minitest handles bookkeeping, or decrementassertions.-assertion = ::Minitest::Assertion.new(e) -assertion.set_backtrace [] -failures << assertion +raise ::Minitest::Assertion, e.messageCleaner and matches how Minitest treats normal
assertfailures.test/capybara/screenshot/diff/test_methods_test.rb (1)
112-114: Fix method argument syntaxThe current method definition with splat operator (
*) is triggering a RuboCop syntax error. Consider using named parameters for better clarity.-def assert_image_not_changed(*) - CapybaraScreenshotDiff::ScreenshotAssertion.assert_image_not_changed(*) +def assert_image_not_changed(*args) + CapybaraScreenshotDiff::ScreenshotAssertion.assert_image_not_changed(*args)🧰 Tools
🪛 RuboCop (1.73)
[fatal] 113-113: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure usingTargetRubyVersionparameter, underAllCops)(Lint/Syntax)
test/capybara/screenshot/diff_test.rb (2)
30-32: Prefer the public façade instead of reaching into the registryYou already expose
CapybaraScreenshotDiff.reset, which delegates to the registry. CallingCapybaraScreenshotDiff.registry.resetcouples the test to an implementation detail and would break if the registry ever stops being publicly accessible.- CapybaraScreenshotDiff.registry.reset + CapybaraScreenshotDiff.reset
191-195: Debugputs/ppwill pollute test outputThe unconditional
putsandppstatements will clutter CI logs and slow the suite. Consider guarding them behind a verbose flag or removing them entirely.lib/capybara_screenshot_diff/screenshot_assertion.rb (3)
3-7:callerattribute shadows Kernel#callerUsing the attribute name
callermakes stack traces harder to read and may confuse future readers.
Consider renaming it to something likecall_stackorsource_location.
74-86:add_assertionsilently ignores assertions without a compare objectSilently returning
nilmay hide logic errors upstream. Either raise with a clear message or log a warning so missing comparisons are noticed during test authorship.
107-120:Forwardableis used without arequireStdlib still needs an explicit require when the file is loaded standalone:
require "forwardable"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
test/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!**/*.png
📒 Files selected for processing (19)
lib/capybara/screenshot/diff/browser_helpers.rb(1 hunks)lib/capybara/screenshot/diff/stable_screenshoter.rb(1 hunks)lib/capybara/screenshot/diff/test_methods.rb(3 hunks)lib/capybara_screenshot_diff.rb(2 hunks)lib/capybara_screenshot_diff/cucumber.rb(1 hunks)lib/capybara_screenshot_diff/dsl.rb(1 hunks)lib/capybara_screenshot_diff/minitest.rb(1 hunks)lib/capybara_screenshot_diff/rspec.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(1 hunks)test/capybara/screenshot/diff/stable_screenshoter_test.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(3 hunks)test/capybara/screenshot/diff/vcs_test.rb(1 hunks)test/capybara/screenshot/diff_test.rb(8 hunks)test/capybara/screenshot/screenshot_test.rb(1 hunks)test/fixtures/files/rspec_spec.rb(1 hunks)test/integration/browser_screenshot_test.rb(4 hunks)test/support/non_minitest_assertions.rb(1 hunks)test/support/stub_test_methods.rb(1 hunks)test/system_test_case.rb(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
lib/capybara/screenshot/diff/stable_screenshoter.rb (1)
lib/capybara_screenshot_diff/attempts_reporter.rb (1)
generate(13-19)
test/capybara/screenshot/screenshot_test.rb (1)
lib/capybara_screenshot_diff.rb (1)
root(34-36)
lib/capybara_screenshot_diff/cucumber.rb (1)
lib/capybara/screenshot/diff/browser_helpers.rb (1)
resize_window_if_needed(8-12)
lib/capybara/screenshot/diff/browser_helpers.rb (1)
test/integration/browser_screenshot_test.rb (1)
window_size(213-219)
lib/capybara_screenshot_diff/rspec.rb (2)
lib/capybara/screenshot/diff/browser_helpers.rb (1)
resize_window_if_needed(8-12)lib/capybara_screenshot_diff/screenshot_assertion.rb (2)
verify(89-93)reset(101-103)
test/integration/browser_screenshot_test.rb (4)
lib/capybara_screenshot_diff/screenshot_assertion.rb (2)
reset(101-103)failed_assertions(95-99)lib/capybara/screenshot/diff/region.rb (1)
present?(88-90)lib/capybara/screenshot/diff/image_compare.rb (1)
different?(58-60)lib/capybara/screenshot/diff/difference.rb (1)
different?(9-11)
test/system_test_case.rb (3)
lib/capybara_screenshot_diff.rb (1)
root(34-36)lib/capybara_screenshot_diff/snap_manager.rb (1)
root(68-70)lib/capybara/screenshot/diff/reporters/default.rb (1)
comparison(116-118)
🪛 GitHub Actions: Lint
lib/capybara_screenshot_diff.rb
[error] 14-14: Layout/EmptyLineBetweenDefs: Expected 1 empty line between class definitions; found 0. (Correctable)
[error] 15-15: Layout/EmptyLineBetweenDefs: Expected 1 empty line between class definitions; found 0. (Correctable)
🪛 RuboCop (1.73)
test/capybara/screenshot/diff/test_methods_test.rb
[fatal] 113-113: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
test/capybara/screenshot/diff_test.rb
[warning] 210-210: Unreachable code detected.
(Lint/UnreachableCode)
🔇 Additional comments (25)
test/capybara/screenshot/screenshot_test.rb (1)
21-21: Good improvement to test setup.Changing the path from
"."to"./tmp"makes the test more robust by using a directory specifically intended for temporary files. This ensures consistency across different environments and avoids potential issues with the current directory.test/capybara/screenshot/diff/vcs_test.rb (1)
24-24: Formatting improvement.Adding a blank line improves readability by separating the declaration of the screenshot path from the base screenshot path. This follows good Ruby style practices.
test/support/stub_test_methods.rb (2)
13-13: Good use of the configured screenshot root.Using
Capybara::Screenshot.rootas the base directory ensures that tests use the same root path as the actual screenshot code, making tests more consistent with production behavior.
20-20: Essential cleanup to prevent test state leakage.Adding
CapybaraScreenshotDiff.resetto the teardown block ensures that each test starts with a clean state, preventing one test from affecting subsequent tests. This aligns with the centralized assertion management approach mentioned in the PR summary.lib/capybara_screenshot_diff/cucumber.rb (1)
9-9: Improved code organization with helper method.Replacing conditional logic with
Capybara::Screenshot::BrowserHelpers.resize_window_if_neededconsolidates window resizing behavior in one place, making the code more maintainable and consistent across different test frameworks.test/fixtures/files/rspec_spec.rb (1)
19-20: Path configuration looks good and aligns with broader refactoring.The updated path configuration simplifies the screenshot storage location and explicitly sets the root path, which supports the centralized screenshot assertion management introduced in this PR.
lib/capybara/screenshot/diff/browser_helpers.rb (1)
8-12: Good refactoring to centralize window resizing logic.This helper method effectively centralizes the window resizing logic that was previously duplicated across different test framework integrations. The method appropriately checks if the window_size is available before attempting to use it, following good defensive programming practices.
test/capybara/screenshot/diff/stable_screenshoter_test.rb (1)
93-93: Improved error specificity with custom exception.Updating the test to expect the more specific
CapybaraScreenshotDiff::UnstableImageexception instead of a genericRuntimeErrorimproves error handling clarity and aligns with the implementation change in the StableScreenshoter class.lib/capybara/screenshot/diff/stable_screenshoter.rb (1)
103-103: Better error typing with domain-specific exception.Replacing the generic
failwith a specific exception class improves error handling by providing a more targeted exception type. This change enhances the error hierarchy and makes troubleshooting easier.lib/capybara_screenshot_diff/rspec.rb (3)
16-17: LGTM: Clean inclusion of DSL moduleThe inclusion of the DSL module for both feature and system specs simplifies the configuration and makes it more explicit.
19-22: LGTM: Improved window resizing logicThe before hook now unconditionally calls
resize_window_if_needed, which centralizes the window resizing logic and removes the need for conditional checks in various places. This is a good refactoring that simplifies the code.
25-31: LGTM: Streamlined screenshot verificationThe after hook improves error handling by using a begin/ensure block to guarantee that
CapybaraScreenshotDiff.resetis called regardless of whether verification succeeds or fails. This centralized approach is more robust than the previous manual error collection.test/capybara/screenshot/diff/test_methods_test.rb (3)
4-4: LGTM: Added required dependencyAdding the requirement for the screenshot assertion module is necessary for the new centralized approach.
56-58: LGTM: Helpful utility methodGood addition of a helper method to check for pending screenshot jobs, which improves readability of the tests.
62-72: LGTM: Updated test to use centralized assertionsThe test now correctly uses
CapybaraScreenshotDiff.assertionsfor tracking screenshots instead of a local instance variable, aligning with the centralized approach.test/system_test_case.rb (3)
18-18: LGTM: Simplified path configurationUsing
Rails.root / "../test/fixtures/app"is a cleaner way to specify the relative path.
20-20: LGTM: Simpler save pathUsing a relative path
"./doc/screenshots"simplifies the configuration.
60-63: Improved method signature and early returnThe method now accepts a
ScreenshotAssertionobject directly and handles extracting the comparison more robustly with an early return. This is a good defensive programming practice.test/integration/browser_screenshot_test.rb (6)
13-13: LGTM: Clear assertions after each testAdding
CapybaraScreenshotDiff.resetin the teardown method ensures that assertions from each test don't leak into subsequent tests.
16-23: Good cleanup strategy in after_teardownThis ensures any unprocessed screenshot assertions are properly cleaned up and raises an error if assertions remain unverified, which helps catch bugs in the test suite.
140-142: LGTM: Updated assertions to use centralized stateThe test now correctly accesses assertion state from
CapybaraScreenshotDiff.assertionsinstead of a local instance variable.
185-185: Improved error specificityUsing
CapybaraScreenshotDiff::UnstableImageexception instead ofMinitest::UnexpectedErrormakes the test more specific and better aligned with the domain model.
222-226: LGTM: Updated assertion helperThe helper method now correctly uses
CapybaraScreenshotDiff.failed_assertionsfor checking screenshot errors.
229-237: LGTM: Updated error checking helperThis helper method now correctly uses the centralized failed assertions collection.
lib/capybara/screenshot/diff/test_methods.rb (1)
80-82: Thread-safety caveat
CapybaraScreenshotDiff.add_assertionstores assertions in a thread-local registry.
If tests spawn additional threads the assertion may be lost. Make sure the gem’s documentation explicitly states that screenshot assertions must be scheduled from the test thread, or propagate the registry across threads.
| expected_message = | ||
| "Screenshot does not match for 'sample_screenshot' expected error message for non minitest" | ||
| assert_raises(::StandardError, expected_message) { test_case.teardown } | ||
| assert_empty(test_case.instance_variable_get(:@test_screenshots)) | ||
| assert_raises(CapybaraScreenshotDiff::ExpectationNotMet, expected_message) { test_case.teardown } | ||
| assert_empty(CapybaraScreenshotDiff.assertions) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The error message is never asserted
assert_raises ignores the second argument unless the block raises no error.
If you really want to verify the error text, capture the exception and make an explicit assertion:
- assert_raises(CapybaraScreenshotDiff::ExpectationNotMet, expected_message) { test_case.teardown }
+ err = assert_raises(CapybaraScreenshotDiff::ExpectationNotMet) { test_case.teardown }
+ assert_includes err.message, expected_message📝 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.
| expected_message = | |
| "Screenshot does not match for 'sample_screenshot' expected error message for non minitest" | |
| assert_raises(::StandardError, expected_message) { test_case.teardown } | |
| assert_empty(test_case.instance_variable_get(:@test_screenshots)) | |
| assert_raises(CapybaraScreenshotDiff::ExpectationNotMet, expected_message) { test_case.teardown } | |
| assert_empty(CapybaraScreenshotDiff.assertions) | |
| end | |
| expected_message = | |
| "Screenshot does not match for 'sample_screenshot' expected error message for non minitest" | |
| err = assert_raises(CapybaraScreenshotDiff::ExpectationNotMet) { test_case.teardown } | |
| assert_includes err.message, expected_message | |
| assert_empty(CapybaraScreenshotDiff.assertions) | |
| end |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
test/capybara/screenshot/diff/test_methods_test.rb (1)
56-58: Preferassert_emptyover custom predicate for clarityInstead of checking
blank?(ActiveSupport-specific) you can rely on the built-in Minitest helper:-def assert_no_screenshot_jobs_scheduled - assert_predicate CapybaraScreenshotDiff.assertions, :blank? -end +def assert_no_screenshot_jobs_scheduled + assert_empty CapybaraScreenshotDiff.assertions +endThis removes the implicit dependency on ActiveSupport and communicates intent more directly.
lib/capybara/screenshot/diff/test_methods.rb (2)
104-124: Minor: Backtrace filtering can drop user frames in Rails engines
caller.reject { |l| l =~ /gems\/(activesupport|minitest|railties)/ }
will also remove user code living in vendor/engines because paths often contain
railties. Consider a stricter pattern or configurable allow-list to avoid
empty backtraces.
133-144:invoke_match_jobalways returnstrueeven after failureWhen
CapybaraScreenshotDiff.assert_image_not_changedreturns an error string,
invoke_match_jobraises, rescues nothing, then reachestrue.
But theraiseprevents the method from ever returning – the trailingtrue
is dead code.def invoke_match_job(job) error_msg = CapybaraScreenshotDiff.assert_image_not_changed(*job) if error_msg _raise_error(error_msg, job[0]) - false end - true endYou can either remove the dead paths or return explicitly when no error to
clarify intent.lib/capybara_screenshot_diff/screenshot_assertion.rb (2)
33-37: RDOC comment references outdated parameter format.The comment refers to the parameter
screenshotsas an array of arrays ([Array(Array(String), String, ImageCompare)]), but the method now expects an array ofScreenshotAssertionobjects, based on line 40-42 where each item is treated as an assertion object.-# @param screenshots [Array(Array(Array(String), String, ImageCompare))] The list of match screenshots jobs. Defaults to all screenshots taken during the test. +# @param screenshots [Array<ScreenshotAssertion>] The list of screenshot assertions to verify. Defaults to all assertions taken during the test.
38-38: Direct reference to external modules.The method references
::Capybara::Screenshotand::Capybara::Screenshot::Diffdirectly with full qualification. Consider importing these at the top of the file or referencing them through a consistent namespace strategy to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/capybara/screenshot/diff/region.rb(1 hunks)lib/capybara/screenshot/diff/test_methods.rb(4 hunks)lib/capybara/screenshot/diff/vcs.rb(1 hunks)lib/capybara_screenshot_diff/rspec.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(3 hunks)test/capybara/screenshot/diff_test.rb(7 hunks)test/integration/browser_screenshot_test.rb(4 hunks)test/support/non_minitest_assertions.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/capybara/screenshot/diff/region.rb
- lib/capybara_screenshot_diff/rspec.rb
- test/integration/browser_screenshot_test.rb
- test/capybara/screenshot/diff_test.rb
🧰 Additional context used
🪛 GitHub Actions: Lint
lib/capybara_screenshot_diff/screenshot_assertion.rb
[error] 16-16: StandardRB: Layout/EmptyLineBetweenDefs - Expected 1 empty line between method definitions; found 0. This offense is autocorrectable.
🪛 RuboCop (1.73)
test/capybara/screenshot/diff/test_methods_test.rb
[fatal] 110-110: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🔇 Additional comments (8)
lib/capybara/screenshot/diff/vcs.rb (1)
55-58: Guard against nilsvn_infoto avoid unexpectedNoMethodError
Kernel\`` always returns aString, but defensive coding costs virtually nothing. Ifsvnis missing or exits abnormally,svn_infomight benilin some Ruby versions when$CHILD_STATUSisnil.String#empty?would then raiseNoMethodError`.- unless svn_info.empty? + unless svn_info.to_s.empty?[ suggest_nitpick ]
test/support/non_minitest_assertions.rb (1)
15-19: 👍 Registry is now always cleaned up – previous leak fixedThe
ensureblock guaranteesCapybaraScreenshotDiff.reseteven when verification raises, eliminating the state-leak flagged in an earlier review.
Looks great!test/capybara/screenshot/diff/test_methods_test.rb (1)
62-69: Confirm intent: first assertion’s caller comes from helper, not user code
assert_match(/our_screenshot'/, …caller.first)asserts that the top frame is the helper, not the test case.
If the goal is to ensure user-level backtraces, the regex should target the test file instead.
Please double-check the expectation.lib/capybara_screenshot_diff/screenshot_assertion.rb (5)
1-4: Good use ofrequire "fileutils"in this new file.The required library is correctly imported at the beginning of the file, which addresses the issue mentioned in a previous review comment.
10-15: LGTM! Fixed block parameter syntax.The initialization method uses proper block handling with
block_given?andyield(self), which addresses the syntax error mentioned in a previous review comment.
58-67: FileUtils operations are better protected now.The file operations now include proper existence checking before moving files and are guarded by conditions, which addresses the concerns raised in a previous review comment.
114-116: Thread safety is correctly implemented.Good use of thread-local storage for the registry, which ensures proper isolation in parallel test environments.
97-97:Details
✅ Verification successful
Dependency on an externally defined exception class.
This line raises
CapybaraScreenshotDiff::ExpectationNotMetwhich is not defined in this file. Ensure this class is defined elsewhere in the codebase before this file is loaded.
🏁 Script executed:
#!/bin/bash # Verify that the ExpectationNotMet class is defined elsewhere in the codebase rg -A 2 -B 2 "class ExpectationNotMet"Length of output: 377
No missing exception class definition
- The
ExpectationNotMetexception is defined inlib/capybara_screenshot_diff.rb:class ExpectationNotMet < CapybaraScreenshotDiffError; endNo further action is needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/capybara/screenshot/diff/test_methods.rb (1)
66-68: Previous suggestion not implemented: Avoid mutating state when screenshots are disabledThis code still initializes
@screenshot_groupand@screenshot_counterbefore checking if screenshots are active, which could lead to state leakage across tests that toggle the flag at runtime. The previous review comment already suggested a solution to this issue.def screenshot_group(name) - @screenshot_group = name.to_s - @screenshot_counter = (@screenshot_group.nil? || @screenshot_group.empty?) ? nil : 0 - name_present = !(name.nil? || name.empty?) - return unless Screenshot.active? && name_present + return unless Screenshot.active? && name.present? + + @screenshot_group = name.to_s + @screenshot_counter = 0
🧹 Nitpick comments (1)
lib/capybara/screenshot/diff/test_methods.rb (1)
104-130: Well-structured refactoring with clear separation of concernsThe restructuring of the
screenshotmethod with clear setup, exercise, and verify phases improves code organization. The delegation to centralized validation aligns with the PR objectives.One small suggestion for the backtrace filtering: consider extracting the regex pattern to a constant for better maintainability:
- backtrace = caller(skip_stack_frames + 1).reject { |l| l =~ /gems\/(activesupport|minitest|railties)/ } + FRAMEWORK_GEMS_PATTERN = /gems\/(activesupport|minitest|railties)/ + backtrace = caller(skip_stack_frames + 1).reject { |l| l =~ FRAMEWORK_GEMS_PATTERN }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/capybara/screenshot/diff/test_methods.rb(4 hunks)
🔇 Additional comments (2)
lib/capybara/screenshot/diff/test_methods.rb (2)
81-81: Good refactoring to use centralized assertion registryReplacing local assertion tracking with
CapybaraScreenshotDiff.add_assertionsuccessfully implements the centralized approach described in the PR objectives.
87-88: Logic preserved with clearer nil/empty checksThe updates to the condition checks maintain the same functionality while making the intention clearer.
pftg
force-pushed
the
cleanup-frameworks-integrations
branch
from
100ea63 to
7788ec7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/capybara/screenshot/diff/test_methods_test.rb (1)
118-120:⚠️ Potential issueSyntax error – splat operator used without a target variable
*by itself is only valid in a parameter list. Inside a method body you need an explicit variable to splat.-def assert_image_not_changed(*) - CapybaraScreenshotDiff::ScreenshotAssertion.assert_image_not_changed(*) +def assert_image_not_changed(*args) + CapybaraScreenshotDiff::ScreenshotAssertion.assert_image_not_changed(*args) end🧰 Tools
🪛 RuboCop (1.73)
[fatal] 119-119: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure usingTargetRubyVersionparameter, underAllCops)(Lint/Syntax)
lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
111-114: 🛠️ Refactor suggestionMissing require for Forwardable
The code extends
Forwardablebut doesn't explicitly require it. Add the require statement to avoid potential loading issues.module CapybaraScreenshotDiff class << self + require 'forwardable' extend Forwardable
🧹 Nitpick comments (1)
lib/capybara_screenshot_diff/screenshot_assertion.rb (1)
63-68: Potential for FileUtils error handling improvementThe FileUtils operations could fail if files don't exist (due to race conditions or parallel tests). Consider using safer alternatives or adding error handling.
if !result && comparison.base_image_path.exist? - FileUtils.mv(comparison.base_image_path, comparison.image_path, force: true) + begin + FileUtils.mv(comparison.base_image_path, comparison.image_path, force: true) + rescue Errno::ENOENT => e + # Log or handle the case where the file doesn't exist + end elsif !comparison.dimensions_changed? - FileUtils.rm_rf(comparison.base_image_path) + begin + FileUtils.rm_rf(comparison.base_image_path) + rescue Errno::ENOENT => e + # Log or handle the case where the file doesn't exist + end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/capybara_screenshot_diff/minitest.rb(1 hunks)lib/capybara_screenshot_diff/screenshot_assertion.rb(1 hunks)test/capybara/screenshot/diff/test_methods_test.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/capybara_screenshot_diff/minitest.rb
🧰 Additional context used
🪛 RuboCop (1.73)
test/capybara/screenshot/diff/test_methods_test.rb
[fatal] 119-119: unexpected token tRPAREN
(Using Ruby 3.1 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🔇 Additional comments (11)
test/capybara/screenshot/diff/test_methods_test.rb (4)
56-58: Good addition of a verification helperThe
assert_no_screenshot_jobs_scheduledhelper method provides a nice abstraction for verifying that no screenshot assertions are registered, making tests more readable.
80-87: Good addition of inline validation testThis test ensures that screenshot assertions can be validated immediately when delayed mode is disabled, which improves test coverage for the new assertion framework.
4-4: Appropriate require statement for new dependencyGood addition of the require statement for the new screenshot assertion module, which is now being used in this test file.
62-77: Clean migration to centralized assertion trackingThe test has been properly updated to use the new centralized
CapybaraScreenshotDiff.assertionscollection instead of relying on instance variables, which aligns with the overall refactoring goal of centralizing screenshot assertion management.lib/capybara_screenshot_diff/screenshot_assertion.rb (7)
1-4: Good file organization with necessary requiresThe file includes the required FileUtils module explicitly, addressing a potential issue where the module might be used without being required.
10-15: Well-designed initialization with block supportThe initialization method properly handles block arguments and uses standard Ruby idioms like
yield(self) if block_given?instead of relying on anonymous parameters.
115-126: Well-designed thread-safe registry with delegated methodsGood implementation of a thread-local registry with method delegation, which provides a clean interface for managing assertions across different contexts.
38-50: Clean error aggregation and verification logicThe
verify_screenshots!method elegantly maps validations, compacts the results, and ensures proper cleanup in theensureblock, making the error handling robust.
5-25: Well-structured ScreenshotAssertion class initializationThe class provides flexible initialization with both direct and factory methods, which allows for easy creation and conversion of assertion objects.
95-99: Good separation of verification and error reportingThe
verifymethod properly separates the concerns of screenshot verification and raising exceptions, making the code easier to maintain and extend.
91-93: Simple and clear assertion presence checkThe
assertions_present?method provides a semantically clear way to check if there are any assertions to verify, improving code readability.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the screenshot diffing system to support multiple test frameworks, improves error handling with a centralized assertion mechanism, and streamlines window resizing and screenshot verification logic.
- Centralized management of screenshot assertions and state with reset/verify methods.
- Updated test lifecycle hooks for RSpec, Minitest, and Cucumber to ensure consistent error handling.
- Refactored diff methods and exception types for clearer, more specific error reporting.
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/stub_test_methods.rb | Updated SnapManager initialization and added a cleanup reset to refresh assertion state in teardown. |
| test/support/non_minitest_assertions.rb | Replaced explicit resize/verify methods with streamlined helper calls. |
| test/integration/browser_screenshot_test.rb | Transitioned to using assertions from CapybaraScreenshotDiff and updated expected exception types. |
| test/fixtures/files/rspec_spec.rb | Changed screenshot save path configurations to reflect centralized management. |
| test/capybara/screenshot/diff_test.rb | Consolidated diff driver checks and revised error raising for screenshot differences. |
| lib/capybara_screenshot_diff/screenshot_assertion.rb | Introduced a new ScreenshotAssertion class to encapsulate screenshot verification. |
| lib/capybara_screenshot_diff/rspec.rb | Updated RSpec hooks to verify and reset screenshot assertions, mapping errors to RSpec exceptions. |
| lib/capybara_screenshot_diff/minitest.rb | Refactored Minitest hooks to translate custom exceptions and ensure proper cleanup. |
| lib/capybara_screenshot_diff/dsl.rb | Replaced the original screenshot method with a DSL wrapper that integrates with the new assertion system. |
| lib/capybara/screenshot/diff/vcs.rb | Updated SVN revision checking based on empty output rather than ActiveSupport’s present? check. |
| lib/capybara/screenshot/diff/test_methods.rb | Removed legacy screenshot job storage in favor of centralized assertion handling. |
| lib/capybara/screenshot/diff/stable_screenshoter.rb | Changed screenshot stabilization errors to raise a custom UnstableImage error. |
| lib/capybara/screenshot/diff/browser_helpers.rb | Added a helper to conditionally resize the browser window. |
Comments suppressed due to low confidence (4)
test/support/stub_test_methods.rb:20
- [nitpick] Adding CapybaraScreenshotDiff.reset in the teardown block ensures that the assertion state is consistently cleared across tests. Please verify that this reset call is applied uniformly in all test frameworks.
CapybaraScreenshotDiff.reset
lib/capybara/screenshot/diff/test_methods.rb:135
- [nitpick] The refactored method invoke_match_job encapsulates screenshot verification and error handling; ensure that its behavior aligns with the intended test outcome and that branch conditions are adequately tested.
def invoke_match_job(job)
lib/capybara_screenshot_diff/minitest.rb:26
- [nitpick] The new exception handling converts CapybaraScreenshotDiff exceptions to Minitest assertions, which improves clarity. Please confirm that this change integrates seamlessly with existing test reporting and lifecycle hooks.
rescue ::CapybaraScreenshotDiff::ExpectationNotMet => e
lib/capybara_screenshot_diff/dsl.rb:12
- [nitpick] The new DSL method aliasing centralizes screenshot assertion management; ensure that any external customizations relying on the previous implementation are updated accordingly.
alias_method :_screenshot, :screenshot
084be33 to
ff27ec2
Compare
* removed dependency on the global variable to schedule assertions
pftg
force-pushed
the
cleanup-frameworks-integrations
branch
from
ff27ec2 to
a42f248
Compare
|
@donv @UweKubosch please check. |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests