From 9f86ddfc5bdd83008ad610e4e5573865c17a0ea7 Mon Sep 17 00:00:00 2001 From: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com> Date: Sun, 6 Sep 2026 13:42:24 -0700 Subject: [PATCH] A refresh replaced both artifacts wholesale and nothing said what it moved The figures on these pages move only when the pinned retrievals are deliberately refreshed, and a refresh rewrites site/data/perimeters-coverage.json and dins-coverage.json in full. The only account of what changed was git diff over a large JSON document, which answers a different question: it reports lines, so a reordered list reads as hundreds of changes and one count that moved reads as two. python -m perimeter.diff OLD NEW walks every leaf of both documents and reports the value at its path with both sides. Exit 0 no change, 1 changes reported, 2 a refused removal or an unreadable input. --json writes the rows sorted by path, byte-identical on repeat, so a refresh can cite a comparison in PROVENANCE.md. make diff wraps it for a terminal, and records in the Makefile that make collapses every recipe failure to its own exit 2, so a script that needs the three codes calls the module. Three distinctions decide the design. A key the later artifact stops publishing is not a value moving: it is refused unless ALLOW_REMOVALS names it deliberate. A number becoming null is a change to absence and never a removal, because ADR-0010 writes a domain the layer stopped publishing as null rather than dropping it. And 1000 and 1000.0 are a type change, because every percentage here is a *_tenths_pct integer precisely so that no float decides an equality. make site-check now prints the leaf comparison before its byte-for-byte check decides. The comparison is the report and diff -r is still the gate: the report's exit status is discarded deliberately, so it can add detail and can never turn a red target green. Refusals per ADR-0004: a missing file, an empty file, an unparseable one and a JSON document whose top level is not an object are each refused rather than parsed into an empty document, because two empty documents compare equal and would report no change about two artifacts that were never read. Running the tests against the faults they exist to catch found a weak assertion in this change's own test file. With the emptiness check deleted, an empty file still failed as unparseable JSON and match='empty' was satisfied by the fixture filename empty.json. The fixture is renamed and the pattern now matches the reason. Closes #58 --- CHANGELOG.md | 33 ++++ Makefile | 33 +++- README.md | 28 ++++ src/perimeter/diff.py | 343 ++++++++++++++++++++++++++++++++++++++++++ tests/test_diff.py | 310 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 746 insertions(+), 1 deletion(-) create mode 100644 src/perimeter/diff.py create mode 100644 tests/test_diff.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ce1c997..16f1c1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,39 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this project a ## [Unreleased] +### Added, a refresh can now say what it moved + +- **`python -m perimeter.diff OLD NEW`, and `make diff`.** A refresh of the pinned + retrievals replaces both coverage artifacts wholesale, and until now the only account of + what changed was `git diff` over a large JSON document, which answers a different + question. It reports lines, so a reordered list reads as hundreds of changes and one + count that moved reads as two. This walks every leaf of both documents and reports the + value at its path, with both sides. Exit `0` no change, `1` changes reported, `2` a + refused removal or an unreadable input. `--json` writes the rows sorted by path, so the + output is byte-identical on repeat and a refresh can cite it in `PROVENANCE.md`. +- **A key that stops being published is refused, not reported as a change.** `--allow-removals` + is how a deliberate one gets through, and it is the only way. A key present in the + earlier artifact and absent from the later one means the build stopped publishing + something, which is a different event from a number moving. +- **A number becoming `null` is a change to absence, never a removal.** ADR-0010 writes a + domain the layer stopped publishing as `null` rather than omitting it, so collapsing the + two would lose the distinction the artifact exists to carry. +- **Nothing is compared as a float.** `1000` and `1000.0` are equal in Python and are not + the same published value; the comparison reports a type change. Empty containers get a + marker leaf, so deleting `"markers": {}` outright is visible rather than invisible. +- **`make site-check` prints the leaf comparison before the byte-for-byte check decides.** + The comparison is a report and `diff -r` is still the gate; the report's exit status is + discarded deliberately, so it can add detail and can never turn a red target green. +- **Refusals, per ADR-0004.** A missing file, an empty file, an unparseable one, and a + JSON document whose top level is not an object are each refused rather than parsed into + an empty document. Two empty documents compare equal, and would report "no change" + about two artifacts that were never read. +- The gate was run against the faults it exists to catch, and one of those runs found a + weak assertion in this repository's own new test: with the emptiness check deleted, an + empty file still failed as unparseable JSON and `match="empty"` was satisfied by the + fixture's filename, `empty.json`. The fixture is renamed and the pattern now matches the + reason. A control that passes for the wrong reason is a control that is not there. + ### Fixed, the browser accessibility gate was shallowing this checkout - **Every pull request's `verify` job could fail on five tests about tags.** On a diff --git a/Makefile b/Makefile index 36d66ba..7fb4bff 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,5 @@ .PHONY: verify lock-check sync lint format typecheck test audit site site-offline \ - site-check acquire pages node-sync htmlvalidate a11y node-audit determinism \ + site-check diff acquire pages node-sync htmlvalidate a11y node-audit determinism \ browser-sync a11y-browser browser-audit # CI / `make verify` body: the two MUST stay byte-for-byte identical. @@ -76,9 +76,40 @@ site-check: --perimeters data/raw/frap_perimeters.json \ --dins data/raw/dins_postfire.json \ --out build/site-current + @# What moved, before what follows decides whether it may. `diff -r` is the gate; + @# this is the report, and it is written so it can only ever add detail: its exit + @# status is deliberately discarded here and the byte comparison below is what + @# fails the target. A reporting step that could turn a red target green would be + @# the swallowed-failure defect ADR-0004 is about. + @for f in perimeters-coverage.json dins-coverage.json; do \ + uv run python -m perimeter.diff "site/data/$$f" "build/site-current/data/$$f" \ + --allow-removals || true; \ + done diff -r site build/site-current @echo "site-check: site/ is byte-identical to a fresh build from data/raw/" +# Compare two coverage artifacts leaf by leaf. Offline, reads only the two files named. +# +# `git diff` answers a different question about these documents: it reports lines, so a +# reordered list reads as hundreds of changes and one count that moved reads as two. This +# reports values, with their paths, and refuses a key the later artifact stopped +# publishing unless ALLOW_REMOVALS names that as deliberate. Exit 0 no change, 1 changes +# reported, 2 a refused removal or an unreadable input. +# +# make diff OLD=site/data/dins-coverage.json NEW=build/site-current/data/dins-coverage.json +# make diff OLD=a.json NEW=b.json ALLOW_REMOVALS=1 IGNORE=is_fixture +# +# Those three exit codes are the module's. Make collapses every recipe failure to its own +# exit 2, so a script that needs to tell "changes reported" from "removal refused" must +# call `uv run python -m perimeter.diff` directly. This target is for a person at a +# terminal, where the printed lines carry the distinction. +DIFF_FLAGS = $(if $(ALLOW_REMOVALS),--allow-removals,) $(if $(IGNORE),--ignore $(IGNORE),) $(if $(JSON),--json,) + +diff: + @test -n "$(OLD)" || { echo "make diff: set OLD=" >&2; exit 2; } + @test -n "$(NEW)" || { echo "make diff: set NEW=" >&2; exit 2; } + uv run python -m perimeter.diff "$(OLD)" "$(NEW)" $(DIFF_FLAGS) + # The same pipeline over committed fixtures: runs anywhere, output flagged is_fixture. site-offline: uv run python -m perimeter.cli --fixture \ diff --git a/README.md b/README.md index 72c74b2..c02dfde 100644 --- a/README.md +++ b/README.md @@ -152,8 +152,36 @@ uv sync npm ci make verify # lockfile, lint, format, types, tests, SCA, page checks, determinism make site-offline # build from committed fixtures; runs anywhere, no network +make diff OLD=a.json NEW=b.json # compare two coverage artifacts leaf by leaf ``` +### What a refresh moved + +The figures on these pages move only when the pinned retrievals are deliberately +refreshed, and a refresh replaces both JSON artifacts wholesale. `make diff` says what +that changed, value by value rather than line by line: + +```sh +make diff OLD=site/data/dins-coverage.json NEW=build/site-current/data/dins-coverage.json +``` + +Every leaf is compared at its path (`/fields[3]/present`), with both values printed. +`--json` writes the same rows sorted by path, so a refresh can cite a comparison in +`PROVENANCE.md` rather than a screenshot of `git diff`. Three distinctions it keeps: + +- A key the later artifact **stops publishing** is not the same event as a value moving. + It is refused outright unless `ALLOW_REMOVALS=1` names it as deliberate. +- A number that becomes `null` is a **change to absence**, reported with both sides, and + never a removal. ADR-0010 writes a domain the layer stopped publishing as `null`. +- `1000` and `1000.0` are a type change and are reported as one. Every percentage in + these artifacts is a `*_tenths_pct` integer so that no float decides an equality, and + nothing here converts, rounds, or tolerances a value. + +An empty, missing, or unparseable input is refused rather than compared: two empty files +compare equal, and "no change" about two files that were never read is the failure this +whole repository is organised against. `make site-check` prints the same leaf comparison +before its byte-for-byte check decides, so a drift report names the values that moved. + `make verify` includes `make pages`, which builds the pages from the committed fixtures and checks them four ways: `html-validate` for HTML conformance and the markup-level accessibility rules, `axe-core` in a headless DOM for the WCAG 2.0, 2.1 and 2.2 A and AA diff --git a/src/perimeter/diff.py b/src/perimeter/diff.py new file mode 100644 index 0000000..ab7d26c --- /dev/null +++ b/src/perimeter/diff.py @@ -0,0 +1,343 @@ +"""Compare two coverage artifacts value by value. + +A refresh of the pinned retrievals replaces ``site/data/perimeters-coverage.json`` and +``site/data/dins-coverage.json`` wholesale. ``git diff`` over those documents shows which +*lines* moved, which is not the same question: a reordered list reads as hundreds of +changes and a single count that moved reads as two. README.md promises the figures "move +only when those retrievals are deliberately refreshed", and a deliberate refresh should be +able to say, leaf by leaf, exactly what it moved. + +Three rules decide the shape of this module. + +**A disappearance is not a change.** A key present in the old artifact and absent from the +new one means the build stopped publishing something, and that is a different event from a +number moving. It is refused (exit 2) unless ``--allow-removals`` names it as deliberate. + +**Absence is a value, and a removal is not.** ADR-0010 records that a domain the layer +stopped publishing is published as ``null``, not omitted. So a field going from ``12`` to +``null`` is a *change to absence*, reported with both sides, and is never counted as a +removal. Collapsing the two would be this portfolio's most common defect in reverse: +absence and non-publication are different facts and the reader needs both. + +**Integers are compared as integers.** Every percentage in these artifacts is a +``*_tenths_pct`` integer precisely so that no float ever decides an equality. Nothing here +converts, rounds, or tolerances a value; ``1000`` and ``1000.0`` are a type change and are +reported as one. + +Offline, deterministic, and reads only the two files it is given. +""" + +from __future__ import annotations + +import argparse +import json +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Any, TextIO + +EMPTY_OBJECT = "" +"""Marker leaf for ``{}``. + +An empty container holds no leaves, so a walk that emitted only scalars would give +``"markers": {}`` no path at all, and deleting the key entirely would then look like no +change. Emitting a marker keeps the key visible to the removal check. +""" + +EMPTY_ARRAY = "" +"""Marker leaf for ``[]``, for the same reason.""" + +Leaf = str | int | float | bool | None + + +@dataclass(frozen=True) +class Change: + """One leaf whose value differs between the two artifacts.""" + + path: str + old: Leaf + new: Leaf + + @property + def is_type_change(self) -> bool: + """``1000`` to ``1000.0``, or ``0`` to ``"0"``: equal-looking, differently typed.""" + return type(self.old) is not type(self.new) + + def as_row(self) -> dict[str, Any]: + return {"path": self.path, "old": self.old, "new": self.new} + + +@dataclass(frozen=True) +class Removal: + """A leaf the old artifact published and the new one does not carry at all.""" + + path: str + old: Leaf + + def as_row(self) -> dict[str, Any]: + return {"path": self.path, "old": self.old} + + +@dataclass(frozen=True) +class Addition: + """A leaf the new artifact publishes and the old one did not.""" + + path: str + new: Leaf + + def as_row(self) -> dict[str, Any]: + return {"path": self.path, "new": self.new} + + +@dataclass(frozen=True) +class Comparison: + """The whole result, sorted by path so two runs produce identical bytes.""" + + changes: tuple[Change, ...] + additions: tuple[Addition, ...] + removals: tuple[Removal, ...] + old_leaves: int + new_leaves: int + + @property + def unchanged(self) -> bool: + return not (self.changes or self.additions or self.removals) + + def as_document(self) -> dict[str, Any]: + return { + "changes": [c.as_row() for c in self.changes], + "additions": [a.as_row() for a in self.additions], + "removals": [r.as_row() for r in self.removals], + "leaves_compared": {"old": self.old_leaves, "new": self.new_leaves}, + } + + +class ArtifactUnreadable(Exception): + """The input is not an artifact this can compare, so no comparison is reported. + + Per ADR-0004: a check that could not run is not a check that passed. Every way of + failing to read a file ends here rather than in an empty document that would then + compare equal to another empty document. + """ + + +def read_artifact(path: Path) -> dict[str, Any]: + """Parse one artifact, or refuse. Never returns an empty document as a default.""" + if not path.is_file(): + raise ArtifactUnreadable(f"{path}: no such file") + raw = path.read_text(encoding="utf-8") + if not raw.strip(): + raise ArtifactUnreadable( + f"{path}: file is empty. An empty file compares equal to another empty " + "file, which would report 'no change' about two artifacts that were " + "never read." + ) + try: + loaded = json.loads(raw) + except json.JSONDecodeError as exc: + raise ArtifactUnreadable(f"{path}: not parseable JSON: {exc}") from exc + if not isinstance(loaded, dict): + raise ArtifactUnreadable( + f"{path}: top level is {type(loaded).__name__}, not a JSON object" + ) + return loaded + + +def _escape(segment: str) -> str: + """Keep a path unambiguous when a key contains the characters paths are built from.""" + return segment.replace("~", "~0").replace("/", "~1") + + +def flatten(document: Any, prefix: str = "") -> dict[str, Leaf]: + """Every leaf in the document, keyed by a path like ``/fields[3]/present``. + + Lists are walked by index. These artifacts write every list in a declared order + (fields in registry order, incidents sorted, years ascending), so index is a stable + identity here and a reordering is a real finding rather than diff noise. + """ + if isinstance(document, dict): + if not document: + return {prefix or "/": EMPTY_OBJECT} + leaves: dict[str, Leaf] = {} + for key, value in document.items(): + leaves.update(flatten(value, f"{prefix}/{_escape(str(key))}")) + return leaves + if isinstance(document, list): + if not document: + return {prefix or "/": EMPTY_ARRAY} + listed: dict[str, Leaf] = {} + for index, value in enumerate(document): + listed.update(flatten(value, f"{prefix}[{index}]")) + return listed + return {prefix or "/": document} + + +def _ignored(path: str, ignore: frozenset[str]) -> bool: + """True when any segment of the path is a name the caller asked to ignore. + + ``--ignore is_fixture`` is the documented use: comparing a fixture build against + itself, where that one flag is expected to differ and nothing else is. + """ + if not ignore: + return False + segments = path.replace("[", "/").replace("]", "").split("/") + return any(segment in ignore for segment in segments) + + +def compare( + old: dict[str, Any], + new: dict[str, Any], + *, + ignore: frozenset[str] = frozenset(), +) -> Comparison: + """Leaf-by-leaf comparison of two parsed artifacts. + + A pure function of two documents, so the tests can run it over documents it must + report on rather than only over the committed pair. + """ + old_leaves = {p: v for p, v in flatten(old).items() if not _ignored(p, ignore)} + new_leaves = {p: v for p, v in flatten(new).items() if not _ignored(p, ignore)} + + changes = tuple( + sorted( + ( + Change(path, old_leaves[path], new_leaves[path]) + for path in old_leaves.keys() & new_leaves.keys() + if old_leaves[path] != new_leaves[path] + or type(old_leaves[path]) is not type(new_leaves[path]) + ), + key=lambda change: change.path, + ) + ) + additions = tuple( + sorted( + (Addition(p, new_leaves[p]) for p in new_leaves.keys() - old_leaves.keys()), + key=lambda addition: addition.path, + ) + ) + removals = tuple( + sorted( + (Removal(p, old_leaves[p]) for p in old_leaves.keys() - new_leaves.keys()), + key=lambda removal: removal.path, + ) + ) + return Comparison( + changes=changes, + additions=additions, + removals=removals, + old_leaves=len(old_leaves), + new_leaves=len(new_leaves), + ) + + +def _render(value: Leaf) -> str: + """One rendering for every value, so ``null`` never prints as a blank.""" + if value is None: + return "null (absent, per ADR-0010)" + if isinstance(value, bool): + return "true" if value else "false" + return json.dumps(value) + + +def render_text(comparison: Comparison, *, allow_removals: bool) -> str: + """The console rendering. States the population it compared, never only the diff.""" + lines: list[str] = [] + if comparison.unchanged: + lines.append( + f"no change: {comparison.old_leaves} leaves compared, none differ, " + "none added, none removed" + ) + return "\n".join(lines) + "\n" + + if comparison.removals: + heading = ( + "removed (allowed)" if allow_removals else "REMOVED (refused, see below)" + ) + lines.append(f"{heading}: {len(comparison.removals)}") + for removal in comparison.removals: + lines.append(f" {removal.path}: {_render(removal.old)} -> not published") + if comparison.additions: + lines.append(f"added: {len(comparison.additions)}") + for addition in comparison.additions: + lines.append(f" {addition.path}: not published -> {_render(addition.new)}") + if comparison.changes: + lines.append(f"changed: {len(comparison.changes)}") + for change in comparison.changes: + note = " [type change]" if change.is_type_change else "" + lines.append( + f" {change.path}: {_render(change.old)} -> {_render(change.new)}{note}" + ) + lines.append( + f"{comparison.old_leaves} leaves in the earlier artifact, " + f"{comparison.new_leaves} in the later one" + ) + if comparison.removals and not allow_removals: + lines.append( + "refusing: the later artifact stops publishing a key the earlier one " + "published. If that is deliberate, re-run with --allow-removals; it is " + "not the same event as a value moving, and ADR-0010 says a domain the " + "layer stopped publishing is written as null rather than dropped." + ) + return "\n".join(lines) + "\n" + + +def main(argv: list[str] | None = None, stdout: TextIO | None = None) -> int: + """``0`` no change, ``1`` changes reported, ``2`` removals refused or unreadable.""" + out = stdout if stdout is not None else sys.stdout + parser = argparse.ArgumentParser( + prog="python -m perimeter.diff", + description=( + "Compare two coverage artifacts leaf by leaf. Offline; reads only the two " + "files given." + ), + ) + parser.add_argument("old", type=Path, help="the earlier artifact") + parser.add_argument("new", type=Path, help="the later artifact") + parser.add_argument( + "--allow-removals", + action="store_true", + help="accept a key the later artifact stops publishing (exit 1 instead of 2)", + ) + parser.add_argument( + "--json", + action="store_true", + help="write the comparison as JSON rows sorted by path", + ) + parser.add_argument( + "--ignore", + action="append", + default=[], + metavar="NAME", + help="ignore any path segment with this name (repeatable, e.g. is_fixture)", + ) + args = parser.parse_args(argv) + + try: + old = read_artifact(args.old) + new = read_artifact(args.new) + except ArtifactUnreadable as exc: + print(f"perimeter.diff: {exc}", file=sys.stderr) + return 2 + + comparison = compare(old, new, ignore=frozenset(args.ignore)) + if args.json: + print( + json.dumps(comparison.as_document(), indent=2, sort_keys=True), + file=out, + ) + else: + print( + render_text(comparison, allow_removals=args.allow_removals), + end="", + file=out, + ) + + if comparison.removals and not args.allow_removals: + return 2 + if comparison.unchanged: + return 0 + return 1 + + +if __name__ == "__main__": # pragma: no cover + raise SystemExit(main()) diff --git a/tests/test_diff.py b/tests/test_diff.py new file mode 100644 index 0000000..c287534 --- /dev/null +++ b/tests/test_diff.py @@ -0,0 +1,310 @@ +"""The artifact diff, and the artifacts it must refuse. + +Per ADR-0004 a gate is not adopted until it has been seen to fail, so most of this module +is documents the comparison has to report on: a removed key, a changed count, a type +change, a reordered list, an empty file, an unparseable one, and a file that is not there. +The positive control matters as much: an artifact compared against itself must report +nothing, or "no change" would just be what this prints when it cannot see. +""" + +from __future__ import annotations + +import io +import json +from pathlib import Path +from typing import Any + +import pytest + +from perimeter.diff import ( + EMPTY_ARRAY, + EMPTY_OBJECT, + ArtifactUnreadable, + compare, + flatten, + main, + read_artifact, + render_text, +) + +ROOT = Path(__file__).resolve().parents[1] +COMMITTED = ROOT / "site" / "data" + + +def artifact() -> dict[str, Any]: + """A small document with every shape these artifacts actually contain.""" + return { + "measurement": "Coverage of something", + "is_fixture": False, + "records": 132522, + "duplicate_signals": [ + {"key": "irwin_id", "reused_keys": 8, "records_sharing_a_key": 23}, + ], + "fields": [ + {"name": "DAMAGE", "present": 132522, "tenths_pct": 1000}, + {"name": "EAVES", "present": 0, "tenths_pct": None}, + ], + "markers": {}, + "years": [], + } + + +def write(tmp_path: Path, name: str, document: dict[str, Any]) -> Path: + path = tmp_path / name + path.write_text(json.dumps(document, indent=2), encoding="utf-8") + return path + + +# --- the walk ----------------------------------------------------------------- + + +def test_every_scalar_gets_a_path() -> None: + leaves = flatten(artifact()) + assert leaves["/records"] == 132522 + assert leaves["/fields[0]/present"] == 132522 + assert leaves["/duplicate_signals[0]/reused_keys"] == 8 + + +def test_a_null_is_a_leaf_with_a_value_not_a_missing_path() -> None: + """ADR-0010: a domain the layer stopped publishing is written as null.""" + leaves = flatten(artifact()) + assert "/fields[1]/tenths_pct" in leaves + assert leaves["/fields[1]/tenths_pct"] is None + + +def test_empty_containers_are_visible_to_the_walk() -> None: + """Otherwise deleting the key outright would compare as no change.""" + leaves = flatten(artifact()) + assert leaves["/markers"] == EMPTY_OBJECT + assert leaves["/years"] == EMPTY_ARRAY + + +def test_a_key_containing_a_slash_cannot_forge_another_path() -> None: + assert flatten({"a/b": 1}) == {"/a~1b": 1} + assert flatten({"a": {"b": 1}}) == {"/a/b": 1} + + +# --- the positive control ----------------------------------------------------- + + +def test_an_artifact_against_itself_reports_nothing() -> None: + result = compare(artifact(), artifact()) + assert result.unchanged + assert result.changes == () + assert result.additions == () + assert result.removals == () + assert result.old_leaves > 0, "a walk that found nothing would also look unchanged" + + +def test_no_change_states_the_population_it_compared() -> None: + text = render_text(compare(artifact(), artifact()), allow_removals=False) + assert "no change" in text + assert "leaves compared" in text + + +@pytest.mark.parametrize("name", ["perimeters-coverage.json", "dins-coverage.json"]) +def test_the_committed_artifacts_compare_equal_to_themselves(name: str) -> None: + """Over the real documents, not only the small one written for this module.""" + document = read_artifact(COMMITTED / name) + result = compare(document, document) + assert result.unchanged + assert result.old_leaves > 500, f"{name} walked to only {result.old_leaves} leaves" + + +# --- the changes it must report ------------------------------------------------ + + +def test_a_changed_count_is_reported_with_both_values() -> None: + later = artifact() + later["records"] = 132600 + result = compare(artifact(), later) + assert not result.unchanged + assert [c.as_row() for c in result.changes] == [ + {"path": "/records", "old": 132522, "new": 132600} + ] + + +def test_a_number_becoming_null_is_a_change_and_never_a_removal() -> None: + later = artifact() + later["fields"][0]["tenths_pct"] = None + result = compare(artifact(), later) + assert result.removals == () + assert result.changes[0].path == "/fields[0]/tenths_pct" + assert result.changes[0].new is None + assert "null" in render_text(result, allow_removals=False) + + +def test_a_type_change_is_reported_even_when_the_values_look_equal() -> None: + """1000 and 1000.0 are equal in Python and are not the same published value.""" + later = artifact() + later["fields"][0]["tenths_pct"] = 1000.0 + result = compare(artifact(), later) + assert len(result.changes) == 1 + assert result.changes[0].is_type_change + assert "[type change]" in render_text(result, allow_removals=False) + + +def test_a_reordered_list_is_reported_rather_than_smoothed_away() -> None: + later = artifact() + later["fields"] = list(reversed(later["fields"])) + result = compare(artifact(), later) + assert result.changes, "these artifacts write lists in a declared order" + + +def test_a_removed_key_is_a_removal_not_a_change() -> None: + later = artifact() + del later["duplicate_signals"] + result = compare(artifact(), later) + assert [r.path for r in result.removals] == [ + "/duplicate_signals[0]/key", + "/duplicate_signals[0]/records_sharing_a_key", + "/duplicate_signals[0]/reused_keys", + ] + assert result.changes == () + + +def test_an_added_key_is_reported_as_an_addition() -> None: + later = artifact() + later["geometry_acquired"] = False + result = compare(artifact(), later) + assert [a.as_row() for a in result.additions] == [ + {"path": "/geometry_acquired", "new": False} + ] + + +def test_rows_are_sorted_by_path_so_two_runs_agree() -> None: + later = artifact() + later["records"] = 1 + later["fields"][0]["present"] = 2 + later["duplicate_signals"][0]["reused_keys"] = 3 + paths = [c.path for c in compare(artifact(), later).changes] + assert paths == sorted(paths) + + +def test_ignore_drops_the_named_segment_and_nothing_else() -> None: + later = artifact() + later["is_fixture"] = True + later["records"] = 1 + assert len(compare(artifact(), later).changes) == 2 + ignored = compare(artifact(), later, ignore=frozenset({"is_fixture"})) + assert [c.path for c in ignored.changes] == ["/records"] + + +# --- the inputs it must refuse ------------------------------------------------- + + +def test_a_missing_file_is_refused(tmp_path: Path) -> None: + with pytest.raises(ArtifactUnreadable, match="no such file"): + read_artifact(tmp_path / "absent.json") + + +def test_an_empty_file_is_refused_rather_than_compared(tmp_path: Path) -> None: + """Two empty files compare equal, which would print 'no change' about nothing. + + The filename here deliberately does not contain the word being matched. It did once, + and a negative control (deleting the emptiness check outright) still passed: the file + then failed as unparseable JSON, and `match="empty"` was satisfied by `empty.json` in + the message. A pattern a path can satisfy is not an assertion about a reason. + """ + path = tmp_path / "nothing-here.json" + path.write_text("", encoding="utf-8") + with pytest.raises(ArtifactUnreadable, match="file is empty"): + read_artifact(path) + + +def test_an_unparseable_file_is_refused(tmp_path: Path) -> None: + path = tmp_path / "broken.json" + path.write_text("{not json", encoding="utf-8") + with pytest.raises(ArtifactUnreadable, match="not parseable"): + read_artifact(path) + + +def test_a_json_document_that_is_not_an_object_is_refused(tmp_path: Path) -> None: + path = tmp_path / "list.json" + path.write_text("[1, 2]", encoding="utf-8") + with pytest.raises(ArtifactUnreadable, match="not a JSON object"): + read_artifact(path) + + +# --- exit codes ---------------------------------------------------------------- + + +def run(argv: list[str]) -> tuple[int, str]: + out = io.StringIO() + code = main(argv, stdout=out) + return code, out.getvalue() + + +def test_identical_artifacts_exit_zero(tmp_path: Path) -> None: + old = write(tmp_path, "old.json", artifact()) + new = write(tmp_path, "new.json", artifact()) + code, text = run([str(old), str(new)]) + assert code == 0 + assert "no change" in text + + +def test_a_changed_value_exits_one(tmp_path: Path) -> None: + later = artifact() + later["records"] = 9 + old = write(tmp_path, "old.json", artifact()) + new = write(tmp_path, "new.json", later) + code, text = run([str(old), str(new)]) + assert code == 1 + assert "/records" in text + assert "132522" in text and "9" in text + + +def test_a_removal_exits_two_and_names_the_key(tmp_path: Path) -> None: + later = artifact() + del later["duplicate_signals"] + old = write(tmp_path, "old.json", artifact()) + new = write(tmp_path, "new.json", later) + code, text = run([str(old), str(new)]) + assert code == 2 + assert "duplicate_signals" in text + assert "--allow-removals" in text + + +def test_the_same_removal_exits_one_when_it_is_declared_deliberate( + tmp_path: Path, +) -> None: + later = artifact() + del later["duplicate_signals"] + old = write(tmp_path, "old.json", artifact()) + new = write(tmp_path, "new.json", later) + code, text = run([str(old), str(new), "--allow-removals"]) + assert code == 1 + assert "duplicate_signals" in text + assert "removed (allowed)" in text + + +def test_an_unreadable_input_exits_two(tmp_path: Path) -> None: + old = write(tmp_path, "old.json", artifact()) + code, _ = run([str(old), str(tmp_path / "absent.json")]) + assert code == 2 + + +def test_json_output_is_byte_identical_on_repeat(tmp_path: Path) -> None: + later = artifact() + later["records"] = 9 + later["fields"][0]["present"] = 4 + old = write(tmp_path, "old.json", artifact()) + new = write(tmp_path, "new.json", later) + first = run([str(old), str(new), "--json"]) + second = run([str(old), str(new), "--json"]) + assert first == second + document = json.loads(first[1]) + assert document["changes"] == [ + {"path": "/fields[0]/present", "old": 132522, "new": 4}, + {"path": "/records", "old": 132522, "new": 9}, + ] + assert document["leaves_compared"]["old"] == document["leaves_compared"]["new"] + + +def test_the_committed_pair_compares_clean_through_the_command(tmp_path: Path) -> None: + """End to end over a real artifact, so the command is exercised on the real shape.""" + source = COMMITTED / "dins-coverage.json" + copy = tmp_path / "copy.json" + copy.write_text(source.read_text(encoding="utf-8"), encoding="utf-8") + code, text = run([str(source), str(copy)]) + assert code == 0, text