Settle the page before the shot, and render a mismatch again

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:

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,

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.
 

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")