From dd5238d91b55df4cbc4270faef71261a010d09b8 Mon Sep 17 00:00:00 2001 From: Tristan Escalada <355457+tescalada@users.noreply.github.com> Date: Mon, 20 Jul 2026 20:28:17 -0400 Subject: [PATCH 1/2] Add a --regenerate-snapshots pytest option ContentTester.verify() writes a .page snapshot only when the file is absent; when it exists and the rendered content differs it fails the test. Re-recording snapshots therefore meant deleting every .page file, rebuilding the test image, running the suite, and copying the files back. With --regenerate-snapshots, a snapshot whose content differs is rewritten in place and a SnapshotRegenerated warning naming the file is emitted instead of failing. A snapshot whose content matches is left alone, so the resulting diff is the set of pages the source change affected. Without the flag, behavior is unchanged: compare, and fail on any difference. The rewrite goes through a temporary file in the same directory and os.replace, so an interrupted run cannot leave a half-written snapshot to be committed as the new baseline. The option is registered in a top level conftest.py because pytest only calls pytest_addoption for initial conftests, and with no path argument the rootdir is the only directory searched for one. regenerate_snapshots.sh drives a full regeneration run: the test image COPYs the source in rather than bind-mounting it, so the rewritten files are copied back out of the container. --- conftest.py | 43 ++++ deploy/Dockerfile | 6 + regenerate_snapshots.sh | 55 +++++ sparkmeter/web/tests/test_unittestutils.py | 249 +++++++++++++++++++++ sparkmeter/web/unittestutils.py | 54 +++++ 5 files changed, 407 insertions(+) create mode 100644 conftest.py create mode 100755 regenerate_snapshots.sh create mode 100644 sparkmeter/web/tests/test_unittestutils.py diff --git a/conftest.py b/conftest.py new file mode 100644 index 0000000..597314b --- /dev/null +++ b/conftest.py @@ -0,0 +1,43 @@ +# -*- coding: utf-8 -*- +# Copyright © 2026 SparkMeter, Inc. +# All Rights Reserved. +"""Top level pytest configuration. + +This file exists at the top of the checkout, rather than alongside the tests +in sparkmeter/conftest.py, because pytest only calls pytest_addoption for +*initial* conftests -- the ones found by walking down to the invocation's +anchor directory before the command line is parsed. With no path argument the +anchor is the rootdir, so a conftest.py one directory further down is loaded +too late for its options to exist: `pytest --regenerate-snapshots` would be +rejected as an unrecognized argument. +""" + +# pytest's own `pytester` fixture, used by the tests that run pytest on a +# throwaway test to check this file's wiring end to end. `pytest_plugins` is +# only honored in the top level conftest, which is here. +pytest_plugins = ["pytester"] + + +def pytest_addoption(parser): + """Register our own command line options.""" + parser.addoption( + "--regenerate-snapshots", + action="store_true", + default=False, + help=( + "Rewrite the .page snapshot of any content test whose rendered " + "output differs, instead of failing the test." + ), + ) + + +def pytest_configure(config): + """Propagate command line options to the code that needs them. + + ContentTester is a plain class with no fixture access, so the option is + handed to it as a module level flag. This runs once per process, which + covers each xdist worker as well as a plain single-process run. + """ + from sparkmeter.web import unittestutils + + unittestutils.regenerate_snapshots = config.getoption("--regenerate-snapshots") diff --git a/deploy/Dockerfile b/deploy/Dockerfile index 733e0e9..59e3597 100755 --- a/deploy/Dockerfile +++ b/deploy/Dockerfile @@ -169,6 +169,12 @@ USER spark COPY --chown=spark:spark setup.cfg setup.cfg +# The top level conftest.py, which registers the suite's own command line +# options. It has to be at the top of the tree, next to setup.cfg: pytest only +# calls pytest_addoption for initial conftests, and with no path argument the +# rootdir is the only directory searched for one. +COPY --chown=spark:spark conftest.py conftest.py + # Test fixtures: the committed snapshot baseline that the snapshot tests # (verify()) compare generated output against. Without it the tests find # the expected files absent and write them fresh every run instead of diff --git a/regenerate_snapshots.sh b/regenerate_snapshots.sh new file mode 100755 index 0000000..bb0c286 --- /dev/null +++ b/regenerate_snapshots.sh @@ -0,0 +1,55 @@ +#!/usr/bin/env bash +# +# Regenerate the .page snapshots under test-data/ from the current sources. +# +# Run from the top level of the checkout to regenerate -- the main checkout or +# any worktree: +# +# ./regenerate_snapshots.sh +# +# The suite runs with --regenerate-snapshots, which makes a content test whose +# rendered output differs rewrite its .page file instead of failing; see +# ContentTester.verify in sparkmeter/web/unittestutils.py. Matching snapshots +# are not touched, so the diff is the set of pages the source change affected. +# Orphans are not pruned: the .page file of a renamed or deleted test is left +# in place and is not reported, so finding those is a manual step. +# +set -euo pipefail + +# The image build derives the version from .git via hatch-vcs. In a worktree +# .git is a file pointing elsewhere rather than a directory, so that fails and +# SETUPTOOLS_SCM_PRETEND_VERSION supplies the version instead. Built as its own +# step because `docker compose run --build` cannot forward --build-arg. +docker compose -f docker-compose.test.yml build \ + --build-arg SETUPTOOLS_SCM_PRETEND_VERSION="0.0.0+g$(git rev-parse --short HEAD)" \ + test + +# Compose namespaces its own containers by project, but this one is created +# with an explicit --name, so it needs the checkout in its name too. Otherwise +# the pre-run `docker rm -f` below would remove a container another checkout is +# using: docker-compose.test.yml is written so two checkouts can run their +# suites concurrently. +container="$(basename "$PWD")-snapshot-regen" + +docker rm -f "$container" >/dev/null 2>&1 || true + +# The run's status is captured rather than allowed to abort the script: a test +# failing for a reason unrelated to snapshots would otherwise end the run +# before the copy back, discarding every snapshot it had already rewritten. +status=0 +docker compose -f docker-compose.test.yml run --name "$container" test \ + uv run pytest -n auto --regenerate-snapshots || status=$? + +# The test image COPYs the source in rather than bind-mounting the working +# tree, so the rewritten files exist only inside the container -- hence the +# name instead of --rm, and this copy back. +if ! docker cp "$container:/app/test-data/." ./test-data/; then + echo "Failed to copy the regenerated snapshots out of the container." >&2 + echo "Leaving the container in place so the run can still be retrieved:" >&2 + echo " docker cp $container:/app/test-data/. ./test-data/" >&2 + exit 1 +fi + +docker rm -f "$container" >/dev/null || true + +exit "$status" diff --git a/sparkmeter/web/tests/test_unittestutils.py b/sparkmeter/web/tests/test_unittestutils.py new file mode 100644 index 0000000..42a7574 --- /dev/null +++ b/sparkmeter/web/tests/test_unittestutils.py @@ -0,0 +1,249 @@ +# -*- coding: utf-8 -*- +# Copyright © 2026 SparkMeter, Inc. +# All Rights Reserved. +"""Unittests for the snapshot comparison in ContentTester.verify().""" + +import os +import warnings + +import pytest + +from sparkmeter.web import unittestutils +from sparkmeter.web.unittestutils import ContentTester, SnapshotRegenerated + +# An mtime far enough in the past that any rewrite moves it. +OLD_MTIME = 1000000000 + + +class SnapshotTester(ContentTester): + """A ContentTester whose snapshot lives at an explicit path. + + ContentTester derives expected_filename from the caller's stack frame and + the test-data tree; these tests need a throwaway file under tmp_path + instead. The frame is still the test method that built this object, so + ``name`` -- which is what the regeneration warning reports -- keeps its + normal shape. + """ + + def __init__(self, path): + """Create a tester reading and writing the snapshot at ``path``.""" + super(SnapshotTester, self).__init__(frame=2) + self.path = str(path) + + @property + def expected_filename(self): + """Get the snapshot path this tester was pointed at.""" + return self.path + + +def write_snapshot(path, content): + """Write a snapshot file and backdate it so a rewrite is detectable.""" + with open(path, "w", encoding="utf-8") as fp: + fp.write(content) + os.utime(path, (OLD_MTIME, OLD_MTIME)) + + +def read_snapshot(path): + """Read a snapshot file back.""" + with open(path, encoding="utf-8") as fp: + return fp.read() + + +def regeneration_warnings(caught): + """Pick the snapshot regeneration warnings out of recorded warnings. + + Unrelated warnings from application imports surface in the same recorder, + so select the dedicated category rather than every warning raised. + """ + return [w for w in caught if issubclass(w.category, SnapshotRegenerated)] + + +class ContentTesterVerifyTest(object): + @pytest.fixture(autouse=True) + def flag_off(self, monkeypatch): + """Keep the module level flag off unless a test turns it on. + + It is process global, so leaking it would change how later tests in + the same worker compare their snapshots. + """ + monkeypatch.setattr(unittestutils, "regenerate_snapshots", False) + + @pytest.fixture() + def regenerating(self, monkeypatch): + """Turn on snapshot regeneration for the duration of a test.""" + monkeypatch.setattr(unittestutils, "regenerate_snapshots", True) + + @pytest.fixture() + def snapshot(self, tmp_path): + """Path of the snapshot file under test.""" + return tmp_path / "example.page" + + def test_matching_content_passes_without_touching_the_file(self, snapshot): + """Flag off, content matches: test passes and the file is untouched.""" + write_snapshot(snapshot, "hello\n") + tester = SnapshotTester(snapshot) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + tester.verify("hello") + + assert regeneration_warnings(caught) == [] + assert read_snapshot(snapshot) == "hello\n" + assert os.stat(snapshot).st_mtime == OLD_MTIME + + def test_differing_content_fails_and_leaves_the_file_alone(self, snapshot): + """Flag off, content differs: the existing failure path runs.""" + write_snapshot(snapshot, "hello\n") + tester = SnapshotTester(snapshot) + + with pytest.raises(pytest.fail.Exception): + tester.verify("goodbye") + + assert read_snapshot(snapshot) == "hello\n" + assert os.stat(snapshot).st_mtime == OLD_MTIME + + def test_differing_content_is_rewritten_when_regenerating(self, snapshot, regenerating): + """Flag on, content differs: the file is rewritten and a warning names its path.""" + write_snapshot(snapshot, "hello\n") + tester = SnapshotTester(snapshot) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + tester.verify("goodbye") + + assert read_snapshot(snapshot) == "goodbye\n" + assert os.stat(snapshot).st_mtime != OLD_MTIME + regenerated = regeneration_warnings(caught) + assert [str(w.message) for w in regenerated] == ["Regenerated snapshot: %s" % (snapshot,)] + + def test_rewriting_leaves_no_temporary_file_behind(self, snapshot, regenerating): + """The rewrite goes through a temporary file, which must not survive it. + + The whole test-data tree is copied out of the container after a + regeneration run, so a leftover temporary would be committed too. + """ + write_snapshot(snapshot, "hello\n") + tester = SnapshotTester(snapshot) + + with warnings.catch_warnings(record=True): + warnings.simplefilter("always") + tester.verify("goodbye") + + assert sorted(os.listdir(os.path.dirname(str(snapshot)))) == ["example.page"] + + def test_matching_content_is_not_rewritten_when_regenerating(self, snapshot, regenerating): + """Flag on, content matches: no rewrite, no warning, no mtime churn.""" + write_snapshot(snapshot, "hello\n") + tester = SnapshotTester(snapshot) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + tester.verify("hello") + + assert regeneration_warnings(caught) == [] + assert read_snapshot(snapshot) == "hello\n" + assert os.stat(snapshot).st_mtime == OLD_MTIME + + @pytest.mark.parametrize("regenerate", [False, True]) + def test_absent_file_is_written_without_warning(self, snapshot, monkeypatch, regenerate): + """Either flag state, no file: it is written and no warning is emitted.""" + monkeypatch.setattr(unittestutils, "regenerate_snapshots", regenerate) + tester = SnapshotTester(snapshot) + assert not os.path.exists(snapshot) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + tester.verify("hello") + + assert regeneration_warnings(caught) == [] + assert read_snapshot(snapshot) == "hello\n" + + def test_snapshot_differing_only_in_leading_whitespace_stays_stale(self, snapshot, regenerating): + """Leading whitespace is stripped from both sides before comparing. + + Nothing on disk is normalized as a result: a snapshot whose only + difference from the rendered content is leading whitespace compares + equal, so it is not rewritten and its stored form survives verbatim. + """ + write_snapshot(snapshot, "\n\n hello\n") + tester = SnapshotTester(snapshot) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + tester.verify(" hello") + + assert regeneration_warnings(caught) == [] + assert read_snapshot(snapshot) == "\n\n hello\n" + assert os.stat(snapshot).st_mtime == OLD_MTIME + + +# --------------------------------------------------------------------------- +# The conftest wiring, exercised by running pytest on a throwaway test +# --------------------------------------------------------------------------- + +# The top level conftest.py, i.e. the one actually shipped. The runs below use +# its real source rather than a restatement of it, so they cover the option's +# registration, its default, and pytest_configure handing it to unittestutils. +ROOT_CONFTEST = os.path.join(os.path.abspath(unittestutils.rootdir), "conftest.py") + +# A test rendering content that differs from its snapshot -- the one case whose +# outcome --regenerate-snapshots changes. +DIFFERING_SNAPSHOT_TEST = ''' +from sparkmeter.web.unittestutils import ContentTester + + +class SnapshotTester(ContentTester): + """A ContentTester whose snapshot lives at an explicit path.""" + + def __init__(self, path): + """Create a tester reading and writing the snapshot at ``path``.""" + super(SnapshotTester, self).__init__(frame=2) + self.path = path + + @property + def expected_filename(self): + """Get the snapshot path this tester was pointed at.""" + return self.path + + +def test_content_differs_from_its_snapshot(): + """Render content that does not match the snapshot on disk.""" + SnapshotTester(%r).verify("goodbye") +''' + + +@pytest.fixture() +def stale_snapshot(pytester, monkeypatch): + """Build a pytest run of one test whose snapshot on disk is out of date. + + Returns the path of that snapshot. The inner runs set the module level + flag exactly as a real run does -- that is the point of them -- so it is + pinned through monkeypatch here and restored when the test ends. + """ + monkeypatch.setattr(unittestutils, "regenerate_snapshots", False) + + with open(ROOT_CONFTEST, encoding="utf-8") as fp: + pytester.makeconftest(fp.read()) + + snapshot = pytester.path / "example.page" + write_snapshot(snapshot, "hello\n") + pytester.makepyfile(test_snapshot=DIFFERING_SNAPSHOT_TEST % (str(snapshot),)) + return snapshot + + +def test_run_without_the_flag_fails_and_keeps_the_snapshot(pytester, stale_snapshot): + """The shipped default is off: a differing snapshot fails and is not touched.""" + result = pytester.runpytest() + + result.assert_outcomes(failed=1) + assert read_snapshot(stale_snapshot) == "hello\n" + assert os.stat(stale_snapshot).st_mtime == OLD_MTIME + + +def test_run_with_the_flag_rewrites_the_snapshot_and_warns(pytester, stale_snapshot): + """With --regenerate-snapshots the same run passes, rewriting the snapshot.""" + result = pytester.runpytest("--regenerate-snapshots") + + result.assert_outcomes(passed=1) + assert read_snapshot(stale_snapshot) == "goodbye\n" + result.stdout.fnmatch_lines(["*SnapshotRegenerated: Regenerated snapshot: %s*" % (stale_snapshot,)]) diff --git a/sparkmeter/web/unittestutils.py b/sparkmeter/web/unittestutils.py index c52dfe3..142ad5d 100644 --- a/sparkmeter/web/unittestutils.py +++ b/sparkmeter/web/unittestutils.py @@ -7,8 +7,11 @@ import difflib import os import re +import stat import sys +import tempfile import urllib.parse +import warnings from builtins import str import html5lib @@ -24,6 +27,46 @@ rootdir = os.path.join(os.path.dirname(__file__), "..", "..") +# When true, ContentTester.verify() rewrites a snapshot whose content differs +# instead of failing the test, and warns naming the rewritten snapshot. Set from +# pytest_configure() in the top level conftest.py when --regenerate-snapshots is +# passed; each xdist worker is its own process and sets it independently. +regenerate_snapshots = False + + +class SnapshotRegenerated(UserWarning): + """Warning issued for each .page snapshot rewritten by --regenerate-snapshots. + + A dedicated category so the warnings are greppable in a run's output and so + tests can select them by category rather than by message text. + """ + + +def rewrite_snapshot(path, content): + """Replace the snapshot at ``path`` with ``content``, atomically. + + A regeneration run rewrites up to one file per content test and the + results are copied out of the container afterwards, so a run interrupted + part way through must not leave a half written .page file behind to be + committed as the new baseline. The content is written to a temporary file + in the same directory (same filesystem, so the rename is atomic), flushed + and closed, then moved onto the snapshot path with os.replace(). + """ + directory = os.path.dirname(path) or "." + fd, tmppath = tempfile.mkstemp(dir=directory, prefix=os.path.basename(path) + ".", suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding="utf-8") as fp: + fp.write(content) + # mkstemp creates the temporary 0600; carry over the permissions of the + # file being replaced so a regenerated snapshot is readable as before. + if os.path.exists(path): + os.chmod(tmppath, stat.S_IMODE(os.stat(path).st_mode)) + os.replace(tmppath, path) + except BaseException: # pragma: nocoverage + if os.path.exists(tmppath): + os.unlink(tmppath) + raise + def validate_html(data, ignores=None): """Validate the html in the request's response.""" @@ -139,6 +182,17 @@ def verify(self, content): expected_content = open(expected_filename, encoding="utf-8").read() expected_content = expected_content.lstrip() + if expected_content == content: + return + + if regenerate_snapshots: + rewrite_snapshot(expected_filename, content) + warnings.warn( + "Regenerated snapshot: {}".format(expected_filename), + SnapshotRegenerated, + ) + return + self._diff_lines( expected_content.split("\n"), content.split("\n"), From 016a8e22e4a1ac34dae8a04dff79f2d952a3bd35 Mon Sep 17 00:00:00 2001 From: Tristan Escalada <355457+tescalada@users.noreply.github.com> Date: Mon, 20 Jul 2026 20:28:24 -0400 Subject: [PATCH 2/2] Report the suite's exit status from run_coverage.sh The inner sh -c ran pytest through tee and then tar, so the container's exit status was tar's. An ordinary red suite still failed the job, but only because --no-cov-on-fail suppresses .coverage.xml and tar then errored on the missing file. A run that wrote the reports and still exited non-zero -- pytest's exit 5, "no tests collected", from a -k that matches nothing -- left tar succeeding and the job green. pipefail makes the pipeline report pytest's status, it is captured before tar runs, and the container exits with it. On the host the status is captured rather than allowed to abort, so the script runs to the end, and it exits with that status ahead of the coverage gate. A suite that fails writes no coverage at all, so the gate is now skipped with a message rather than letting diff-cover fail on the missing file. The artifacts are cleared before the run so that a run which fails before writing them cannot leave the previous run's files behind to be read as current. --- scripts/run_coverage.sh | 56 +++++++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/scripts/run_coverage.sh b/scripts/run_coverage.sh index 2d27801..0c822d1 100644 --- a/scripts/run_coverage.sh +++ b/scripts/run_coverage.sh @@ -24,9 +24,30 @@ docker compose -f docker-compose.test.yml build \ # pytest-coverage.txt for the PR report comment. All three are produced in the # container, tarred to stdout, and unpacked on the host. pytest's output is teed # to stderr, not stdout, so stdout carries only the tar stream. +# +# pytest's exit status has to be carried out by hand. `pipefail` makes the +# pipeline report pytest's status rather than tee's, it is captured before tar +# runs, and the container exits with it. Otherwise the container's status is +# tar's: an ordinary red suite still failed the job, but only because +# --no-cov-on-fail suppresses .coverage.xml and tar then errors on the missing +# file. A run that writes the reports and still exits non-zero -- pytest's +# exit 5, "no tests collected", from a -k that matches nothing -- left tar +# succeeding and the job green. On the host the status is captured rather than +# allowed to abort, so the script runs to the end and reports it there instead +# of dying mid-script; when coverage did reach the host, the diff-cover report +# below is still produced. +# +# The artifacts are cleared first so that a run which fails before writing them +# cannot leave the previous run's files behind to be read as current -- the gate +# below reads .coverage.xml, and the CI report steps read the other two. +rm -f .coverage.xml junit.xml pytest-coverage.txt + +set +e docker compose -f docker-compose.test.yml run --rm test \ - sh -c 'uv run pytest -n auto --junitxml=/app/junit.xml 2>&1 | tee /app/pytest-coverage.txt 1>&2; tar -c -C /app .coverage.xml junit.xml pytest-coverage.txt' \ + sh -c 'set -o pipefail; uv run pytest -n auto --junitxml=/app/junit.xml 2>&1 | tee /app/pytest-coverage.txt 1>&2; status=$?; tar -c -C /app .coverage.xml junit.xml pytest-coverage.txt; exit $status' \ | tar -x +tests_rc=${PIPESTATUS[0]} +set -e # diff-cover gates the changed lines and always prints a terminal report. It # also writes a markdown report, which is appended to the GitHub Actions job @@ -40,16 +61,35 @@ fi # --fail-under exits non-zero when changed-line coverage is below the threshold. # Capture that instead of aborting so the report still reaches the job summary. -set +e -uvx "diff-cover==10.3.0" .coverage.xml \ - --compare-branch "$compare_branch" \ - --fail-under "$fail_under" \ - --format "$formats" -diff_cover_rc=$? -set -e +# +# A suite that fails writes no coverage at all (--no-cov-on-fail in setup.cfg), +# so there is nothing to gate. Skip it and say so, rather than letting +# diff-cover fail on the missing file with a traceback that reads like a broken +# tool instead of a failing test. The suite's own status is reported below. +diff_cover_rc=0 +if [ -f .coverage.xml ]; then + set +e + uvx "diff-cover==10.3.0" .coverage.xml \ + --compare-branch "$compare_branch" \ + --fail-under "$fail_under" \ + --format "$formats" + diff_cover_rc=$? + set -e +else + # Drop any report from a previous run so the summary below cannot append a + # stale one for a gate that did not run. + rm -f "$report_md" + echo "No coverage was produced, so the coverage gate is skipped." >&2 +fi if [ -n "${GITHUB_STEP_SUMMARY:-}" ] && [ -f "$report_md" ]; then cat "$report_md" >> "$GITHUB_STEP_SUMMARY" fi +# A failing suite fails the job. It is reported ahead of the coverage gate +# because a red run makes the coverage number meaningless. +if [ "$tests_rc" -ne 0 ]; then + exit "$tests_rc" +fi + exit "$diff_cover_rc"