refactor: ADR-006 T3+T4 — hoist setup, consolidate assertion helpers#186
refactor: ADR-006 T3+T4 — hoist setup, consolidate assertion helpers#186
Conversation
T3: Hoist duplicated setup (base_path, new_path, driver, comparison) from 3 nested classes to parent DifferenceFinderTest. Nested classes inherit setup and only add their own (@finder). T4: Extract assert_driver_check_called(driver, check_type, times) parameterized helper. Old methods kept as one-line aliases. T6: Skipped — param order already matches Minitest convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors test helpers to use a single parameterized assertion for driver checks while preserving old helper names as thin aliases, and hoists duplicated setup logic from nested DifferenceFinderTest subclasses into the parent test class to reduce duplication and centralize test initialization. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 15 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughConsolidates three test assertion helpers into a single generalized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/difference_finder_test.rb`:
- Around line 21-22: The class body for InitializationTest starts with an extra
blank line causing a StandardRB lint failure (Layout/EmptyLinesAroundClassBody);
remove the empty line immediately after the declaration "class
InitializationTest < self" so the first statement in the class follows the class
line directly, ensuring no blank line between the class header and its contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30089cca-91a5-4b6a-92c7-b5a77babcbe8
📒 Files selected for processing (2)
test/support/test_helpers.rbtest/unit/difference_finder_test.rb
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
assert_driver_check_called, consider validatingcheck_typeagainst the supported options before callingpublic_sendso that a typo or unexpected symbol yields a clearer failure than aNoMethodErroron the driver. - The error message in
assert_driver_check_callednow interpolates the rawcheck_typesymbol; you might want to normalize or prettify it (e.g.,dimension checkinstead ofdimension_check) to keep assertion messages as readable as the previous hard-coded versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `assert_driver_check_called`, consider validating `check_type` against the supported options before calling `public_send` so that a typo or unexpected symbol yields a clearer failure than a `NoMethodError` on the driver.
- The error message in `assert_driver_check_called` now interpolates the raw `check_type` symbol; you might want to normalize or prettify it (e.g., `dimension check` instead of `dimension_check`) to keep assertion messages as readable as the previous hard-coded versions.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
CodeRabbit blank line finding already fixed in b6a50cc. |
Add pr-comment input to upload-screenshots composite action. When enabled, it finds/creates a PR comment with report links automatically. Also adds job summary with artifact links. Removes ~40 lines of duplicated YAML from test.yml. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Document pr-comment input for upload-screenshots action - Document setup-ruby-and-dependencies action with inputs table - Add full example combining both actions - Move manual setup and update-baselines to collapsible sections - Update README CI snippet to show pr-comment: 'true' Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
DifferenceFinderTest(−12 lines)assert_driver_check_called(driver, check_type, times)parameterized helper. Old methods kept as one-line aliases for backward compatibility.assert_same_imagesparam order already matches Minitest conventionNet: −16 lines, cleaner test structure.
Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Refactor test helpers and difference finder tests to reduce duplication and centralize assertion logic.
Enhancements:
Summary by CodeRabbit
This release contains internal test infrastructure improvements with no user-facing changes.