feat: an optional readiness block, and a run-level tally of selectors that never matched (#277) - #279
Merged
Merged
Conversation
`assert_matches_screenshot` and `capture_screenshot` return at the
`active?` guard before doing anything else. So readiness scaffolding
written on the line above -- `preload_all_images`, `document.fonts.ready`,
waiting on a widget -- still runs when screenshots are DISABLED, and in a
real consumer that is three browser round-trips (scroll to bottom, an
`assert_text` with its own wait, scroll back) per screenshot that is never
taken. Turning visual tests off was free except for the scaffolding, and
the scaffolding is the expensive part.
Both methods now take an optional block, run after the guard and before
the capture. `screenshot` and `assert_no_screenshot_changes` forward it --
a delegator that swallowed it would hand the user a block that silently
never runs.
Deliberately NOT a hook: no config-level `before_capture`, no after-hooks,
no block on Comparison. It runs once per assertion, not once per stability
attempt (a per-attempt block is a different feature and a separate
decision), and an error raised inside it is the user's own and propagates
unchanged.
RSpec's `match_screenshot` matcher does not take one: `expect(page).to
match_screenshot('x') { ... }` binds the block by Ruby's `{}`/`do...end`
precedence rather than by intent. RSpec and Cucumber users call
`assert_matches_screenshot` directly, where the block is unambiguous.
Also drops the docs' claim that an unmatched `skip_area` selector waits for
the element -- untrue since #272 -- and says what replaced it: the mask
covers what exists at assertion time, and late-loading content belongs in
the block.
) Removing the implicit wait (#272) took a measured 44% off a real suite, but that wait was also, accidentally, giving late-loading elements time to appear. A `skip_area` selector that matches nothing now yields an EMPTY mask, silently: nothing excluded, the unstable region compared, the test flakes -- and the only tell is a flake weeks later. #275 deliberately declined a per-screenshot warning, and was right to: the gem cannot tell a typo from a legitimately image-less page, so the legitimate case would fire on every screenshot until people stopped reading it. A RUN-level tally has no such problem. A selector that matched somewhere is doing its job and is never mentioned; one that matched nowhere in the entire run is a typo or a stale selector with high probability. Fed from BrowserHelpers.bounds_for_css, the one seam that still knows which selector produced which regions -- AreaCalculator sees only the flattened list -- so a selector that was configured but never reached can never be named. Two sets rather than a counter, because under fork-parallel (#266) the hit and the miss for one selector arrive from different processes and only the parent that merged every fragment can answer "did this match ANYWHERE"; both halves ride the existing fragment alongside verified/changed/new, `fetch`ed with defaults like every key added since. Silent when the set is empty, on purpose. A line that prints on every run is a line users learn to skip, which is how this one would stop working.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reachedNext included review available in 19 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 (8)
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 |
Contributor
Reviewer's GuideThe PR adds an opt-in readiness block to screenshot APIs so expensive page-settling work is disabled alongside inactive screenshots, and introduces fork-safe run-level reporting for selectors that matched nowhere while removing outdated documentation about implicit selector waits. Sequence diagram for the optional screenshot readiness blocksequenceDiagram
participant Test
participant DSL
participant ScreenshotMatcher
Test->>DSL: assert_matches_screenshot(name, options)
alt screenshots inactive
DSL-->>Test: false
else screenshots active
DSL->>Test: yield readiness block
Test-->>DSL: readiness complete
DSL->>ScreenshotMatcher: capture or compare screenshot
ScreenshotMatcher-->>DSL: result
DSL-->>Test: result
end
Sequence diagram for fork-safe unmatched selector reportingsequenceDiagram
participant BrowserHelpers
participant Reporting
participant Worker
participant Parent
BrowserHelpers->>Reporting: record_selector_use(selector, matched: boolean)
Worker->>Reporting: dump_parallel_fragment()
Reporting-->>Parent: matched_selectors and unmatched_selectors
Parent->>Reporting: merge_parallel_fragments!
Reporting->>Reporting: never_matched_selectors_summary()
Reporting-->>Parent: print selectors unmatched across the run
Flow diagram for run-level selector matching summaryflowchart TD
A[Resolve each CSS selector] --> B{Did it match any regions?}
B -->|Yes| C[Add selector to matched_selectors]
B -->|No| D[Add selector to unmatched_selectors]
C --> E[Merge worker fragments]
D --> E
E --> F[Subtract matched_selectors from unmatched_selectors]
F --> G{Any selectors left?}
G -->|No| H[Print nothing]
G -->|Yes| I[Print never-matched selector summary]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Aug 24, 2026
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
#271) Stabilisation failures printed a bare list of attempt paths. Diagnosing one meant opening N PNGs and eyeballing them -- so `sleep 2` won, and suites got SLOWER as a consequence of diagnosis being hard. A maintainer reported a 10-minute suite dominated by stabilisation waiting, with sleeps adopted deliberately "to avoid debugging as much as possible". The information was already here and thrown away: AttemptsReporter compares every consecutive pair of attempts, and that comparison knows the region that changed. Print it, with the escape hatch: Could not get stable screenshot for 'index-with-ticker' within 1.2s (5 attempts). The page kept changing in 1 area, over 4 attempt pairs: [67,50,213,68] (left,top,right,bottom edges) -- 0.55% of the 800x600 image, changed in 4 of 4 pairs Always the same area, in every pair: that is an animation, clock, carousel or live counter. Exclude it and the page is stable without waiting: assert_matches_screenshot "index-with-ticker", skip_area: [67,50,213,68] <attempt paths> Animation vs churn is decided by count, not by shape: regions are clustered by overlap, and a cluster present in EVERY attempt pair is animating -- skip_area fixes it. Anything less is the page still rendering, where masking would hide real content, so the message says so and suggests nothing to mask. The suggested coordinates are the ones just measured. Guarded by following the advice on a real browser and a really unstable page (test/fixtures/app/ index-with-ticker.html): the failing run's own suggestion, pasted back in, makes the page stable. Fabricating the coordinate reds that test -- which is the check this gem lacked when it shipped RECORD_SCREENSHOTS=1 in its own error message for years while nothing read it. Success path: the run-level summary now reports the worst stabilisation it saw. A user who set `stability_time_limit: 2` had no way to learn their pages settle on the first retry, and without evidence tuning it down is guesswork. Run-level rather than per-assertion (per-test noise is the last thing a slow suite needs) and silent when nothing waited -- the same rule as the never-matched-selector line. It rides the fork-parallel fragment, since a run-level line that vanishes under Rails' default parallelize is #269 again; counts add, worst cases max. Pairs with #272 (masking is instant) and #279 (dead selectors are surfaced): "here is the region, mask it" is finally a complete workflow.
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
#279 shipped the optional block on assert_matches_screenshot / capture_screenshot, but documented only that it exists. Nobody finds a mechanism without the use-cases, and the use-cases here are exactly the workarounds real users already hand-roll. Two recipes, both in docs/configuration.md next to the block and cross-linked from the skip_area section: - Webfonts. A font swapping mid-capture reflows text bimodally -- the "only fails on CI" flake people paper over with a skip_area, a loosened tolerance and a retry, all three of which weaken the comparison everywhere. `document.fonts.ready` waits for exactly the swap and returns on the first round trip once fonts are cached. - Lazy images. Scroll, wait for something at the bottom, scroll back -- and note the ORDER: skip_area masks what exists at assertion time, so a selector for content that has not loaded yet produces an empty mask and the unstable region is compared anyway. Plus what does NOT belong in the block, and why there is no built-in font wait: it would be a browser round trip imposed on every screenshot in every suite, and a driver-compatibility surface the gem would own forever, in exchange for one line a user can write.
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
…289) `jruby-10.0/rails80` timed out on master run 32763256451 and took the whole run red. Tests were still printing dots when SIGTERM landed, so the cell was SLOW, not hung -- and the same cell had passed the previous three runs. The 15-minute per-attempt budget was sized from run 32643567648, where a clean JRuby attempt was 545-713s: "~26% headroom over the slowest", as the comment says. That measurement is stale. Re-measured on run 32758898367, the last green one before this bit: rails71 652s rails81 740s rails80 870s rails72 881s <- against a 900s cap The suite grew from 646 to 757 runs in between (#274, #277, #278, #279, #283), and the headroom went with it: 881/900 is 2%. The cells have been passing by seconds, which is why this looked stable for three runs and then was not. 20 minutes restores the ~26% margin this was originally sized for, and the job cap follows to keep the arithmetic true: 1 + 20 + 20 = 41. Both numbers move together on purpose -- a per-attempt timeout that does not fit the cap kills the last attempt mid-run and reports `cancelled`, which reads as an absence rather than a failure. MRI is unchanged: 128s against 3 minutes. Costs nothing on a green run; it is a ceiling, not a sleep.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
#271) Stabilisation failures printed a bare list of attempt paths. Diagnosing one meant opening N PNGs and eyeballing them -- so `sleep 2` won, and suites got SLOWER as a consequence of diagnosis being hard. A maintainer reported a 10-minute suite dominated by stabilisation waiting, with sleeps adopted deliberately "to avoid debugging as much as possible". The information was already here and thrown away: AttemptsReporter compares every consecutive pair of attempts, and that comparison knows the region that changed. Print it, with the escape hatch: Could not get stable screenshot for 'index-with-ticker' within 1.2s (5 attempts). The page kept changing in 1 area, over 4 attempt pairs: [67,50,213,68] (left,top,right,bottom edges) -- 0.55% of the 800x600 image, changed in 4 of 4 pairs Always the same area, in every pair: that is an animation, clock, carousel or live counter. Exclude it and the page is stable without waiting: assert_matches_screenshot "index-with-ticker", skip_area: [67,50,213,68] <attempt paths> Animation vs churn is decided by count, not by shape: regions are clustered by overlap, and a cluster present in EVERY attempt pair is animating -- skip_area fixes it. Anything less is the page still rendering, where masking would hide real content, so the message says so and suggests nothing to mask. The suggested coordinates are the ones just measured. Guarded by following the advice on a real browser and a really unstable page (test/fixtures/app/ index-with-ticker.html): the failing run's own suggestion, pasted back in, makes the page stable. Fabricating the coordinate reds that test -- which is the check this gem lacked when it shipped RECORD_SCREENSHOTS=1 in its own error message for years while nothing read it. Success path: the run-level summary now reports the worst stabilisation it saw. A user who set `stability_time_limit: 2` had no way to learn their pages settle on the first retry, and without evidence tuning it down is guesswork. Run-level rather than per-assertion (per-test noise is the last thing a slow suite needs) and silent when nothing waited -- the same rule as the never-matched-selector line. It rides the fork-parallel fragment, since a run-level line that vanishes under Rails' default parallelize is #269 again; counts add, worst cases max. Pairs with #272 (masking is instant) and #279 (dead selectors are surfaced): "here is the region, mask it" is finally a complete workflow.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
#279 shipped the optional block on assert_matches_screenshot / capture_screenshot, but documented only that it exists. Nobody finds a mechanism without the use-cases, and the use-cases here are exactly the workarounds real users already hand-roll. Two recipes, both in docs/configuration.md next to the block and cross-linked from the skip_area section: - Webfonts. A font swapping mid-capture reflows text bimodally -- the "only fails on CI" flake people paper over with a skip_area, a loosened tolerance and a retry, all three of which weaken the comparison everywhere. `document.fonts.ready` waits for exactly the swap and returns on the first round trip once fonts are cached. - Lazy images. Scroll, wait for something at the bottom, scroll back -- and note the ORDER: skip_area masks what exists at assertion time, so a selector for content that has not loaded yet produces an empty mask and the unstable region is compared anyway. Plus what does NOT belong in the block, and why there is no built-in font wait: it would be a browser round trip imposed on every screenshot in every suite, and a driver-compatibility surface the gem would own forever, in exchange for one line a user can write.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
#271) Stabilisation failures printed a bare list of attempt paths. Diagnosing one meant opening N PNGs and eyeballing them -- so `sleep 2` won, and suites got SLOWER as a consequence of diagnosis being hard. A maintainer reported a 10-minute suite dominated by stabilisation waiting, with sleeps adopted deliberately "to avoid debugging as much as possible". The information was already here and thrown away: AttemptsReporter compares every consecutive pair of attempts, and that comparison knows the region that changed. Print it, with the escape hatch: Could not get stable screenshot for 'index-with-ticker' within 1.2s (5 attempts). The page kept changing in 1 area, over 4 attempt pairs: [67,50,213,68] (left,top,right,bottom edges) -- 0.55% of the 800x600 image, changed in 4 of 4 pairs Always the same area, in every pair: that is an animation, clock, carousel or live counter. Exclude it and the page is stable without waiting: assert_matches_screenshot "index-with-ticker", skip_area: [67,50,213,68] <attempt paths> Animation vs churn is decided by count, not by shape: regions are clustered by overlap, and a cluster present in EVERY attempt pair is animating -- skip_area fixes it. Anything less is the page still rendering, where masking would hide real content, so the message says so and suggests nothing to mask. The suggested coordinates are the ones just measured. Guarded by following the advice on a real browser and a really unstable page (test/fixtures/app/ index-with-ticker.html): the failing run's own suggestion, pasted back in, makes the page stable. Fabricating the coordinate reds that test -- which is the check this gem lacked when it shipped RECORD_SCREENSHOTS=1 in its own error message for years while nothing read it. Success path: the run-level summary now reports the worst stabilisation it saw. A user who set `stability_time_limit: 2` had no way to learn their pages settle on the first retry, and without evidence tuning it down is guesswork. Run-level rather than per-assertion (per-test noise is the last thing a slow suite needs) and silent when nothing waited -- the same rule as the never-matched-selector line. It rides the fork-parallel fragment, since a run-level line that vanishes under Rails' default parallelize is #269 again; counts add, worst cases max. Pairs with #272 (masking is instant) and #279 (dead selectors are surfaced): "here is the region, mask it" is finally a complete workflow.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
#279 shipped the optional block on assert_matches_screenshot / capture_screenshot, but documented only that it exists. Nobody finds a mechanism without the use-cases, and the use-cases here are exactly the workarounds real users already hand-roll. Two recipes, both in docs/configuration.md next to the block and cross-linked from the skip_area section: - Webfonts. A font swapping mid-capture reflows text bimodally -- the "only fails on CI" flake people paper over with a skip_area, a loosened tolerance and a retry, all three of which weaken the comparison everywhere. `document.fonts.ready` waits for exactly the swap and returns on the first round trip once fonts are cached. - Lazy images. Scroll, wait for something at the bottom, scroll back -- and note the ORDER: skip_area masks what exists at assertion time, so a selector for content that has not loaded yet produces an empty mask and the unstable region is compared anyway. Plus what does NOT belong in the block, and why there is no built-in font wait: it would be a browser round trip imposed on every screenshot in every suite, and a driver-compatibility surface the gem would own forever, in exchange for one line a user can write.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
…ss recipes (#271, #273) (#280) * feat: name the region that would not settle, and what the waiting cost (#271) Stabilisation failures printed a bare list of attempt paths. Diagnosing one meant opening N PNGs and eyeballing them -- so `sleep 2` won, and suites got SLOWER as a consequence of diagnosis being hard. A maintainer reported a 10-minute suite dominated by stabilisation waiting, with sleeps adopted deliberately "to avoid debugging as much as possible". The information was already here and thrown away: AttemptsReporter compares every consecutive pair of attempts, and that comparison knows the region that changed. Print it, with the escape hatch: Could not get stable screenshot for 'index-with-ticker' within 1.2s (5 attempts). The page kept changing in 1 area, over 4 attempt pairs: [67,50,213,68] (left,top,right,bottom edges) -- 0.55% of the 800x600 image, changed in 4 of 4 pairs Always the same area, in every pair: that is an animation, clock, carousel or live counter. Exclude it and the page is stable without waiting: assert_matches_screenshot "index-with-ticker", skip_area: [67,50,213,68] <attempt paths> Animation vs churn is decided by count, not by shape: regions are clustered by overlap, and a cluster present in EVERY attempt pair is animating -- skip_area fixes it. Anything less is the page still rendering, where masking would hide real content, so the message says so and suggests nothing to mask. The suggested coordinates are the ones just measured. Guarded by following the advice on a real browser and a really unstable page (test/fixtures/app/ index-with-ticker.html): the failing run's own suggestion, pasted back in, makes the page stable. Fabricating the coordinate reds that test -- which is the check this gem lacked when it shipped RECORD_SCREENSHOTS=1 in its own error message for years while nothing read it. Success path: the run-level summary now reports the worst stabilisation it saw. A user who set `stability_time_limit: 2` had no way to learn their pages settle on the first retry, and without evidence tuning it down is guesswork. Run-level rather than per-assertion (per-test noise is the last thing a slow suite needs) and silent when nothing waited -- the same rule as the never-matched-selector line. It rides the fork-parallel fragment, since a run-level line that vanishes under Rails' default parallelize is #269 again; counts add, worst cases max. Pairs with #272 (masking is instant) and #279 (dead selectors are surfaced): "here is the region, mask it" is finally a complete workflow. * docs: the readiness-block recipes -- webfonts and lazy images (#273) #279 shipped the optional block on assert_matches_screenshot / capture_screenshot, but documented only that it exists. Nobody finds a mechanism without the use-cases, and the use-cases here are exactly the workarounds real users already hand-roll. Two recipes, both in docs/configuration.md next to the block and cross-linked from the skip_area section: - Webfonts. A font swapping mid-capture reflows text bimodally -- the "only fails on CI" flake people paper over with a skip_area, a loosened tolerance and a retry, all three of which weaken the comparison everywhere. `document.fonts.ready` waits for exactly the swap and returns on the first round trip once fonts are cached. - Lazy images. Scroll, wait for something at the bottom, scroll back -- and note the ORDER: skip_area masks what exists at assertion time, so a selector for content that has not loaded yet produces an empty mask and the unstable region is compared anyway. Plus what does NOT belong in the block, and why there is no built-in font wait: it would be a browser round trip imposed on every screenshot in every suite, and a driver-compatibility surface the gem would own forever, in exchange for one line a user can write. * fix: the suggested skip_area must be integers that COVER the region CI produced [62.0,50.0,218.0,68.0] where macOS produced integers, and the message's own regex (`skip_area: (\[[\d,]+\])`) silently failed to match -- so the integration test that pastes the suggestion back in could not find it. Two defects, not one. Float coordinates are not pasteable into a test file. And the naive fix, truncation, would shave the right and bottom edges and leave the moving pixels exposed -- a mask that under-covers is worse than no suggestion, because it looks like it worked. Round OUTWARD: floor the near edges, ceil the far ones. Guarded, and the guard reds under truncation. * fix: merge every transitively overlapping area, and give the fixture a doctype Both from CodeRabbit review on #280. The clustering merged an incoming region into the FIRST area it touched. A chain -- A touches B, B touches C, A does not touch C -- therefore left two areas instead of one. The lane's own comment argued that under-counting is safe because it cannot invent a mask, and that is true, but it misses the cost: each fragment is then seen in fewer attempt pairs than the whole, so a single animation is classified as churn and NO mask is offered. It withholds the one suggestion this message exists to make. Now merges every touching area and sums their pair counts. Guarded with the bridge case; the guard reds under first-overlap-wins. The ticker fixture had no doctype, so browsers rendered it in quirks mode -- different box model, in a fixture whose entire purpose is pixel comparison. * fix: a one-pixel change suggested a skip_area that masks nothing CI (3.4/rails81) failed the integration test that pastes the suggestion back in, because the suggestion was degenerate: The page kept changing in 1 area, over 2 attempt pairs: [216,52,216,65] -- <0.01% of the 800x600 image, changed in 2 of 2 pairs Exclude it and the page is stable without waiting: assert_matches_screenshot "index-with-ticker", skip_area: [216,52,216,65] left == right. Region carries WIDTH and `from_edge_coordinates` derives it as `right - left`, so that mask is 0 px wide: it masks nothing, the page still does not settle, and the user is told to paste a fix that cannot work. A ticker digit or a caret is one column wide, which is exactly when the two edges collapse -- so the message was worst precisely where it was most needed. Timing-dependent, which is why it passed locally and on 15 other cells. This is the degenerate case of the invariant the outward rounding already states -- "a mask that under-covers is worse than no suggestion, because it looks like it worked" -- taken to the limit where it covers nothing at all. floor/ceil cannot reach it: the edges are already integral. Floor the near edges as before, then require at least one pixel of extent on each axis. Reproduced first as a deterministic unit test (the CI failure needs a real browser and the right millisecond); mutation-checked by reverting to plain ceil, which reds it. standardrb clean, `rake test` 771 runs / 2289 assertions / 0 failures. * fix: pin the ticker's extent, and document that a suggested mask is a sample The round-trip test -- paste the message's own suggestion back in, page must then be stable -- failed twice on CI, on two different cells (runs 32750597989 and 32752142873), with well-formed regions both times: [216,52,216,65] 3.4/rails81 (zero-width; fixed in 606a0b9) [62,50,219,67] 4.0/rails71 157x17, and the masked re-run STILL failed The second is not a bug in the measurement. `index-with-ticker.html` re-randomises all ten characters every 30ms inside a `text-align: center` box, and in a PROPORTIONAL font ten random glyphs render to a different WIDTH each tick. The suggested box is the union of what changed across the attempts that ran -- a sample of a moving target -- so a later frame can render outside it. Two changes, because there are two separate facts here. **The fixture** pins the font to monospace. The pixels still change completely every tick, which is the property under test; only the extent stops moving, which is not. That makes the round trip deterministic instead of a coin flip on how many attempts happened to sample. **The docs** state the limitation rather than hide it, because it is real for users too: for an animation whose SIZE varies frame to frame the first suggestion can under-cover, the failure then reports a much smaller region, and pasting the new one converges. For the usual case -- a clock or spinner repainting inside a fixed element -- the extent does not move and the first suggestion is the fix. Deliberately NOT done: padding the suggested box by a fixed margin. The number would be arbitrary, it can still under-cover, and it would widen every correct suggestion to paper over a case the message can simply be honest about. `rake test` and standardrb below. * fix: make the ticker's changing region an ELEMENT, not a run of glyphs The round trip -- fail, parse the message's own suggestion, apply it, page must then be stable -- failed on three separate CI runs with three unrelated regions for the same page: [216,52,216,65] 1 column [62,50,219,67] 157x17 [71,51,71,68] 1 column The diagnosis was right every time. What it was diagnosing would not hold still. Ten random glyphs are a bad thing to measure. Their extent depends on which characters came up, and a capture on a slower machine can land mid-repaint and see a single column of a single character -- hence regions ranging over two orders of magnitude. Pinning the font to monospace (previous commit) fixed the extent but not the mid-repaint sliver, so it was necessary and not sufficient. Now every tick paints the box a RANDOM colour. Two attempts then differ across the whole element at high contrast, a partial repaint is still an unmistakable diff, and the region is the element -- which `position: absolute` with a fixed width and height pins exactly. Random, specifically, and not a black/white toggle: a two-state flip depends on parity, and two attempts ~100ms apart are an unpredictable number of 30ms ticks apart, so they can land on the SAME phase. Measured -- with the toggle the region came back as [69,50,210,66], the text again. Measured after: eight consecutive local runs, all green, every one reporting [40,40,239,79] -- 1.62% of the 800x600 image, changed in 4 of 4 pairs which is the CSS box (left:40 top:40 200x40) to the pixel. Before this change no two runs agreed. This is also the honest shape of what the fixture stands in for: a clock or spinner repainting inside a box that does not move, which is exactly the case where `skip_area` is the right answer. `rake test` and standardrb below.
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.
Closes the two remaining 2.0 items from
.okf/decisions/skip-area-element-readiness.md. Refs #277, #272, #275.1. An optional readiness block (#277a)
assert_matches_screenshotandcapture_screenshotreturn at theactive?guard before doing anything else. So readiness scaffolding written on the line above —preload_all_images,document.fonts.ready, waiting on a widget — still runs when screenshots are disabled. In a real consumer that is three browser round-trips (scroll to bottom, anassert_textwith its own wait, scroll back) for a screenshot that is never taken. Turning visual tests off was free except for the scaffolding, and the scaffolding is the expensive part.active?guard, before the capture, once per assertion — not once per stability attempt. A per-attempt block is a different feature and a separate decision.screenshotandassert_no_screenshot_changes; a delegator that swallowed it would hand the user a block that silently never runs.before_capture, no after-hooks, no block onComparison.RSpec / Cucumber: Cucumber needs no wiring — the DSL is in the World, so step definitions pass a block exactly as a Minitest test does. The RSpec
match_screenshotmatcher deliberately does not take one:expect(page).to match_screenshot("x") { ... }binds the block by Ruby's{}/do...endprecedence rather than by intent, sodo...endwould attach totoand vanish. RSpec users callassert_matches_screenshotdirectly, where the block is unambiguous. Documented rather than half-wired.2. Selectors that never match (#277b)
Removing the implicit wait (#272) also removed the grace period late-loading elements were accidentally getting. An unmatched
skip_areaselector now yields an empty mask, silently, and the only tell is a flake weeks later.#275 declined a per-screenshot warning — the gem cannot tell a typo from a legitimately image-less page, and the legitimate case would fire constantly. A run-level tally can:
BrowserHelpers.bounds_for_css, the last seam that knows which selector produced which regions, so a selector that was configured but never reached cannot be named.verified/changed/new,fetched with defaults like every key added since.Evidence
Guard test first for each item, then every guard mutation-checked — broken, confirmed red, restored with a targeted edit, file verified byte-identical by checksum:
yieldmoved above theactive?guard (both methods)yieldmoved after the capture (both methods)yieldwrapped inrescuebounds_for_cssbounds_for_cssmatched_selectorsdropped from the fork fragmentreturn if names.empty?droppedfinalize!print removedreset_run_totals!clears removedThe disabled-path guard asserts a real side effect — a counter the block increments — not a stub's call count: a stub never called and a stub never wired up look identical. Ordering is asserted on the artifact, not a call count: the block looks for the screenshot file, which cannot be there yet if it really runs first.
rake test:unit,rake test:canonicalandstandardrb lib testgreen, each run both locally and underCI=true.Docs
docs/configuration.mddrops the stale claim that an unmatchedskip_areaselector waits for the element (untrue since #272) and says what replaced it: the mask covers what exists at assertion time, late-loading content belongs in the readiness block, and work placed in the block is skipped when screenshots are off.🤖 Generated with Claude Code
https://claude.ai/code/session_014BQJX6eWzBj2UTm5zQsjEs
Summary by Sourcery
Add conditional screenshot readiness handling and run-level diagnostics for selectors that never match.
New Features:
Enhancements:
Documentation:
Tests: