From cc990154ccfd4a12c82b713c05e6a7e5ad6446bb Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 20 Sep 2026 12:55:00 +0200 Subject: [PATCH] Settle the page before the shot, and render a mismatch again 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. --- src/htmlcmp/common.py | 31 ++++++++++-- src/htmlcmp/compare_output_cli.py | 28 +++++++++-- src/htmlcmp/html_render_diff.py | 45 ++++++++++++++--- tests/test_compare_html_retries.py | 80 ++++++++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 16 deletions(-) create mode 100644 tests/test_compare_html_retries.py diff --git a/src/htmlcmp/common.py b/src/htmlcmp/common.py index 9caa5f2..94b30b1 100644 --- a/src/htmlcmp/common.py +++ b/src/htmlcmp/common.py @@ -6,6 +6,8 @@ from htmlcmp.html_render_diff import get_browser, html_render_diff +logger = logging.getLogger(__name__) + class bcolors: HEADER = "\033[95m" @@ -40,22 +42,41 @@ def compare_json(a: Path, b: Path) -> bool: return json_a == json_b -def compare_html(a: Path, b: Path, browser=None, diff_output: Path = None) -> bool: +def compare_html( + a: Path, b: Path, browser=None, diff_output: Path = None, retries: int = 1 +) -> bool: + """Whether `a` and `b` render to the same pixels. + + A mismatch is rendered again before it is reported, `retries` times. A real + difference is in the markup and comes back every time; one that does not is + the page having been caught mid-render, and a browser gives no promise that + two runs of the same page paint alike at the same instant. The retry costs + nothing on the matching files, which are almost all of them. + """ if not isinstance(a, Path) or not isinstance(b, Path): raise TypeError("Both arguments must be of type Path") if not a.is_file() or not b.is_file(): raise FileNotFoundError("Both arguments must be files") + if not isinstance(retries, int) or retries < 0: + raise ValueError(f"retries must be a non-negative int, got {retries!r}") if browser is None: browser = get_browser("firefox") - diff, (image_a, image_b) = html_render_diff(a, b, browser=browser) - result = diff.getbbox() is None - if diff_output is not None and not result: + + for attempt in range(retries + 1): + diff, (image_a, image_b) = html_render_diff(a, b, browser=browser) + if diff.getbbox() is None: + return True + logger.debug( + "%s and %s differ on attempt %d of %d", a, b, attempt + 1, retries + 1 + ) + + if diff_output is not None: diff_output.mkdir(parents=True, exist_ok=True) image_a.save(diff_output / "a.png") image_b.save(diff_output / "b.png") diff.save(diff_output / "diff.png") - return result + return False def compare_files(a: Path, b: Path, **kwargs) -> bool: diff --git a/src/htmlcmp/compare_output_cli.py b/src/htmlcmp/compare_output_cli.py index 5b20d27..159412f 100644 --- a/src/htmlcmp/compare_output_cli.py +++ b/src/htmlcmp/compare_output_cli.py @@ -34,11 +34,14 @@ class Config: class Task: """A single file comparison between the reference (A) and monitored (B) tree.""" - def __init__(self, rel: Path, a: Path, b: Path, diff_output: Path = None): + def __init__( + self, rel: Path, a: Path, b: Path, diff_output: Path = None, retries: int = 1 + ): self.rel = rel self.a = a self.b = b self.diff_output = diff_output + self.retries = retries class Failure: @@ -54,7 +57,7 @@ def __init__(self, rel: Path, kind: str, reason: str): def collect_tasks( - a: Path, b: Path, root: Path = None, diff_output: Path = None + a: Path, b: Path, root: Path = None, diff_output: Path = None, retries: int = 1 ) -> tuple[list[Task], list[Failure]]: """Walk both trees once and return (comparable tasks, structural failures). @@ -91,6 +94,7 @@ def collect_tasks( a / name, b / name, None if diff_output is None else diff_output / name, + retries=retries, ) ) elif name in left_files: @@ -107,6 +111,7 @@ def collect_tasks( b / name, root=root, diff_output=None if diff_output is None else diff_output / name, + retries=retries, ) tasks.extend(sub_tasks) failures.extend(sub_failures) @@ -126,7 +131,13 @@ def collect_tasks( def run_task(task: Task) -> bool: logger.debug("Comparing %s", task.rel) browser = getattr(Config.thread_local, "browser", None) - return compare_files(task.a, task.b, browser=browser, diff_output=task.diff_output) + return compare_files( + task.a, + task.b, + browser=browser, + diff_output=task.diff_output, + retries=task.retries, + ) def make_executor(max_workers: int, driver: str | None) -> ThreadPoolExecutor: @@ -248,6 +259,7 @@ def run( driver: str | None, max_workers: int, diff_output: Path | None, + retries: int, console: Console, live: bool, github: bool, @@ -256,7 +268,7 @@ def run( f"[bold]Comparing[/bold] {escape(str(a))} [dim]→[/dim] {escape(str(b))}" ) - tasks, failures = collect_tasks(a, b, diff_output=diff_output) + tasks, failures = collect_tasks(a, b, diff_output=diff_output, retries=retries) logger.info( "Collected %d comparable file(s), %d structural difference(s)", len(tasks), @@ -326,6 +338,13 @@ def main(): default=0, help="Increase verbosity (-v, -vv, -vvv)", ) + parser.add_argument( + "--retries", + type=int, + default=1, + help="Re-render a mismatch this many times before reporting it " + "(default: 1; 0 reports the first render)", + ) parser.add_argument("--log-file", type=Path, help="Path to log file") parser.add_argument( "--log-file-verbosity", type=int, help="Log file verbosity level" @@ -351,6 +370,7 @@ def main(): driver=driver, max_workers=args.max_workers, diff_output=args.diff_output, + retries=args.retries, console=console, live=live, github=github, diff --git a/src/htmlcmp/html_render_diff.py b/src/htmlcmp/html_render_diff.py index 13f0f51..cfac7a9 100755 --- a/src/htmlcmp/html_render_diff.py +++ b/src/htmlcmp/html_render_diff.py @@ -9,6 +9,7 @@ from PIL import Image, ImageChops from selenium import webdriver +from selenium.common.exceptions import WebDriverException from selenium.webdriver.common.by import By from selenium.webdriver.support import expected_conditions from selenium.webdriver.support.ui import WebDriverWait @@ -25,7 +26,9 @@ def to_url(path: str | Path) -> str: return path -def screenshot(browser: webdriver.Remote, url: str) -> Image.Image: +def screenshot( + browser: webdriver.Remote, url: str, settling_time: float = 0 +) -> Image.Image: if not isinstance(url, str): raise TypeError(f"Expected str, got {type(url)}") if not isinstance(browser, webdriver.Remote): @@ -35,11 +38,6 @@ def screenshot(browser: webdriver.Remote, url: str) -> Image.Image: target_find_by = By.TAG_NAME target = "body" - loaded_page_settling_time = 0 - - # TODO for pdf2htmlex the second screenshot sometimes fades in from white... not sure why, but a sleep solves it - if "poppler" in url: - loaded_page_settling_time = 0.3 web_driver_wait = WebDriverWait(browser, 10) web_driver_wait.until( @@ -49,12 +47,45 @@ def screenshot(browser: webdriver.Remote, url: str) -> Image.Image: lambda driver: driver.execute_script("return document.readyState") == "complete" ) - time.sleep(loaded_page_settling_time) + settle(browser, settling_time) png = browser.get_screenshot_as_png() return Image.open(io.BytesIO(png)) +#: Waits for the fonts and then for two frames, and answers when both are done. +#: `readyState` 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. Two frames +#: then say a paint has happened rather than merely been asked for. +_SETTLE = """ +const done = arguments[arguments.length - 1]; +const frames = () => + requestAnimationFrame(() => requestAnimationFrame(() => done(true))); +(document.fonts ? document.fonts.ready : Promise.resolve()).then(frames, frames); +""" + + +def settle(browser: webdriver.Remote, settling_time: float = 0) -> None: + """Waits until the page has finished painting what it loaded. + + A screenshot taken before that catches the page mid-render, which is what + makes an otherwise identical pair compare as different from one run to the + next. + """ + if not isinstance(browser, webdriver.Remote): + raise TypeError(f"Expected webdriver.Remote, got {type(browser)}") + + browser.set_script_timeout(10) + try: + browser.execute_async_script(_SETTLE) + except WebDriverException: + # an old driver without async scripts still gets the sleep below + pass + + if settling_time: + time.sleep(settling_time) + + def content_bottom(image: Image.Image) -> int: """Row just below the last pixel that differs from the page background. diff --git a/tests/test_compare_html_retries.py b/tests/test_compare_html_retries.py new file mode 100644 index 0000000..9ce2622 --- /dev/null +++ b/tests/test_compare_html_retries.py @@ -0,0 +1,80 @@ +"""A mismatch is rendered again before it is reported. + +A browser gives no promise that two runs of the same page paint alike at the +same instant, so a single mismatching render does not say the two files differ. +These drive `compare_html` over a stub renderer, because what is under test is +what it does with a mismatch rather than how a page paints. +""" + +from pathlib import Path + +import pytest +from PIL import Image + +from htmlcmp import common + +TEST1 = Path(__file__).parent / "test1.html" + + +def images(same: bool): + """A `html_render_diff` result that says the pair matches, or does not.""" + diff = Image.new("RGB", (4, 4), (0, 0, 0) if same else (255, 0, 0)) + return diff, (Image.new("RGB", (4, 4)), Image.new("RGB", (4, 4))) + + +def renderer(pattern, calls): + """Stands in for `html_render_diff`, answering `pattern` in turn.""" + + def render(a, b, browser=None): + calls.append((a, b)) + return images(pattern[min(len(calls) - 1, len(pattern) - 1)]) + + return render + + +def test_a_mismatch_that_does_not_come_back_is_a_match(monkeypatch): + calls = [] + monkeypatch.setattr(common, "html_render_diff", renderer([False, True], calls)) + + assert common.compare_html(TEST1, TEST1, browser=object()) is True + assert len(calls) == 2 + + +def test_a_mismatch_that_comes_back_is_reported(monkeypatch, tmp_path): + calls = [] + monkeypatch.setattr(common, "html_render_diff", renderer([False], calls)) + + assert ( + common.compare_html(TEST1, TEST1, browser=object(), diff_output=tmp_path) + is False + ) + assert len(calls) == 2 + assert (tmp_path / "a.png").is_file() + assert (tmp_path / "b.png").is_file() + assert (tmp_path / "diff.png").is_file() + + +def test_a_match_is_rendered_once(monkeypatch): + calls = [] + monkeypatch.setattr(common, "html_render_diff", renderer([True], calls)) + + assert common.compare_html(TEST1, TEST1, browser=object()) is True + assert len(calls) == 1 + + +def test_retries_zero_reports_the_first_render(monkeypatch): + calls = [] + monkeypatch.setattr(common, "html_render_diff", renderer([False, True], calls)) + + assert common.compare_html(TEST1, TEST1, browser=object(), retries=0) is False + assert len(calls) == 1 + + +def test_retries_is_a_count(monkeypatch): + calls = [] + monkeypatch.setattr(common, "html_render_diff", renderer([True], calls)) + + with pytest.raises(ValueError): + common.compare_html(TEST1, TEST1, browser=object(), retries=-1) + with pytest.raises(ValueError): + common.compare_html(TEST1, TEST1, browser=object(), retries="two")