fix: skip at_exit when framework adapter handles finalization#183
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideConditionally register the HTML reporter at_exit finalization fallback only when no known test framework integration is present, to avoid duplicate finalization when Minitest, RSpec, or Cucumber adapters already handle it via native hooks. Sequence diagram for reporter finalization with and without framework adapterssequenceDiagram
actor Developer
participant TestFramework as TestFramework(Minitest_RSpec_Cucumber)
participant RubyAtExit as Ruby_at_exit
participant Capybara as CapybaraScreenshotDiff
participant HTML as HTMLReporter
Developer->>Capybara: require html_reporter
Capybara->>HTML: register embed_images option
alt Known framework loaded
Developer->>TestFramework: run tests
TestFramework->>Capybara: finalize_reporters! via native hook
Capybara->>HTML: write_report
note over RubyAtExit: at_exit fallback not registered
else No known framework loaded
Developer->>RubyAtExit: register at_exit { finalize_reporters! }
Developer->>RubyAtExit: process exits
RubyAtExit->>Capybara: finalize_reporters!
Capybara->>HTML: write_report
end
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 7 minutes and 5 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 (3)
📝 WalkthroughWalkthroughThe PR introduces a coordination mechanism for HTML reporter finalization across test frameworks. It adds an Changes
Sequence Diagram(s)sequenceDiagram
participant Framework as Test Framework<br/>(Minitest/RSpec/Cucumber)
participant Reporter as CapybaraScreenshotDiff<br/>(Flag & Core)
participant HTMLRptr as HTML Reporter
participant AtExit as at_exit Handler
rect rgba(100, 150, 200, 0.5)
note over Framework,AtExit: Setup Phase
Framework->>Reporter: external_at_exit = true
Framework->>Framework: Register native test hook<br/>(after_run / after(:suite) / AfterAll)
end
rect rgba(150, 100, 200, 0.5)
note over Framework,AtExit: Test Execution Phase
HTMLRptr->>AtExit: Register at_exit block
end
rect rgba(200, 150, 100, 0.5)
note over Framework,AtExit: Finalization Phase
Framework->>Reporter: finalize_reporters!<br/>(via native hook)
Reporter->>Reporter: Generate report & summary
AtExit->>Reporter: Check external_at_exit?
alt external_at_exit is true
AtExit->>AtExit: Skip finalization
else external_at_exit is false
AtExit->>Reporter: finalize_reporters!
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using
defined?(Minitest) || defined?(RSpec) || defined?(AfterAll)to detect framework presence may be brittle; consider checking for the specific integration hook/adapter you rely on (e.g., a module or method your adapters define) to avoid false positives/negatives from unrelated constants. - If this file can be loaded multiple times in a process (e.g., with code reloading), it may be safer to guard the
at_exitregistration with an idempotent flag onCapybaraScreenshotDiffrather than only checking framework constants.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `defined?(Minitest) || defined?(RSpec) || defined?(AfterAll)` to detect framework presence may be brittle; consider checking for the specific integration hook/adapter you rely on (e.g., a module or method your adapters define) to avoid false positives/negatives from unrelated constants.
- If this file can be loaded multiple times in a process (e.g., with code reloading), it may be safer to guard the `at_exit` registration with an idempotent flag on `CapybaraScreenshotDiff` rather than only checking framework constants.
## Individual Comments
### Comment 1
<location path="lib/capybara_screenshot_diff/reporters/html.rb" line_range="141-137" />
<code_context>
-at_exit { CapybaraScreenshotDiff.finalize_reporters! }
+# Fallback for frameworks without explicit integration.
+# Minitest, RSpec, and Cucumber adapters call finalize_reporters! via native hooks.
+unless defined?(Minitest) || defined?(RSpec) || defined?(AfterAll)
+ at_exit { CapybaraScreenshotDiff.finalize_reporters! }
+end
</code_context>
<issue_to_address>
**issue:** Guarding `at_exit` based on `defined?` may be unreliable if frameworks are loaded after this file
This relies on Minitest/RSpec/AfterAll being loaded before this file. If the test framework is loaded later (e.g., via lazy loading or a different boot path), these `defined?` checks will be false and the fallback `at_exit` will still be registered, leading to duplicate `finalize_reporters!` calls when the framework-specific hooks run. It may be safer to use an explicit configuration flag or have the adapters actively disable this fallback instead of inferring framework presence at require time.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Minitest, RSpec, and Cucumber adapters call finalize_reporters! via native hooks. The at_exit fallback in html.rb was firing as a wasted no-op for those frameworks (hitting @Finalized guard every time). Now only registers at_exit when no known framework is loaded. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pftg
force-pushed
the
fix/skip-at-exit-when-framework-loaded
branch
from
03f2745 to
504676a
Compare
Framework adapters set external_at_exit = true and register native hooks (Minitest.after_run, RSpec after(:suite), Cucumber AfterAll). The at_exit block checks the flag and skips when a framework adapter handles finalization. This eliminates both the double-call problem and the LIFO ordering problem cleanly — same pattern SimpleCov uses successfully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Document how to call finalize_reporters! for custom test frameworks - Add brew/apt install commands for libvips in Requirements Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The external_at_exit flag was overkill — @Finalized in #finalize already prevents double work. Simpler: at_exit always registers, framework adapters also register native hooks, @Finalized ensures only the first call does work. Also move custom framework docs from README to docs/framework-setup.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@README.md`:
- Around line 197-206: Update the README's "Custom Test Frameworks" example to
document and recommend setting the external_at_exit flag before calling
finalize_reporters! so the HTML reporter will skip its at_exit hook; mention
CapybaraScreenshotDiff.external_at_exit = true, show the call to
CapybaraScreenshotDiff.finalize_reporters! in the framework "after suite" hook,
and change the explanatory text to say the report/summary will be generated once
after the suite completes; reference the reporters/html.rb behavior (it checks
CapybaraScreenshotDiff.external_at_exit?) so readers understand why the flag is
required for custom runners.
🪄 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: b72ba67b-222a-4271-9552-7cb124e63b9c
📒 Files selected for processing (6)
README.mdlib/capybara_screenshot_diff/cucumber.rblib/capybara_screenshot_diff/minitest.rblib/capybara_screenshot_diff/reporters/html.rblib/capybara_screenshot_diff/rspec.rblib/capybara_screenshot_diff/screenshot_assertion.rb
Framework adapters (Minitest, RSpec, Cucumber) call finalize_reporters! via native hooks. at_exit was unreliable due to LIFO ordering and added complexity for a case nobody hits. Custom frameworks can call finalize_reporters! manually (documented in docs/framework-setup.md). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
at_exitfallback when no known framework (Minitest/RSpec/Cucumber) is loadedfinalize_reporters!via native hooks —at_exitwas a wasted no-opTest plan
🤖 Generated with Claude Code
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
Release Notes
Documentation
brew install vips) and Ubuntu (apt-get install libvips-dev) setup commands for the vips driverImprovements