Merged
Conversation
Contributor
Reviewer's GuideMakes reporter notification and HTML reporter operations thread-safe by introducing mutex-protected snapshots and internal locks, adds tests to guard synchronization behavior, and documents snap_diff thread-safety guarantees. Sequence diagram for thread-safe reporter notification and HTML reporter operationssequenceDiagram
actor TestThread
participant ScreenshotAssertion
participant ReportersMutex
participant HtmlReporter
TestThread->>ScreenshotAssertion: notify_reporters(assertions)
ScreenshotAssertion->>ScreenshotAssertion: validate assertions not nil/empty
ScreenshotAssertion->>ReportersMutex: synchronize
activate ReportersMutex
ScreenshotAssertion->>ScreenshotAssertion: reporters_snapshot = reporters.dup
ReportersMutex-->>ScreenshotAssertion: release lock
deactivate ReportersMutex
ScreenshotAssertion->>ScreenshotAssertion: return if reporters_snapshot.empty?
loop for each reporter in reporters_snapshot
ScreenshotAssertion->>HtmlReporter: record(assertions)
activate HtmlReporter
HtmlReporter->>HtmlReporter: return if @finalized
HtmlReporter->>HtmlReporter: local failures = []
HtmlReporter->>HtmlReporter: local total = 0
HtmlReporter->>HtmlReporter: iterate assertions, build failures and total
HtmlReporter->>HtmlReporter: synchronize on @mutex
activate HtmlReporter
HtmlReporter->>HtmlReporter: return if @finalized
HtmlReporter->>HtmlReporter: @total += local total
HtmlReporter->>HtmlReporter: @failures.concat(local failures)
HtmlReporter-->>ScreenshotAssertion: release @mutex
deactivate HtmlReporter
deactivate HtmlReporter
end
TestThread->>HtmlReporter: finalize
activate HtmlReporter
HtmlReporter->>HtmlReporter: synchronize on @mutex
activate HtmlReporter
HtmlReporter->>HtmlReporter: return if @finalized
HtmlReporter->>HtmlReporter: @finalized = true
HtmlReporter->>HtmlReporter: return if failures.empty?
HtmlReporter->>HtmlReporter: write_report
HtmlReporter-->>TestThread: output_path
deactivate HtmlReporter
deactivate HtmlReporter
Updated class diagram for ScreenshotAssertion and HtmlReporter thread safetyclassDiagram
class ScreenshotAssertion {
+Array reporters
+Mutex reporters_mutex
+reporters()
+reporters_mutex()
-notify_reporters(assertions)
+screenshot_namer()
+verify()
}
class HtmlReporter {
-String output_path
-Boolean embed_images
-Array failures
-Integer total
-Boolean finalized
-Mutex mutex
+initialize(output_path, embed_images)
+record(assertions)
+finalize()
+passed()
-failure_entry_for(name, compare)
-write_report()
}
ScreenshotAssertion "1" o-- "*" HtmlReporter : reporters
ScreenshotAssertion --> "1" Mutex : reporters_mutex
HtmlReporter --> "1" Mutex : mutex
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 8 minutes and 43 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 (5)
✨ 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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test/unit/reporters_mutex_test.rb" line_range="18-10" />
<code_context>
+ CapybaraScreenshotDiff.instance_variable_set(:@reporters_mutex, nil)
+ end
+
+ test "reporters notification uses mutex snapshot" do
+ fake_mutex = Class.new do
+ attr_reader :synchronize_calls
+
+ def initialize
+ @synchronize_calls = 0
+ end
+
+ def synchronize
</code_context>
<issue_to_address>
**suggestion (testing):** Broaden coverage to assert that the reporters snapshot is actually used
This test only verifies that `reporters_mutex.synchronize` is invoked; it doesn’t confirm that iteration uses the snapshot (`reporters.dup`), which is the core thread-safety behavior.
Please add a test that configures a reporter whose `record` mutates `CapybaraScreenshotDiff.reporters` (e.g., clearing it or appending another reporter) and asserts that:
- The iteration is unaffected by this mutation (only the original reporter(s) receive `record`), and
- No error is raised during notification.
That would explicitly validate that the loop operates on a snapshot rather than the live array.
Suggested implementation:
```ruby
CapybaraScreenshotDiff.instance_variable_set(:@reporters_mutex, nil)
end
test "reporters notification uses mutex snapshot" do
fake_mutex = Class.new do
attr_reader :synchronize_calls
def initialize
@synchronize_calls = 0
end
def synchronize
@synchronize_calls += 1
yield
end
end.new
CapybaraScreenshotDiff.instance_variable_set(:@reporters_mutex, fake_mutex)
reporter = Class.new do
def record(_assertions); end
end.new
CapybaraScreenshotDiff.reporters << reporter
# exercise notification, ensuring it goes through the mutex
CapybaraScreenshotDiff.notify_reporters([])
assert_equal 1, fake_mutex.synchronize_calls
end
test "reporters notification iterates over reporters snapshot" do
received = []
mutating_reporter = Class.new do
define_method :record do |assertions|
received << [:original, assertions]
# Mutate the reporters collection during notification to ensure
# that iteration is done on a snapshot (reporters.dup), not on
# the live array.
CapybaraScreenshotDiff.reporters.clear
CapybaraScreenshotDiff.reporters << Class.new {
define_method(:record) { |a| received << [:added, a] }
}.new
end
end.new
CapybaraScreenshotDiff.reporters << mutating_reporter
assertions = [:some, :assertions]
# This should not raise even though reporters are mutated while iterating
CapybaraScreenshotDiff.notify_reporters(assertions)
# Only the originally-registered reporter should have been invoked
assert_equal [[:original, assertions]], received
end
```
If the notification method on `CapybaraScreenshotDiff` is named differently (e.g., `notify` or `notify_assertions` rather than `notify_reporters`), update both test cases to call the correct method. Ensure that any existing tests in this file use the same API so the naming is consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Eagerly initialize reporters_mutex to prevent race on lazy init - Move @Finalized flag after write_report so finalize can retry on failure - Use mutex-protected snapshot in at_exit hook for consistency - Fix THREAD_SAFETY.md: use proper Ruby class names, document eager mutex - Add real multi-threaded concurrency test (10 threads x 5 assertions) - Add finalize retry-after-failure test 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:
tests:
Summary by Sourcery
Ensure thread-safe reporter handling and document thread safety guarantees for snap_diff.
Enhancements:
Tests: