fix: give three tests distinct screenshot names, so a failure keeps its evidence - #290
Merged
Merged
Conversation
…ts evidence `index-without-img-cropped` was asserted by THREE tests, not the two #284 reported: the `color_distance_limit` case (:117), `crop accepts css selector` (:129), and the `skip_area: "input"` case (:200). A differing screenshot is written to its baseline path by design, so when one of them fails the next re-captures the same name, matches, and leaves the file clean. The failure's own evidence is gone before artifacts upload. Measured on the real failing run 32740082766, artifact `screenshots-selenium_headless-chunky_png-diffs`: the job reported a **26.06% difference** and its uploaded `.base.png`, `.png` and the committed baseline were all `5fccb56f4a47d41b3a8c8cdf38531da6`. Anyone opening that artifact to debug the flake finds two identical PNGs and concludes it was spurious. That is why #285 has never been diagnosed. All three visit `/index-without-img.html` and crop `"form"`, so they share one rendering -- which is exactly why the collision went unnoticed for so long. Each renamed test therefore gets a copy of that same baseline in all six os/driver directories; nothing is re-recorded and no comparison changes. The `color_distance_limit` test keeps the original name so its baseline history stays continuous. NOT verified locally, and deliberately not claimed to be: this file reports `0 verified -- NOTHING WAS VERIFIED` on this machine, and does so on unmodified master too, so a local green here is not evidence. `Test Drivers` is where these baselines actually compare; this PR carries `full-ci` for that reason.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuidePrevents screenshot failure evidence from being overwritten by giving the three otherwise-identical assertions distinct baseline names, while preserving the original baseline history and duplicating the existing image across all supported driver directories. Verification is intentionally delegated to full CI because local runs verify no screenshots. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe integration tests now use distinct screenshot fixture names for selector-based cropping and combined cropping with skip-area masking. ChangesScreenshot fixture names
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The tests now use distinct screenshot names while preserving their existing baselines, so failures can retain separate diagnostic evidence; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes address issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 #284. Unblocks #285.
index-without-img-croppedwas asserted by three tests, not the two #284 reported —:117(color_distance_limit),:129(crop accepts css selector), and:200(skip_area: "input").A differing screenshot is written to its baseline path by design. So when one fails, the next re-captures the same name, matches, and leaves the file clean — the failure's evidence is destroyed before artifacts upload.
Measured on a real failing run
Run
32740082766, artifactscreenshots-selenium_headless-chunky_png-diffs. The job reported a 26.06% difference; its uploaded files and the committed baseline are one and the same:Anyone opening that artifact to debug #285 finds two identical PNGs and concludes the failure was spurious.
The change
All three visit
/index-without-img.htmland crop"form", so they share one rendering — which is why this went unnoticed. Each renamed test gets a copy of that same baseline in all six os/driver directories. Nothing is re-recorded; no comparison changes.The
color_distance_limittest keeps the original name so its baseline history stays continuous.On verification — read this before trusting a green
This is not verified locally and I am not claiming it is. Running this file on my machine reports:
…and it reports the same on unmodified master, so a local green here would be the very bug class 2.0 exists to kill.
Test Driversis where these baselines actually compare, so this PR carriesfull-ci.standardrb clean.
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit