feat: the summary line counts what was verified, not just what was compared - #261
Conversation
…mpared
Three separate causes produced a green suite that compared nothing this
week: no committed baseline, `rake test` running zero system tests in a
Rails app, and an inherited GIT_DIR redirecting every baseline lookup. The
last two are invisible to any per-assertion rule -- no assertion runs, so
nothing can warn. The only signal that catches all three is a count of what
was actually verified.
`[snap_diff] N screenshots compared, no failures.` counted only what it
compared, and was therefore silent about exactly the screenshots it did
not -- and printed nothing at all when it compared nothing. Now:
[snap_diff] 14 verified, 0 changed, 1 new (not verified). Report: /abs/path.html
verified -- a committed baseline existed and was compared
changed -- of those, the ones that differed
new -- captured but NOT compared: neither a pass nor a failure
Printed on every run, passing or failing, the way Minitest prints its seed:
the information is worthless if it only appears once you already know you
need it. When nothing was verified the line says so in as many words --
"0 verified" is the entire tell for the two invisible causes.
Observability only: no exit code and no pass/fail behaviour changes, and
every count comes from what the run actually did. The absolute report path
stays exactly where it was.
The guard is a real run of a user's test file in a fresh process against
real committed git baselines (test/fixtures/summary_line_case.rb), because
an in-process assertion on `reporter.summary` stays green through a
regression that stops the line from being printed at all.
|
Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
Reviewer's GuideUpdate the HTML reporter summary line to always report what was actually verified (verified/changed/new counts) and add integration coverage that asserts on the real end-of-run output from a separate process with committed baselines, plus minor docs and unit test updates to match the new behavior. Sequence diagram for the HTML reporter summary outputsequenceDiagram
participant TestRun
participant HTMLReporter
participant Reporting
participant Output
TestRun->>HTMLReporter: summary()
HTMLReporter->>Reporting: missing_baselines_count()
Reporting-->>HTMLReporter: new count
HTMLReporter->>HTMLReporter: calculate total and failures
alt no screenshots verified
HTMLReporter-->>Output: print 0 verified, 0 changed, 0 new + NOTHING WAS VERIFIED
else screenshots verified and no failures
HTMLReporter-->>Output: print verified, changed, and new counts
else changed screenshots exist
HTMLReporter->>HTMLReporter: output_path()
HTMLReporter-->>Output: print counts and report path
end
Flow diagram for screenshot verification outcomesflowchart TD
A[Capture screenshot] --> B{Committed baseline exists?}
B -->|Yes| C[Compare screenshot]
C --> D{Different?}
D -->|No| E[Increment verified]
D -->|Yes| F[Increment verified and changed]
B -->|No| G[Record missing baseline]
G --> H[Increment new not verified]
E --> I[Print summary on every run]
F --> I
H --> I
I --> J{Verified count is zero?}
J -->|Yes| K[Append NOTHING WAS VERIFIED]
J -->|No| L[Print counts and report path when failures exist]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The v2.0.0 section was written before #250, #253, #254, #255, #256, #261, #263, #264, #266 and #267 landed, and three of its claims had gone false: - "Known limitations: fork-based parallel tests produce no HTML report ... Fixed in 2.1" -- fixed in 2.0 by #266. Reproduced both sides here: 1.15.1 + `parallelize(workers: 2, threshold: 0)` writes NO report and prints no summary line; master writes one merged report and `4 verified, 4 changed, 0 new`. - "a suite whose only contact with the v1 API is `require \"capybara_screenshot_diff/minitest\"` + `include ...Assertions` still prints nothing" -- #263 made the require doors warn. That exact setup now prints the migration notice; verified in a scratch project. - "Two removals 2.0 cannot warn about ... `driver:` as a setting" -- #263 made both the setting writer and the per-screenshot key warn. Verified: `Capybara::Screenshot::Diff.driver = :vips` prints the removal line with a call site. And the silent-by-design constant list repeated the shape of the beta2 `defined?` mistake: it listed "Os, Region" inside a run of `Capybara::Screenshot::Diff::` names. Probed on master -- `defined?(Capybara::Screenshot::Diff::Os)` and `defined?(Capybara::Screenshot::Diff::Region)` are both nil. The real names are `Capybara::Screenshot::Os` and the top-level `Region`, neither of which existed under `::Diff` in 1.15.1 either. Fully qualified now, and `::Comparison` added to match docs/UPGRADING.md. New material, every claim checked against the code or a live run: - a "why upgrade" section for the four green-suite-testing-nothing bugs (#255, #256, #254, #266), plus the unfollowable CI message (#267) and the fail_if_new precedence change - before/after transcripts of the failure message (#264), taken from the same page rendered on 1.15.1 and on master - the summary line (#261), with the fact that it comes from the HTML reporter and needs its one-line require -- an omission that would have read as a missing feature - the #250 / #253 perf table, attributed to its harness, with columns labelled before/after rather than 1.x/2.0 - the libvips fix is stated as guarded on libvips 8.15+, so a reader on an older libvips knows the bug is still theirs Install snippets stay pinned to 2.0.0.beta3 on purpose: `~> 2.0` resolves to nothing on rubygems today. docs/RELEASE_PREP.md already carries a precise step to swap all five (its grep finds exactly those five), and gains one line so the record-modes placeholder in the entry cannot ship unfilled. `rake test:unit` 651 runs / 0 failures, `standardrb lib test` clean.
Why
Three separate causes produced a green suite that compared nothing this week:
rake testrunning zero system tests in a Rails app,GIT_DIRredirecting every baseline lookup.(2) and (3) are invisible to any per-assertion rule — no assertion runs, so nothing can warn. The only signal that catches all three is a count of what was actually verified.
[snap_diff] N screenshots compared, no failures.counted only what it compared, so it was silent about exactly the screenshots it did not — and silent altogether when it compared nothing.What
Printed on every run, passing or failing — the way Minitest prints
Run options: --seed N: the information is worthless if it only appears once you already know you need it.The zero case
Same line shape every run (so
0 verifiedis always the first number the eye lands on), with an explicit sentence appended rather than a separate line — a distinct extra line is easy to lose in a failure dump, and re-wording the whole line would break the shape people learn to scan:Evidence — real runs, real git baselines
A throwaway git repo with committed baselines, a user's minitest file, one subprocess per scenario:
[snap_diff] 1 verified, 0 changed, 0 new (not verified).[snap_diff] 2 verified, 1 changed, 0 new (not verified). Report: /private/var/.../screenshots/snap_diff_report.html[snap_diff] 1 verified, 0 changed, 1 new (not verified).[snap_diff] 0 verified, 0 changed, 0 new (not verified). NOTHING WAS VERIFIED -- no screenshot was compared to a committed baseline.Exit codes are unchanged in every case — this PR touches nothing but a string. Whether a new screenshot fails is #259's decision and is not smuggled in here.
Guards
test/integration/summary_line_test.rbrunstest/fixtures/summary_line_case.rb— the user's require, the user'sassert_matches_screenshot, real Minitest, real committed baselines — and asserts on what the finished process printed. An in-process assertion onreporter.summarywould stay green through a regression that stops the line from being printed at all, which is how this class of bug got through before.Six mutations, each reverted with a targeted edit and re-verified green:
NOTHING WAS VERIFIEDclausereturn if total.zero?(silent again)rake test:unit(611 runs),rake test:canonical(506 runs),standardrb lib testall green.Note for review
Four existing
#summaryexpectations intest/unit/reporters/html_reporter_test.rbpinned the old wording ("2 screenshots","1 failure","no failures",assert_nilat zero). They are the specification of the line this PR was asked to replace, so they were rewritten rather than preserved — the only edits to existing test expectations in this branch. Nothing else changed behaviour.🤖 Generated with Claude Code
https://claude.ai/code/session_014BQJX6eWzBj2UTm5zQsjEs
Summary by Sourcery
Report what snapshot runs actually verified, changed, and left unverified, including an unmistakable warning when no screenshot was compared to a committed baseline.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: