Settle the page before the shot, and render a mismatch again - #19
Merged
Merged
Conversation
Two things made an unchanged pair compare as different from one run to the next. `screenshot` waited for `readyState` only. That does not cover a web font: the load event fires while the face is still arriving, and the text is laid out again once it lands, so the shot could catch the page mid-render. It now waits for `document.fonts.ready` and then for two frames, which say a paint has happened rather than merely been asked for. The `poppler` sleep the TODO apologised for is what this replaces; `settling_time` is there for a caller that still wants one. `compare_html` reported the first render it disliked. It now renders a mismatch again before reporting it, once by default. A real difference is in the markup and comes back every time, so nothing is hidden, and the matching files - almost all of them - are rendered once as before. `--retries 0` asks for the old behaviour. Reported downstream: opendocument-app/OpenDocument.core ran this over 420 pdf views on a macos-26 runner and the job failed on five pull requests in a row, naming a different set of files each time. Three of those five changed only spreadsheet or document javascript, which cannot alter a pdf view, so the difference was in the rendering rather than in the files. I could not reproduce the runner's non-determinism here - eight shots of each named page are byte-identical in both firefox and chrome on an idle machine - so the retry is what carries the fix, and the settle is what removes the reason to need it.
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
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.
An unchanged pair could compare as different from one run to the next. Two things allow that.
screenshotwaited forreadyStateonlyThat does not cover a web font. The load event fires while the face is still arriving, and the text is laid out again once it lands, so the shot could catch the page mid-render. The
popplersleep theTODOapologised for was the symptom:settle()now waits fordocument.fonts.readyand then for tworequestAnimationFrameticks, which say a paint has happened rather than merely been asked for.settling_timestays available for a caller that still wants a sleep, and a driver without async scripts falls back to it.compare_htmlreported the first render it dislikedOne pixel anywhere fails the file, on one render, with no second look. It now renders a mismatch again before reporting it — once by default,
--retries Nto change it,--retries 0for the old behaviour.This does not weaken the comparison. A real difference is in the markup and comes back every time; only one that does not reproduce is dropped. The matching files, which are almost all of them, are still rendered once, so the cost lands only where a mismatch was already going to be reported.
Why
Reported downstream: OpenDocument.core runs this over 420 pdf views, and the
test (macos-26)job failed on five pull requests in a row, naming a different set of files each time:style-various-1page0, page1978-3-030-65771-0.pdf-singlestyle-various-1document, page0908 and 909 changed only spreadsheet and document javascript. Neither can alter a pdf view by a single pixel. So the difference was in the rendering, not in the files — and a stable cause, a missing font say, would have named the same files every run.
Checked
pytest: 8 existing tests still pass, plus 5 new ones overcompare_html's handling of a mismatch, driven through a stub renderer because what is under test is the control flow rather than how a page paints. Againstmain'scompare_htmlfour of the five fail.What I could not do: reproduce the runner's non-determinism here. Eight shots of each named page are byte-identical in both firefox and chrome on an idle machine. So the retry is what carries the fix, and the settle is what removes the reason to need it.