From 0f3a8f62d2385c1b4832a28ff2ab112bf2874aaa Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:23:42 +0200 Subject: [PATCH 1/4] fix: skip_area selectors that match nothing no longer block for 5s (#272) Capybara's `all` defaults to `minimum: 1` and blocks in `synchronize` until that count is satisfied, so every `skip_area` (or `crop`) selector matching nothing burned a full `Capybara.default_max_wait_time` -- 5s by default, per selector, per screenshot. Measured with a real browser at Capybara's shipped 5s default, `%w[picture img]` against an image-less page: 10.012s before, 0.009s after. One project reported that exact scenario as 44% of their whole suite. The cost is only half of it. `skip_area` is a MASK -- "exclude whatever is currently there". Waiting for an element to appear is the wrong semantic: a selector matching nothing has nothing to mask, and that answer is available immediately. `all_visible_regions_for` is the only Capybara finder in lib/; the rest of BrowserHelpers is execute_script/evaluate_script and driver introspection, none of which carry a count expectation to block on. So this is the class of bug, not one instance of it. The guard uses a REAL browser session. Every existing test of this path stubs the browser, and a stub answers instantly whether or not the selector matches -- which is exactly why a 5-second wait went unnoticed for years. Its budget is derived from the live `default_max_wait_time` rather than hardcoded, so lowering the suite's wait cannot quietly turn it into an assertion that passes while broken. --- lib/snap_diff/browser_helpers.rb | 13 ++++++++++- test/integration/browser_screenshot_test.rb | 26 +++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/snap_diff/browser_helpers.rb b/lib/snap_diff/browser_helpers.rb index ee129bb4..0bf7fa7e 100644 --- a/lib/snap_diff/browser_helpers.rb +++ b/lib/snap_diff/browser_helpers.rb @@ -98,8 +98,19 @@ def self.blur_from_focused_element ] JS + # `minimum: 0` is load-bearing (issue #272). Capybara's `all` defaults to + # `minimum: 1` and blocks in `synchronize` until the count is satisfied, + # so every `skip_area`/`crop` selector matching nothing burned a full + # `Capybara.default_max_wait_time` -- 5s by default, per selector, per + # screenshot. One project measured `%w[picture img]` on an image-less + # page at 10s per screenshot, 44% of their suite. + # + # Waiting is the wrong semantic here regardless of the cost: these + # selectors describe a MASK over whatever is currently on the page. A + # selector that matches nothing has nothing to mask, and that answer is + # available immediately. def self.all_visible_regions_for(selector) - BrowserHelpers.session.all(selector, visible: true).map { |el| region_for(el) } + BrowserHelpers.session.all(selector, visible: true, minimum: 0).map { |el| region_for(el) } end def self.region_for(element) diff --git a/test/integration/browser_screenshot_test.rb b/test/integration/browser_screenshot_test.rb index 5d83bca1..3ba620e2 100644 --- a/test/integration/browser_screenshot_test.rb +++ b/test/integration/browser_screenshot_test.rb @@ -210,6 +210,32 @@ def test_screenshot_selected_element assert_equal 2, label_bounds.size end + # A REAL browser session on purpose (issue #272). Every other test of this + # code path stubs the browser, and a stub answers instantly whether or not + # the selector matches -- which is exactly why a 5-second wait per + # unmatched `skip_area` selector went unnoticed for years. `skip_area` is a + # mask: a selector that matches nothing has nothing to mask, and that + # answer is available immediately. + test "bounds_for_css does not wait for selectors that match nothing" do + visit "/" + + # Budget: TWO unmatched selectors, so an implicit wait costs 2x this and + # the real work costs milliseconds. Derived from the live setting rather + # than hardcoded, so lowering the suite's wait cannot quietly turn the + # assertion into one that passes while broken. + budget = page.config.default_max_wait_time + + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + SnapDiff::BrowserHelpers.bounds_for_css("picture", "video") + elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started + + assert_operator elapsed, :<, budget, + "two unmatched skip_area selectors took #{elapsed.round(2)}s -- Capybara's implicit wait is back" + + # ... and a selector that DOES match still resolves. + assert_equal 1, SnapDiff::BrowserHelpers.bounds_for_css("img").size + end + test "rect_for for multiple elements returns first visible element" do visit "/index.html" From 4d06d05823777342f315af68882965a1ca35308e Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:44:10 +0200 Subject: [PATCH 2/4] fix: the end-of-run summary prints without a reporter registered (#269) `SnapDiff::Reporting.register` appears exactly once in the whole gem, at reporters/html.rb:140. So the honest summary line shipped bundled with the HTML report, and the documented Rails setup registered nothing: $ ruby -e 'require "snap_diff/integrations/minitest" puts SnapDiff::Reporting.reporters.size' 0 That line exists to catch the failure modes no per-assertion rule can see -- a run where zero system tests executed, or where an inherited GIT_DIR redirected every baseline lookup. `0 verified` is the only signal for either, and it was behind an opt-in require. Separates the two concerns: counting is core honesty, writing an HTML file is a feature. Reporting owns `verified`/`changed` and prints `counts_summary` unconditionally from `finalize!`; Reporters::HTML keeps the report file, stays opt-in, and its `summary` is now just the path of the file it wrote -- on its own line, and nil when it wrote nothing. So the counts print exactly once whether or not the reporter is loaded, and `0 verified` still shouts NOTHING WAS VERIFIED. The fork-parallel merge (#266) carries the new counters in the same fragment as the missing-baseline names, and its guard now goes through `Reporting.notify` -- the real path -- so it checks both halves the worker has to hand back. `count` warns and skips rather than raising: `notify` runs inside every test's teardown, and a raise there aborts SnapDiff.reset before it clears the registry, leaking one test's assertions into the next. Same contract the reporter loop already applies, and just as loud (unconditional, not DEBUG-gated). Adding it surfaced three test doubles that never implemented Comparison's `difference` reader -- they had been blowing up unnoticed inside HTML#record, which swallows under `if ENV["DEBUG"]`. docs: `configuration.md` recommended the LEGACY `Capybara::Screenshot.enabled` spelling inside the canonical config reference, in three places. --- docs/configuration.md | 9 +- docs/reporters.md | 24 ++++ lib/snap_diff/reporters/html.rb | 30 ++--- lib/snap_diff/reporting.rb | 130 ++++++++++++++++++--- test/fixtures/summary_line_case.rb | 5 +- test/integration/summary_line_test.rb | 39 ++++++- test/support/dsl_stub.rb | 9 +- test/test_helper.rb | 3 +- test/unit/diff_test.rb | 4 + test/unit/parallel_report_merge_test.rb | 20 ++-- test/unit/record_modes_test.rb | 2 +- test/unit/registry_concurrency_test.rb | 4 + test/unit/reporters/html_reporter_test.rb | 52 ++------- test/unit/reporters_mutex_test.rb | 5 +- test/unit/reporting_counts_test.rb | 136 ++++++++++++++++++++++ 15 files changed, 368 insertions(+), 104 deletions(-) create mode 100644 test/unit/reporting_counts_test.rb diff --git a/docs/configuration.md b/docs/configuration.md index 441ea914..cbc8dfe9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -48,7 +48,7 @@ exception: `Capybara::Screenshot.enabled` is `SnapDiff.config.screenshot_enabled `SnapDiff.config.enabled` is taken by `Capybara::Screenshot::Diff.enabled`. See [SnapDiff — the canonical API](snapdiff.md) for the full SnapDiff-native surface. -**Note:** Setting `Capybara::Screenshot.enabled = false` is sufficient to disable all screenshots. There is no need to define no-op modules or monkey-patch the gem. +**Note:** Setting `SnapDiff.config.screenshot_enabled = false` is sufficient to disable all screenshots. There is no need to define no-op modules or monkey-patch the gem. ## Record modes — accepting changes @@ -227,15 +227,18 @@ unless the desired window size can be achieved. If you want to skip taking screen shots, set ```ruby -Capybara::Screenshot.enabled = false +SnapDiff.config.screenshot_enabled = false ``` You can of course set this by an environment variable ```ruby -Capybara::Screenshot.enabled = ENV['TAKE_SCREENSHOTS'] +SnapDiff.config.screenshot_enabled = ENV['TAKE_SCREENSHOTS'] ``` +A disabled screenshot is not an assertion, and Minitest is told so: a test whose only assertion +was a screenshot reports as missing assertions rather than as a pass over nothing. + ### Disabling diff If you want to skip the assertion for change in the screen shot, set diff --git a/docs/reporters.md b/docs/reporters.md index 6bccae67..9882fcca 100644 --- a/docs/reporters.md +++ b/docs/reporters.md @@ -20,6 +20,30 @@ The report includes a sidebar with thumbnails, side-by-side comparison with diff **Note:** The report is not generated when all screenshots match. +## The end-of-run summary + +Every run ends with what it actually did, whether or not you require a reporter: + +``` +[snap_diff] 12 verified, 1 changed, 2 new (not verified). +``` + +- **verified** — a committed baseline existed and was compared +- **changed** — of those, the ones that differed +- **new** — captured but *not* compared, for want of a committed baseline: neither a pass nor a + failure. Commit the files to turn them into baselines. + +`0 verified` is printed as `NOTHING WAS VERIFIED`, because it is the only signal for the failures +no per-assertion rule can see: a `rake test` that ran zero system tests, or an inherited `GIT_DIR` +sending every baseline lookup to the wrong repository. Both leave a green suite that compared +nothing. + +Requiring `snap_diff/reporters/html` adds one more line, naming the file it wrote: + +``` +[snap_diff] Report: doc/screenshots/snap_diff_report.html +``` + ## Parallel test runs `finalize` — the hook that writes the report — runs from the framework's end-of-suite hook. Whether diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index 69035b53..9f79a2f4 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -83,31 +83,17 @@ def merge_state!(state) def passed = total - failures.size def failed = failures.size - # The last line of the run, and the only place it says what it - # actually did: + # Both customer personas named this path as the best output in the + # product, so it gets its own line -- but only when a report was + # actually written. # - # verified -- a committed baseline existed and was compared - # changed -- of those, the ones that differed - # new -- captured but NOT compared, for want of a committed - # baseline: neither a pass nor a failure + # The counts this used to carry moved to SnapDiff::Reporting (issue + # #269): they are printed for every user, and this file is not. See + # Reporting.counts_summary. # - # Printed on every run, passing or failing, and never nil. "N - # screenshots compared" counted only what it compared, so it was - # silent about exactly the screenshots it did not -- and silent - # altogether when it compared nothing, which is the one case worth - # shouting about: no assertion runs, so nothing else in the output - # can notice a suite that ran no tests, or a baseline lookup pointed - # at the wrong repository. + # @return [String, nil] nil when no report was written def summary - line = "[snap_diff] #{total} verified, #{failed} changed, " \ - "#{Reporting.missing_baselines_count} new (not verified)." - - return "#{line} NOTHING WAS VERIFIED -- no screenshot was compared to a committed baseline." if total.zero? - return line if failures.empty? - - # Both customer personas named this path as the best output in the - # product. It is only here when a report was actually written. - "#{line} Report: #{output_path}" + "[snap_diff] Report: #{output_path}" if @finalized end def render diff --git a/lib/snap_diff/reporting.rb b/lib/snap_diff/reporting.rb index 39b1915e..26b7a2ed 100644 --- a/lib/snap_diff/reporting.rb +++ b/lib/snap_diff/reporting.rb @@ -19,10 +19,25 @@ module Reporting @mutex = Mutex.new @missing_baselines = Set.new @rerecorded_baselines = Set.new + @verified = 0 + @changed = 0 class << self attr_reader :reporters, :mutex + # How many screenshots were compared to a committed baseline, and how + # many of those differed. + # + # These counters live HERE, not in a reporter (issue #269). Counting + # is core honesty; writing an HTML file is a feature. The summary + # exists to catch the failure modes no per-assertion rule can see -- a + # run where zero system tests executed, or where an inherited GIT_DIR + # redirected every baseline lookup -- and `0 verified` is the only + # signal for either. It shipped inside Reporters::HTML, the gem's one + # and only `register` call site, so the documented Rails setup (which + # requires just the Minitest integration) printed nothing at all. + attr_reader :verified, :changed + # Remembers a screenshot that had no COMMITTED baseline and was # therefore never compared. # @@ -43,9 +58,16 @@ def missing_baselines_count end # @api private - # Per-test isolation for this gem's own suite. - def reset_missing_baselines! - @mutex.synchronize { @missing_baselines.clear } + # Per-test isolation for this gem's own suite: everything {finalize!} + # reports, cleared in one call. One surface rather than one reset per + # tally, so a tally added later cannot be forgotten at the call site. + def reset_run_totals! + @mutex.synchronize do + @missing_baselines.clear + @rerecorded_baselines.clear + @verified = 0 + @changed = 0 + end end # Remembers a screenshot re-recorded by `record: :all` -- captured as @@ -58,11 +80,6 @@ def record_rerecorded_baseline(name) @mutex.synchronize { !!@rerecorded_baselines.add?(name) } end - # @api private - def reset_rerecorded_baselines! - @mutex.synchronize { @rerecorded_baselines.clear } - end - # Registers a reporter for the rest of the process. The canonical way # in: the append happens under the mutex, so concurrent registrations # cannot lose one (issue #217 item 2). `reporters` stays public and @@ -80,6 +97,18 @@ def register(reporter) def notify(assertions) return if assertions.nil? || assertions.empty? + # Warned about and skipped, never raised: `notify` runs inside every + # test's teardown (SnapDiff.reset), and a raise here would abort the + # reset before it clears the registry -- leaking one test's + # assertions into the next. A tally must not be able to take a + # user's suite down. Same contract the reporter loop below applies, + # and just as loud: unconditional, not DEBUG-gated. + begin + count(assertions) + rescue => e + warn "[snap_diff] Could not tally the run (#{e.class}: #{e.message})" + end + reporters_snapshot = @mutex.synchronize { @reporters.dup } return if reporters_snapshot.empty? @@ -90,10 +119,70 @@ def notify(assertions) end end - # End-of-suite hook: finalizes each reporter and prints its summary. - # A raising reporter is warned about and skipped; the rest are still - # finalized. + # Tallies a finished test's assertions. An assertion with no + # `compare` never reached a baseline, so it is neither verified nor + # changed -- it is counted, if at all, by {record_missing_baseline}. + def count(assertions) + verified = 0 + changed = 0 + + assertions.each do |assertion| + compare = assertion.compare + next unless compare + + verified += 1 + changed += 1 if compare.difference&.different? + end + + @mutex.synchronize do + @verified += verified + @changed += changed + end + end + + # The last line of the run, and the only place it says what it + # actually did: + # + # verified -- a committed baseline existed and was compared + # changed -- of those, the ones that differed + # new -- captured but NOT compared, for want of a committed + # baseline: neither a pass nor a failure + # + # Printed on every run, passing or failing, reporter or no reporter, + # and never nil. "N screenshots compared" counted only what it + # compared, so it was silent about exactly the screenshots it did not + # -- and silent altogether when it compared nothing, which is the one + # case worth shouting about. + def counts_summary + verified, changed, new_count, rerecorded = @mutex.synchronize { + [@verified, @changed, @missing_baselines.size, @rerecorded_baselines.size] + } + line = "[snap_diff] #{verified} verified, #{changed} changed, #{new_count} new (not verified)." + + # `record: :all` (#274) accepts the rendering as the new baseline + # without comparing, so those are neither verified nor changed -- + # and not "new" either, which is a different fact. Only shown when + # it happened; the names are on their own line below. + line += " #{rerecorded} re-recorded (not verified)." if rerecorded.positive? + + # The shout is for an UNEXPLAINED zero -- a suite that ran no system + # tests, a GIT_DIR pointed at the wrong repository. Re-recording + # explains it, and the user asked for it: shouting there is a false + # alarm, and false alarms are how the real one stops being read. + if verified.zero? && rerecorded.zero? + return "#{line} NOTHING WAS VERIFIED -- no screenshot was compared to a committed baseline." + end + + line + end + + # End-of-suite hook: prints the counts, then finalizes each reporter + # and prints its summary. A raising reporter is warned about and + # skipped; the rest are still finalized -- and the counts line is + # already out, so no reporter can take it down with it. def finalize! + $stdout.puts counts_summary + @mutex.synchronize { @reporters.dup }.each do |reporter| reporter.finalize if (msg = reporter.summary) @@ -160,6 +249,8 @@ def dump_parallel_fragment payload = { "missing_baselines" => @mutex.synchronize { @missing_baselines.to_a }, "rerecorded_baselines" => @mutex.synchronize { @rerecorded_baselines.to_a }, + "verified" => @verified, + "changed" => @changed, "reporters" => @mutex.synchronize { @reporters.dup } .map { |reporter| reporter.dump_state if reporter.respond_to?(:dump_state) } } @@ -182,10 +273,19 @@ def merge_parallel_fragments! Dir[File.join(parallel_fragments_dir, "*.json")].sort.each do |fragment| payload = JSON.parse(File.read(fragment)) - @mutex.synchronize { payload["missing_baselines"].each { |name| @missing_baselines << name } } - # `to_a` on a fresh install predates this key: a fragment written - # by an older worker has no "rerecorded_baselines" at all. - @mutex.synchronize { payload.fetch("rerecorded_baselines", []).each { |name| @rerecorded_baselines << name } } + # Only "missing_baselines" is read without a default: it is the + # one key every version of this fragment has ever written. Every + # key added since is `fetch`ed with one, because the fragments + # directory is keyed by pid under the system temp dir -- a + # recycled pid can hand this merge a fragment left behind by an + # older version of the gem, and a partial payload must not take + # the run down. + @mutex.synchronize do + payload["missing_baselines"].each { |name| @missing_baselines << name } + payload.fetch("rerecorded_baselines", []).each { |name| @rerecorded_baselines << name } + @verified += payload.fetch("verified", 0) + @changed += payload.fetch("changed", 0) + end reporters_snapshot = @mutex.synchronize { @reporters.dup } payload["reporters"].each_with_index do |state, index| diff --git a/test/fixtures/summary_line_case.rb b/test/fixtures/summary_line_case.rb index 4b589e07..8e020da4 100644 --- a/test/fixtures/summary_line_case.rb +++ b/test/fixtures/summary_line_case.rb @@ -13,7 +13,10 @@ # SNAP_CASES -- comma-separated subset of verified,changed,new (may be empty) require "minitest/autorun" require "snap_diff/integrations/minitest" -require "snap_diff/reporters/html" +# The HTML report is a FEATURE and stays opt-in; the summary line is core +# honesty and must print either way. SNAP_NO_REPORTER runs the documented +# Rails setup, which registers no reporter at all. +require "snap_diff/reporters/html" unless ENV["SNAP_NO_REPORTER"] require "fileutils" require "pathname" diff --git a/test/integration/summary_line_test.rb b/test/integration/summary_line_test.rb index c84ab45b..84e8664c 100644 --- a/test/integration/summary_line_test.rb +++ b/test/integration/summary_line_test.rb @@ -25,10 +25,14 @@ class SummaryLineTest < ActiveSupport::TestCase out, status = run_case("verified,changed,new") refute status.success?, out - assert_match( - %r{\[snap_diff\] 2 verified, 1 changed, 1 new \(not verified\)\. Report: /\S+snap_diff_report\.html}, - out - ) + assert_includes out, "[snap_diff] 2 verified, 1 changed, 1 new (not verified)." + # Its own line since the counts left the HTML reporter (issue #269): the + # counts print for everyone, the report path only when a file was written. + assert_match(%r{^\[snap_diff\] Report: /\S+snap_diff_report\.html$}, out) + # Exactly once. Reporting prints the counts and the HTML reporter prints + # the path; if the reporter ever carries the counts again, the run ends + # on the same line twice. + assert_equal 1, out.scan("verified,").size, "the counts line was printed more than once" end # The case this line exists for: `rake test` running zero system tests, or @@ -42,11 +46,33 @@ class SummaryLineTest < ActiveSupport::TestCase assert_includes out, "NOTHING WAS VERIFIED" end + # The summary is the only thing that can see a run where zero system tests + # executed, or where an inherited GIT_DIR redirected every baseline lookup. + # It shipped registered by `snap_diff/reporters/html` -- so the documented + # Rails setup, which requires only the Minitest integration, printed + # nothing at all. Counting is core honesty; writing an HTML file is a + # feature, and only the feature is opt-in. + test "the summary prints with no reporter registered" do + out, status = run_case("verified", reporter: false) + + assert status.success?, out + assert_includes out, "[snap_diff] 1 verified, 0 changed, 0 new (not verified)." + refute_includes out, "Report:", "no reporter was registered, so no report was written" + end + + test "a run that verified nothing still shouts with no reporter registered" do + out, status = run_case("", reporter: false) + + assert status.success?, out + assert_includes out, "[snap_diff] 0 verified, 0 changed, 0 new (not verified)." + assert_includes out, "NOTHING WAS VERIFIED" + end + private # Builds a throwaway git repo with COMMITTED baselines for `verified` and # `changed` (none for `new`), then runs the user's test file against it. - def run_case(cases) + def run_case(cases, reporter: true) Dir.mktmpdir do |dir| # macOS hands out /var/... symlinks; git reports the physical path, and # baseline lookup is a relative_path_from between the two. @@ -60,7 +86,8 @@ def run_case(cases) Open3.capture2e(*git, "commit", "-qm", "baselines") Open3.capture2e( - {"SNAP_ROOT" => repo, "SNAP_IMAGES" => TEST_IMAGES_DIR.to_s, "SNAP_CASES" => cases, "CI" => nil}, + {"SNAP_ROOT" => repo, "SNAP_IMAGES" => TEST_IMAGES_DIR.to_s, "SNAP_CASES" => cases, "CI" => nil, + "SNAP_NO_REPORTER" => (reporter ? nil : "1")}, RbConfig.ruby, "-Ilib", "-Itest", file_fixture("summary_line_case.rb").to_s ) end diff --git a/test/support/dsl_stub.rb b/test/support/dsl_stub.rb index 5b7db45a..161ffd65 100644 --- a/test/support/dsl_stub.rb +++ b/test/support/dsl_stub.rb @@ -35,8 +35,12 @@ def set_test_images(snap, expected, actual) @manager.provision_snap_with(snap, fixture_image_path_from(expected, snap.format), version: :base) end + # `difference` is part of the real Comparison contract (attr_reader), and + # SnapDiff::Reporting.count reads it to tally the run without triggering + # a comparison the way `different?` would. ImageCompareStub = Struct.new( - :driver, :driver_options, :shift_distance_limit, :quick_equal?, :different?, :reporter, keyword_init: true + :driver, :driver_options, :shift_distance_limit, :quick_equal?, :different?, :difference, :reporter, + keyword_init: true ) def build_image_compare_stub(equal: true) @@ -46,7 +50,8 @@ def build_image_compare_stub(equal: true) driver_options: SnapDiff.config.default_options, shift_distance_limit: nil, quick_equal?: equal, - different?: !equal + different?: !equal, + difference: TestDoubles::TestDifference.new(!equal) ) end diff --git a/test/test_helper.rb b/test/test_helper.rb index 8d211749..a6baf4ee 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -116,8 +116,7 @@ class ActiveSupport::TestCase # Process-global, like the reporter list: without this the whole suite's # baseline-less screenshots pile up and get listed in one enormous line # at the end of `rake test`. - SnapDiff::Reporting.reset_missing_baselines! - SnapDiff::Reporting.reset_rerecorded_baselines! + SnapDiff::Reporting.reset_run_totals! end def persist_comparisons? diff --git a/test/unit/diff_test.rb b/test/unit/diff_test.rb index bbafabe1..a7fb6055 100644 --- a/test/unit/diff_test.rb +++ b/test/unit/diff_test.rb @@ -147,6 +147,8 @@ def _test_sample_screenshot_error mock = ::Minitest::Mock.new mock.expect(:different?, true) mock.expect(:different?, true) + # Read by SnapDiff::Reporting.count when the test ends. + mock.expect(:difference, TestDoubles::TestDifference.new(true)) mock.expect(:dimensions_changed?, false) mock.expect(:base_image_path, Pathname.new("screenshot.base.png")) mock.expect(:error_message, "expected error message") @@ -182,6 +184,8 @@ def _test_sample_screenshot_error comparison = ::Minitest::Mock.new comparison.expect(:different?, true) # to find backtrace comparison.expect(:different?, true) # to find messages + # Read by SnapDiff::Reporting.count when the test ends. + comparison.expect(:difference, TestDoubles::TestDifference.new(true)) comparison.expect(:dimensions_changed?, false) comparison.expect(:base_image_path, Pathname.new("screenshot.base.png")) comparison.expect(:error_message, "expected error message for non minitest") diff --git a/test/unit/parallel_report_merge_test.rb b/test/unit/parallel_report_merge_test.rb index 13cbd371..11fa2f9a 100644 --- a/test/unit/parallel_report_merge_test.rb +++ b/test/unit/parallel_report_merge_test.rb @@ -38,24 +38,29 @@ class ParallelReportMergeTest < ActiveSupport::TestCase FileUtils.rm_rf(SnapDiff::Reporting.parallel_fragments_dir) end - test "records from forked workers reach the parent's report and summary" do - fork_worker { @reporter.record([build_failing_assertion("worker_a")]) } + # Through `Reporting.notify`, the real path: a worker has to hand back BOTH + # halves -- the reporter's records and the counts Reporting keeps itself + # (issue #269) -- or the merged run is wrong in exactly the case this whole + # file is about. + test "records and counts from forked workers reach the parent" do + fork_worker { SnapDiff::Reporting.notify([build_failing_assertion("worker_a")]) } fork_worker do - @reporter.record([build_failing_assertion("worker_b"), build_passing_assertion("worker_b_ok")]) + SnapDiff::Reporting.notify([build_failing_assertion("worker_b"), build_passing_assertion("worker_b_ok")]) SnapDiff::Reporting.record_missing_baseline("worker_b_new") end # The bug, restated as an assertion: the parent holds nothing of its own. assert_equal 0, @reporter.total + assert_equal 0, SnapDiff::Reporting.verified SnapDiff::Reporting.merge_parallel_fragments! assert_equal 3, @reporter.total assert_equal 2, @reporter.failed - # The summary line must count the MERGED totals -- one worker's numbers - # would be wrong in exactly the case this whole file is about. - assert_equal "[snap_diff] 3 verified, 2 changed, 1 new (not verified). Report: #{@output_path}", - @reporter.summary + # The counts line must total the MERGED runs -- one worker's numbers + # would be wrong here. + assert_equal "[snap_diff] 3 verified, 2 changed, 1 new (not verified).", + SnapDiff::Reporting.counts_summary # Symbol keys, like an entry recorded in this process: `failures` is # public, and an array whose shape depends on which process filled it @@ -65,6 +70,7 @@ class ParallelReportMergeTest < ActiveSupport::TestCase @reporter.finalize assert_predicate @output_path, :exist? + assert_equal "[snap_diff] Report: #{@output_path}", @reporter.summary end test "the parent removes the fragments it merged" do diff --git a/test/unit/record_modes_test.rb b/test/unit/record_modes_test.rb index f943cd9d..3870e839 100644 --- a/test/unit/record_modes_test.rb +++ b/test/unit/record_modes_test.rb @@ -38,7 +38,7 @@ def after_teardown super SnapDiff.config.root = @original_root FileUtils.remove_entry(@new_root) if @new_root - SnapDiff::Reporting.reset_rerecorded_baselines! + SnapDiff::Reporting.reset_run_totals! end # --- the default is not touched -------------------------------------- diff --git a/test/unit/registry_concurrency_test.rb b/test/unit/registry_concurrency_test.rb index 44221ee8..50bf5bb2 100644 --- a/test/unit/registry_concurrency_test.rb +++ b/test/unit/registry_concurrency_test.rb @@ -13,6 +13,10 @@ class RegistryConcurrencyTest < ActiveSupport::TestCase PassingCompare = Struct.new(:name) do def different? = false + # A processed comparison that matched: SnapDiff::Reporting.count reads + # this to tally the run. + def difference = TestDoubles::TestDifference.new(false) + def base_image_path = Pathname.new("/nonexistent/#{name}.base.png") end diff --git a/test/unit/reporters/html_reporter_test.rb b/test/unit/reporters/html_reporter_test.rb index bd3ed3ff..e1250f57 100644 --- a/test/unit/reporters/html_reporter_test.rb +++ b/test/unit/reporters/html_reporter_test.rb @@ -170,59 +170,23 @@ def synchronize assert_nil result end - test "#summary counts what was verified, what changed, and what was never compared" do - SnapDiff::Reporting.record_missing_baseline("never_compared") - + # The COUNTS moved to SnapDiff::Reporting (issue #269) -- see + # test/unit/reporting_counts_test.rb. What is left here is the one thing + # only this reporter can say: where the file it wrote went. + test "#summary names the report it wrote" do reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) - reporter.record([build_passing_assertion("ok"), build_failing_assertion("fail")]) - reporter.finalize - - assert_equal "[snap_diff] 2 verified, 1 changed, 1 new (not verified). Report: #{@output_path}", - reporter.summary - end - - test "#summary counts every changed screenshot, not just the first" do - reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) - reporter.record([ - build_failing_assertion("first failure"), - build_failing_assertion("second failure") - ]) + reporter.record([build_failing_assertion("fail")]) reporter.finalize - summary = reporter.summary - assert_includes summary, "2 verified, 2 changed" - assert_includes summary, @output_path.to_s + assert_equal "[snap_diff] Report: #{@output_path}", reporter.summary end - test "#summary when all pass reports zero changed and omits the report path" do + test "#summary is nil when no report was written" do reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) reporter.record([build_passing_assertion("ok")]) reporter.finalize - summary = reporter.summary - assert_equal "[snap_diff] 1 verified, 0 changed, 0 new (not verified).", summary - refute_includes summary, @output_path.to_s - end - - # Zero verified is the whole tell for the failure modes no assertion can - # report: a suite that ran no system tests at all, or one whose baseline - # lookup was redirected to another repository. The line must be printed - # (never nil) and must not read like a pass. - test "#summary shouts when nothing was verified" do - reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) - - assert_equal "[snap_diff] 0 verified, 0 changed, 0 new (not verified). " \ - "NOTHING WAS VERIFIED -- no screenshot was compared to a committed baseline.", - reporter.summary - end - - test "#summary counts screenshots captured with no committed baseline even when nothing was verified" do - SnapDiff::Reporting.record_missing_baseline("a") - SnapDiff::Reporting.record_missing_baseline("b") - - reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) - - assert_includes reporter.summary, "0 verified, 0 changed, 2 new (not verified)." + assert_nil reporter.summary, "nothing was written, so there is no path to name" end test "#finalize can retry after write_report failure" do diff --git a/test/unit/reporters_mutex_test.rb b/test/unit/reporters_mutex_test.rb index 07010acd..deedcb69 100644 --- a/test/unit/reporters_mutex_test.rb +++ b/test/unit/reporters_mutex_test.rb @@ -60,7 +60,10 @@ class ReportersMutexTest < ActiveSupport::TestCase SnapDiff::Reporting.reporters << mutating_reporter - assertions = [:some, :assertions] + # Shaped like real assertions: notify tallies them on the way through, + # and a bare Symbol would make it warn about a double, not about the + # snapshot behaviour under test. + assertions = [SnapDiff::ScreenshotAssertion.new("some"), SnapDiff::ScreenshotAssertion.new("assertions")] assert_nothing_raised do SnapDiff::Reporting.notify(assertions) diff --git a/test/unit/reporting_counts_test.rb b/test/unit/reporting_counts_test.rb new file mode 100644 index 00000000..dab6b8d6 --- /dev/null +++ b/test/unit/reporting_counts_test.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require "test_helper" +require "snap_diff" + +# The end-of-run counts, unit-sized. Their home is SnapDiff::Reporting, not +# a reporter (issue #269): counting is core honesty, writing an HTML file is +# a feature, and the summary exists to catch what no per-assertion rule can +# see -- a run where zero system tests executed, or where an inherited +# GIT_DIR redirected every baseline lookup. +# +# That the line actually reaches a real run's stdout with no reporter +# registered is asserted where it can only be asserted, on a finished +# process: test/integration/summary_line_test.rb. +class ReportingCountsTest < ActiveSupport::TestCase + include DSLStub + + # No reporter registered here on purpose: the counts must not depend on one. + setup do + SnapDiff::Reporting.mutex.synchronize do + @original_reporters = SnapDiff::Reporting.reporters.dup + SnapDiff::Reporting.reporters.clear + end + end + + teardown do + SnapDiff::Reporting.mutex.synchronize do + SnapDiff::Reporting.reporters.clear + SnapDiff::Reporting.reporters.concat(@original_reporters) + end + end + + test "counts what was verified, what changed, and what was never compared" do + SnapDiff::Reporting.record_missing_baseline("never_compared") + SnapDiff::Reporting.notify([build_passing_assertion("ok"), build_failing_assertion("fail")]) + + assert_equal "[snap_diff] 2 verified, 1 changed, 1 new (not verified).", + SnapDiff::Reporting.counts_summary + end + + test "counts every changed screenshot, not just the first" do + SnapDiff::Reporting.notify([build_failing_assertion("first"), build_failing_assertion("second")]) + + assert_includes SnapDiff::Reporting.counts_summary, "2 verified, 2 changed" + end + + test "counts accumulate across tests" do + SnapDiff::Reporting.notify([build_passing_assertion("one")]) + SnapDiff::Reporting.notify([build_failing_assertion("two")]) + + assert_includes SnapDiff::Reporting.counts_summary, "2 verified, 1 changed" + end + + # Zero verified is the whole tell for the failure modes no assertion can + # report: a suite that ran no system tests at all, or one whose baseline + # lookup was redirected to another repository. The line must never be nil + # and must not read like a pass. + test "shouts when nothing was verified" do + assert_equal "[snap_diff] 0 verified, 0 changed, 0 new (not verified). " \ + "NOTHING WAS VERIFIED -- no screenshot was compared to a committed baseline.", + SnapDiff::Reporting.counts_summary + end + + # `record: :all` (#274) re-records without comparing, so those screenshots + # are neither verified nor changed -- and they are not "new" either, which + # is a different fact with its own line. The counts line has to say what + # happened, and must NOT cry NOTHING WAS VERIFIED at a user who asked for + # exactly this: a false alarm here trains people to ignore the real one. + test "re-recorded screenshots are counted, and explain a zero verified" do + SnapDiff::Reporting.record_rerecorded_baseline("a") + SnapDiff::Reporting.record_rerecorded_baseline("b") + + summary = SnapDiff::Reporting.counts_summary + + assert_includes summary, "0 verified, 0 changed, 0 new (not verified)." + assert_includes summary, "2 re-recorded" + refute_includes summary, "NOTHING WAS VERIFIED" + end + + # The shout is for an UNEXPLAINED zero -- that is the whole point of it. + test "a zero verified with nothing re-recorded still shouts" do + assert_includes SnapDiff::Reporting.counts_summary, "NOTHING WAS VERIFIED" + end + + # `record:` is a per-screenshot option too, so a run can mix both. + test "a run that verified some and re-recorded others reports both" do + SnapDiff::Reporting.notify([build_passing_assertion("ok")]) + SnapDiff::Reporting.record_rerecorded_baseline("accepted") + + summary = SnapDiff::Reporting.counts_summary + + assert_includes summary, "1 verified, 0 changed, 0 new (not verified)." + assert_includes summary, "1 re-recorded" + end + + test "counts screenshots captured with no committed baseline even when nothing was verified" do + SnapDiff::Reporting.record_missing_baseline("a") + SnapDiff::Reporting.record_missing_baseline("b") + + assert_includes SnapDiff::Reporting.counts_summary, "0 verified, 0 changed, 2 new (not verified)." + end + + # The reason this moved: the gem's one and only `register` call site is in + # reporters/html.rb, so the documented Rails setup registers nothing. + test "finalize! prints the counts with no reporter registered" do + SnapDiff::Reporting.notify([build_passing_assertion("ok")]) + + out, _err = capture_io { SnapDiff::Reporting.finalize! } + + assert_empty SnapDiff::Reporting.reporters + assert_includes out, "[snap_diff] 1 verified, 0 changed, 0 new (not verified)." + end + + test "an assertion with no comparison is neither verified nor changed" do + SnapDiff::Reporting.notify([SnapDiff::ScreenshotAssertion.new("never_compared")]) + + assert_includes SnapDiff::Reporting.counts_summary, "0 verified, 0 changed" + end + + private + + def build_passing_assertion(name) + build_assertion(name, :a, :a, "pass") + end + + def build_failing_assertion(name) + build_assertion(name, :a, :b, "fail") + end + + def build_assertion(name, base, new, prefix) + compare = make_comparison(base, new, destination: "#{prefix}_#{name}") + compare.processed + + SnapDiff::ScreenshotAssertion.new(name).tap { |assertion| assertion.compare = compare } + end +end From 57f7ac369d961544e6e909faafe6c897a6aa9ad3 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:50:30 +0200 Subject: [PATCH 3/4] fix: a disabled screenshot no longer counts as an assertion (#270) `integrations/minitest.rb` incremented the counter before the `active?` guard inside `super`, which returns false immediately when screenshots are disabled. So a test whose only assertion was a screenshot reported `1 runs, 1 assertions, 0 failures` -- nothing captured, nothing compared, and a green line claiming otherwise. Counting only when active hands the alarm to Rails for free. Rails unconditionally prepends ActiveSupport::Testing::TestsWithoutAssertions into every ActiveSupport::TestCase (test_case.rb:205), so those tests now warn: Test is missing assertions: `test_it` .../my_test.rb:12 Guarded in both directions with the real Rails module, not a stand-in: the alarm fires for a test whose sole assertion was a disabled screenshot, and stays quiet both for a test that asserts something else and for a screenshot that actually ran. Audited the other two adapters, neither needs a change: - RSpec's matcher returns a literal `true`, which is correct rather than the same bug: returning `assert_matches_screenshot`'s false would fail the example over a config switch the user set on purpose, and a real mismatch raises rather than returning false. RSpec has no assertion count to correct and no missing-assertion alarm to trigger, so only the end-of-run `0 verified` line can see a disabled run. Said so at the call site, since the next reader will otherwise "fix" it. - Cucumber counts nothing; `SnapDiff::DSL#assert_matches_screenshot` already returns false when inactive. --- lib/snap_diff/integrations/minitest.rb | 14 +++++- lib/snap_diff/integrations/rspec.rb | 11 +++++ test/unit/minitest_assertions_test.rb | 61 +++++++++++++++++++++++++- 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/lib/snap_diff/integrations/minitest.rb b/lib/snap_diff/integrations/minitest.rb index e9334dd9..0ca379b4 100644 --- a/lib/snap_diff/integrations/minitest.rb +++ b/lib/snap_diff/integrations/minitest.rb @@ -31,8 +31,20 @@ module Minitest module Assertions include ::SnapDiff::DSL + # The `if` is the whole point (issue #270). `super` returns false + # immediately when screenshots are disabled -- nothing captured, + # nothing compared -- so counting unconditionally reported + # `1 runs, 1 assertions, 0 failures` over a test that asserted + # nothing at all. + # + # Getting the count right hands the alarm to Rails for free: it + # prepends ActiveSupport::Testing::TestsWithoutAssertions into every + # ActiveSupport::TestCase (test_case.rb:205), which warns + # "Test is missing assertions: `test_x`" on exactly the tests whose + # sole assertion was a disabled screenshot -- and stays quiet for + # tests that assert something else. def assert_matches_screenshot(*args, skip_stack_frames: 0, **opts) - self.assertions += 1 + self.assertions += 1 if SnapDiff.config.active? super(*args, skip_stack_frames: skip_stack_frames + 1, **opts) rescue ::SnapDiff::ExpectationNotMet => e diff --git a/lib/snap_diff/integrations/rspec.rb b/lib/snap_diff/integrations/rspec.rb index 178ae802..732b9148 100644 --- a/lib/snap_diff/integrations/rspec.rb +++ b/lib/snap_diff/integrations/rspec.rb @@ -9,6 +9,17 @@ RSpec::Matchers.define :match_screenshot do |name, **options| description { "match screenshot '#{name}'" } + # The literal `true` is deliberate, not the Minitest miscount of issue + # #270. `assert_matches_screenshot` returns false when screenshots are + # disabled, and returning that here would FAIL the example for a config + # switch the user set on purpose. A real mismatch does not come back as + # false either -- it raises SnapDiff::ExpectationNotMet, or is deferred + # to the append_after hook below. + # + # There is nothing to hand off to the way Minitest hands off to Rails' + # TestsWithoutAssertions: RSpec has no assertion count, so a disabled + # screenshot leaves an example that passed having checked nothing, and + # only the end-of-run `0 verified` line can see it. match do |_page| assert_matches_screenshot(name, **options) true diff --git a/test/unit/minitest_assertions_test.rb b/test/unit/minitest_assertions_test.rb index 64963d4d..29011696 100644 --- a/test/unit/minitest_assertions_test.rb +++ b/test/unit/minitest_assertions_test.rb @@ -10,8 +10,14 @@ class MinitestAssertionsTest < ActiveSupport::TestCase # @param teardown [Proc, nil] optional replacement `teardown` method, to # simulate a user teardown that runs after `before_teardown`. Calls # `super()` first so DSLStub's own cleanup still happens. - def run_inner_test(teardown: nil, &block) + # @param like_rails [Boolean] prepend the module Rails prepends into every + # ActiveSupport::TestCase, to observe its missing-assertions alarm. + def run_inner_test(teardown: nil, like_rails: false, &block) test_class = Class.new(::Minitest::Test) do + # The real thing, not a stand-in: Rails prepends exactly this, + # unconditionally, at active_support/test_case.rb:205. + prepend ActiveSupport::Testing::TestsWithoutAssertions if like_rails + include SnapDiff::Minitest::Assertions include DSLStub @@ -47,6 +53,59 @@ def run_inner_test(teardown: nil, &block) end end + # Issue #270. The counter was bumped before the `active?` guard inside + # `super`, so with screenshots disabled a test whose only assertion was a + # screenshot reported `1 runs, 1 assertions, 0 failures` -- nothing + # captured, nothing compared, and a green line claiming otherwise. + test "a disabled screenshot is not counted as a Minitest assertion" do + SnapDiff.config.stub(:active?, false) do + result = run_inner_test { screenshot("a") } + + assert_predicate result, :passed? + assert_equal 0, result.assertions, "nothing was captured and nothing was compared" + end + end + + # The payoff: Rails prepends TestsWithoutAssertions into every + # ActiveSupport::TestCase, so a correct count turns Rails itself into a + # free per-test alarm for exactly this case. + test "Rails' missing-assertions alarm fires when the only assertion was a disabled screenshot" do + SnapDiff.config.stub(:active?, false) do + _out, err = capture_io do + run_inner_test(like_rails: true) { screenshot("a") } + end + + assert_match(/Test is missing assertions: `test_it`/, err) + end + end + + test "Rails' missing-assertions alarm stays quiet for a test with other assertions" do + SnapDiff.config.stub(:active?, false) do + _out, err = capture_io do + run_inner_test(like_rails: true) do + screenshot("a") + assert true + end + end + + refute_match(/Test is missing assertions/, err, + "the test asserted something; the disabled screenshot is not the whole story") + end + end + + # And the alarm must not fire when screenshots ARE active: the screenshot + # is the assertion then. + test "Rails' missing-assertions alarm stays quiet for an active screenshot" do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + _out, err = capture_io do + run_inner_test(like_rails: true) { screenshot("a") } + end + + # Not assert_empty: the no-committed-baseline notice shares this stream. + refute_match(/Test is missing assertions/, err) + end + end + test "#before_teardown does not mask a real teardown error behind a pending skip" do SnapDiff::Vcs.stub(:checkout_vcs, false) do SnapDiff.config.stub(:pending_if_new, true) do From 90a78c3151f9298f6e9cc2bacef77c381cc336c2 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:14:39 +0200 Subject: [PATCH 4/4] test: pin the #274 record-modes interactions the rebase created These three checks could not have been written before the rebase: the counts line (#269) and record modes (#274) never met until now. - The fork-parallel fragment carries BOTH tallies, so a worker that counts assertions AND re-records a baseline must hand back both and neither may cost the other. The main merge case now does both. - A fragment with none of the keys added since #266 still merges. The fragments directory is keyed by pid under the system temp dir, so a recycled pid can hand the merge a fragment written by an older version of the gem; every key but "missing_baselines" is read with a default for exactly that. Mutation-checked: dropping one default fails the merge with `TypeError: nil can't be coerced into Integer`. - `record: :all` end to end, on a finished process. It re-records without comparing, which through the summary path is neither verified nor changed, and NOT "new" either -- there was a baseline, it just was not consulted. --- test/fixtures/summary_line_case.rb | 16 ++++++++++++++-- test/integration/summary_line_test.rb | 23 +++++++++++++++++++++++ test/unit/parallel_report_merge_test.rb | 24 +++++++++++++++++++++++- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/test/fixtures/summary_line_case.rb b/test/fixtures/summary_line_case.rb index 8e020da4..1e526dce 100644 --- a/test/fixtures/summary_line_case.rb +++ b/test/fixtures/summary_line_case.rb @@ -34,7 +34,8 @@ class FileCopyScreenshoter < SnapDiff::Screenshoter CAPTURES = { "verified" => "a.png", "changed" => "b.png", - "new" => "a.png" + "new" => "a.png", + "rerecorded" => "b.png" }.freeze def take_screenshot(screenshot_path) @@ -52,7 +53,18 @@ def take_screenshot(screenshot_path) class SummaryLineCase < Minitest::Test include SnapDiff::Minitest::Assertions + # `record: :all` accepts whatever rendered as the new baseline. This one + # has a COMMITTED baseline that differs, and the mode ignores it -- the + # interaction worth asserting on a finished process. + RECORD_ALL = %w[rerecorded].freeze + CASES.each do |name| - define_method(:"test_#{name}") { assert_matches_screenshot(name) } + define_method(:"test_#{name}") do + if RECORD_ALL.include?(name) + assert_matches_screenshot(name, record: :all) + else + assert_matches_screenshot(name) + end + end end end diff --git a/test/integration/summary_line_test.rb b/test/integration/summary_line_test.rb index 84e8664c..b658a295 100644 --- a/test/integration/summary_line_test.rb +++ b/test/integration/summary_line_test.rb @@ -68,6 +68,28 @@ class SummaryLineTest < ActiveSupport::TestCase assert_includes out, "NOTHING WAS VERIFIED" end + # `record: :all` (#274) re-records without comparing. Through the summary + # path (#269) that is neither verified nor changed, and NOT "new" either + # -- there was a baseline, it was just not consulted. Asserted on a + # finished process because the two features never met before this branch. + test "a run that only re-recorded says so instead of crying NOTHING WAS VERIFIED" do + out, status = run_case("rerecorded") + + assert status.success?, out + assert_includes out, "[snap_diff] 0 verified, 0 changed, 0 new (not verified). 1 re-recorded (not verified)." + # The shout is for an UNEXPLAINED zero. The user asked for this one, and + # a false alarm here is how the real alarm stops being read. + refute_includes out, "NOTHING WAS VERIFIED" + assert_includes out, "record: :all re-recorded 1 screenshot WITHOUT comparing: rerecorded" + end + + test "a mixed run counts verified and re-recorded separately" do + out, status = run_case("verified,rerecorded") + + assert status.success?, out + assert_includes out, "[snap_diff] 1 verified, 0 changed, 0 new (not verified). 1 re-recorded (not verified)." + end + private # Builds a throwaway git repo with COMMITTED baselines for `verified` and @@ -80,6 +102,7 @@ def run_case(cases, reporter: true) FileUtils.mkdir_p("#{repo}/screenshots") FileUtils.cp(fixture_image_path_from("a"), "#{repo}/screenshots/verified.png") FileUtils.cp(fixture_image_path_from("a"), "#{repo}/screenshots/changed.png") + FileUtils.cp(fixture_image_path_from("a"), "#{repo}/screenshots/rerecorded.png") git = ["git", "-C", repo, "-c", "user.email=t@example.com", "-c", "user.name=t"] Open3.capture2e(*git, "init", "-q") Open3.capture2e(*git, "add", "screenshots") diff --git a/test/unit/parallel_report_merge_test.rb b/test/unit/parallel_report_merge_test.rb index 11fa2f9a..e4491f5d 100644 --- a/test/unit/parallel_report_merge_test.rb +++ b/test/unit/parallel_report_merge_test.rb @@ -47,6 +47,10 @@ class ParallelReportMergeTest < ActiveSupport::TestCase fork_worker do SnapDiff::Reporting.notify([build_failing_assertion("worker_b"), build_passing_assertion("worker_b_ok")]) SnapDiff::Reporting.record_missing_baseline("worker_b_new") + # Rides the same fragment as the counts above (#274 + #269). Both + # tallies are written by the same worker and merged by the same + # parent, and neither may cost the other. + SnapDiff::Reporting.record_rerecorded_baseline("worker_b_accepted") end # The bug, restated as an assertion: the parent holds nothing of its own. @@ -59,8 +63,9 @@ class ParallelReportMergeTest < ActiveSupport::TestCase assert_equal 2, @reporter.failed # The counts line must total the MERGED runs -- one worker's numbers # would be wrong here. - assert_equal "[snap_diff] 3 verified, 2 changed, 1 new (not verified).", + assert_equal "[snap_diff] 3 verified, 2 changed, 1 new (not verified). 1 re-recorded (not verified).", SnapDiff::Reporting.counts_summary + assert_includes SnapDiff::Reporting.rerecorded_baselines_summary, "worker_b_accepted" # Symbol keys, like an entry recorded in this process: `failures` is # public, and an array whose shape depends on which process filled it @@ -98,6 +103,23 @@ class ParallelReportMergeTest < ActiveSupport::TestCase assert_equal 0, SnapDiff::Reporting.missing_baselines_count end + # The fragments directory is keyed by pid under the system temp dir, so a + # recycled pid can hand this merge a fragment written by an older version + # of the gem -- one with none of the keys added since. Every key but + # "missing_baselines" is read with a default for exactly this. + test "a fragment written before the newer tallies existed still merges" do + fork_worker { SnapDiff::Reporting.notify([build_failing_assertion("current")]) } + + dir = SnapDiff::Reporting.parallel_fragments_dir + File.write(File.join(dir, "99998.json"), JSON.generate({"missing_baselines" => ["ancient"], "reporters" => []})) + + SnapDiff::Reporting.merge_parallel_fragments! + + assert_equal 1, SnapDiff::Reporting.verified, "the current worker's counts survived the old fragment" + assert_equal 1, SnapDiff::Reporting.missing_baselines_count + assert_includes SnapDiff::Reporting.missing_baselines_summary, "ancient" + end + # Serial and `parallelize(with: :threads)` both record in the process that # finalizes; they never write a fragment, and the merge must leave them # exactly as they were.